Node: change test/addons/ to ignore empty folders

[Fishrock123]

• Version: all with /addons/ tests
• Platform: at least macOS and Linux
• Subsystem: test

When changing branches sometimes git seems to leave empty folders behind which in the case of the test/addons/ directory causes at least gyp to explode with an error similar to:

Building addon /Users/Jeremiah/Documents/node/test/addons/async-hooks-id/
gyp: binding.gyp not found (cwd: /Users/Jeremiah/Documents/node/test/addons/async-hooks-id) while trying to load binding.gyp
Makefile:192: recipe for target 'test/addons/.buildstamp' failed
make[1]: *** [test/addons/.buildstamp] Error 1
make[1]: Leaving directory '/Users/Jeremiah/Documents/node'
Makefile:133: recipe for target 'test' failed
make: *** [test] Error 2

[addaleax]

@Fishrock123 This is not a Python thing, build was appropriate (the error pops up while building the addons from our Makefile, not in the test runner) :)

[refack]

If anyone want to tackle this but hits the GYP wall, feel free to ping me.

[vnktram]

Working on it.

[vnktram]

Unable to replicate even after switching between branches.

[bnoordhuis]

It only happens when switching branches leaves an empty directory under test/addons or test/addons-napi. Not a big deal though, git clean -dfx test/addons* will fix it.

[vnktram]

I have been running make test and the tests pass successfully. I removed the empty folders by running git clean -dfx test/addons* as suggested by @bnoordhuis. I still have no luck in replicating the issue.

[shivanth]

Try switching to a staging branch, say v6.x-staging. I ran into this error with that branch yesterday :-)

[vnktram]

I was able to replicate it and while trying to look for empty directories in addons, I realized that the directories which we deem to be empty do have a build folder and a config.gypi file inside it. So should we look if each of these addons folder contain a binding.gyp file instead?

[vnktram]

@refack Can you guide me through what needs to be the best fix here?

[refack]

So should we look if each of these addons folder contain a binding.gyp file instead?

This sounds good to me. https://github.com/nodejs/node/blob/58ca8c68c400919d88b03dded6c24f9f1d31250b/Makefile#L253
would be a good place to start.

P.S. Same idea should be in https://github.com/nodejs/node/blob/58ca8c68c400919d88b03dded6c24f9f1d31250b/vcbuild.bat#L400-L405

P.P.S. both of these have build-napi-addons counterparts, that should be treated the same.

[refack]

[Optional improvement]

After looking at both Makefile and vcbuild.bat IMHO an optimal solution would be to the addon building logic into a new JS script in tools. A good name might be /tools/make-addons.js.

AFAICT this whole section should be refactored out:
https://github.com/nodejs/node/blob/58ca8c68c400919d88b03dded6c24f9f1d31250b/Makefile#L234-L304
Leaving behind only the test/addons/.buildstamp and test/addons/.buildstamp targets

[richardlau]

https://github.com/nodejs/node/pull/12231 refactors out much of the logic for building addons to .js files.

[gr2m]

I’m looking into this with @MylesBorins

[gr2m]

This fixes the binding.gyp not found bug

diff --git a/Makefile b/Makefile
index d917056..c5d96eb 100644
--- a/Makefile
+++ b/Makefile
@@ -268,6 +268,8 @@ test/addons/.buildstamp: config.gypi \
 #  Cannot use $(wildcard test/addons/*/) here, it's evaluated before
 #  embedded addons have been generated from the documentation.
    @for dirname in test/addons/*/; do \
+       if [ ! -f "$$PWD/${$dirname}binding.gyp" ]; then \
+           continue; fi ; \
        printf "\nBuilding addon $$PWD/$$dirname\n" ; \
        env MAKEFLAGS="-j1" $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp \
                --loglevel=$(LOGLEVEL) rebuild \

but now getting Error: Cannot find module './build/Release/addon' :)

[gr2m]

nevermind, the if statement was wrong and we got false negativse. PR incoming