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.

node/Makefile

Line 253 in 58ca8c6

@for dirname in test/addons/*/; do \

would be a good place to start.

P.S. Same idea should be in

node/vcbuild.bat

Lines 400 to 405 in 58ca8c6

	for /d %%F in (test\addons\*) do (
	%node_gyp_exe% rebuild ^
	--directory="%%F" ^
	--nodedir="%cd%"
	if !errorlevel! neq 0 exit /b !errorlevel!
	)

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:

node/Makefile

Lines 234 to 304 in 58ca8c6

	ADDONS_BINDING_GYPS := \
	$(filter-out test/addons/??_*/binding.gyp, \
	$(wildcard test/addons/*/binding.gyp))
	ADDONS_BINDING_SOURCES := \
	$(filter-out test/addons/??_*/*.cc, $(wildcard test/addons/*/*.cc)) \
	$(filter-out test/addons/??_*/*.h, $(wildcard test/addons/*/*.h))
	# Implicitly depends on $(NODE_EXE), see the build-addons rule for rationale.
	# Depends on node-gyp package.json so that build-addons is (re)executed when
	# node-gyp is updated as part of an npm update.
	test/addons/.buildstamp: config.gypi \
	deps/npm/node_modules/node-gyp/package.json \
	$(ADDONS_BINDING_GYPS) $(ADDONS_BINDING_SOURCES) \
	deps/uv/include/*.h deps/v8/include/*.h \
	src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h \
	test/addons/.docbuildstamp
	# Cannot use $(wildcard test/addons/*/) here, it's evaluated before
	# embedded addons have been generated from the documentation.
	@for dirname in test/addons/*/; do \
	printf "\nBuilding addon $$PWD/$$dirname\n" ; \
	env MAKEFLAGS="-j1" $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp \
	--loglevel=$(LOGLEVEL) rebuild \
	--python="$(PYTHON)" \
	--directory="$$PWD/$$dirname" \
	--nodedir="$$PWD" || exit 1 ; \
	done
	touch $@
	# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
	# directly because it calls make recursively. The parent make cannot know
	# if the subprocess touched anything so it pessimistically assumes that
	# .buildstamp and .docbuildstamp are out of date and need a rebuild.
	# Just goes to show that recursive make really is harmful...
	# TODO(bnoordhuis) Force rebuild after gyp update.
	build-addons: $(NODE_EXE) test/addons/.buildstamp
	ADDONS_NAPI_BINDING_GYPS := \
	$(filter-out test/addons-napi/??_*/binding.gyp, \
	$(wildcard test/addons-napi/*/binding.gyp))
	ADDONS_NAPI_BINDING_SOURCES := \
	$(filter-out test/addons-napi/??_*/*.cc, $(wildcard test/addons-napi/*/*.cc)) \
	$(filter-out test/addons-napi/??_*/*.h, $(wildcard test/addons-napi/*/*.h))
	# Implicitly depends on $(NODE_EXE), see the build-addons-napi rule for rationale.
	test/addons-napi/.buildstamp: config.gypi \
	deps/npm/node_modules/node-gyp/package.json \
	$(ADDONS_NAPI_BINDING_GYPS) $(ADDONS_NAPI_BINDING_SOURCES) \
	deps/uv/include/*.h deps/v8/include/*.h \
	src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h \
	src/node_api.h src/node_api_types.h
	# Cannot use $(wildcard test/addons-napi/*/) here, it's evaluated before
	# embedded addons have been generated from the documentation.
	@for dirname in test/addons-napi/*/; do \
	printf "\nBuilding addon $$PWD/$$dirname\n" ; \
	env MAKEFLAGS="-j1" $(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp \
	--loglevel=$(LOGLEVEL) rebuild \
	--python="$(PYTHON)" \
	--directory="$$PWD/$$dirname" \
	--nodedir="$$PWD" || exit 1 ; \
	done
	touch $@
	# .buildstamp and .docbuildstamp need $(NODE_EXE) but cannot depend on it
	# directly because it calls make recursively. The parent make cannot know
	# if the subprocess touched anything so it pessimistically assumes that
	# .buildstamp and .docbuildstamp are out of date and need a rebuild.
	# Just goes to show that recursive make really is harmful...
	# TODO(bnoordhuis) Force rebuild after gyp or node-gyp update.
	build-addons-napi: $(NODE_EXE) test/addons-napi/.buildstamp

Leaving behind only the test/addons/.buildstamp and test/addons/.buildstamp targets

[richardlau]

#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