Revert "build,test: make building addon tests less fragile"

This reverts commit d9b59def72c718aaad3eefb6bf43f409ccefe4d2.

Breaks downloadable source tarball builds as we remove some files prior
to creating a tarball but those files are included in the comprehensive
list of dependencies listed in .deps.

Ref: https://github.com/nodejs/node/pull/17407
PR-URL: https://github.com/nodejs/node/pull/18287
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

1 files changed, 177 insertions, 85 deletions

diff --git a/Makefile b/Makefile
index eaf0710987..66bca71097 100644
--- a/Makefile
+++ b/Makefile
@@ -1,4 +1,3 @@
--include .deps  # Generated by GYP.
 -include config.mk
 
 BUILDTYPE ?= Release
@@ -66,9 +65,9 @@ V ?= 1
 # BUILDTYPE=Debug builds both release and debug builds. If you want to compile
 # just the debug build, run `make -C out BUILDTYPE=Debug` instead.
 ifeq ($(BUILDTYPE),Release)
-all: $(NODE_EXE) ## Default target, builds node in out/Release/node.
+all: out/Makefile $(NODE_EXE) ## Default target, builds node in out/Release/node.
 else
-all: $(NODE_EXE) $(NODE_G_EXE)
+all: out/Makefile $(NODE_EXE) $(NODE_G_EXE)
 endif
 
 .PHONY: help
@@ -78,24 +77,32 @@ help: ## Print help for targets with comments.
 	@grep -E '^[a-zA-Z0-9._-]+:.*?## .*$$' Makefile | sort | \
 		awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-15s\033[0m %s\n", $$1, $$2}'
 
-$(NODE_EXE): out/Release/node
-	ln -fs $< $@
+# The .PHONY is needed to ensure that we recursively use the out/Makefile
+# to check for changes.
+.PHONY: $(NODE_EXE) $(NODE_G_EXE)
 
-$(NODE_G_EXE): out/Debug/node
-	ln -fs $< $@
-
-out/Release/node: out/Makefile $(ALL_DEPS)
+# The -r/-L check stops it recreating the link if it is already in place,
+# otherwise $(NODE_EXE) being a .PHONY target means it is always re-run.
+# Without the check there is a race condition between the link being deleted
+# and recreated which can break the addons build when running test-ci
+# See comments on the build-addons target for some more info
+$(NODE_EXE): config.gypi out/Makefile
 	$(MAKE) -C out BUILDTYPE=Release V=$(V)
+	if [ ! -r $@ -o ! -L $@ ]; then ln -fs out/Release/$(NODE_EXE) $@; fi
 
-out/Debug/node: out/Makefile $(ALL_DEPS)
+$(NODE_G_EXE): config.gypi out/Makefile
 	$(MAKE) -C out BUILDTYPE=Debug V=$(V)
+	if [ ! -r $@ -o ! -L $@ ]; then ln -fs out/Debug/$(NODE_EXE) $@; fi
 
-out/Makefile .deps: deps/uv/uv.gyp deps/http_parser/http_parser.gyp \
+out/Makefile: common.gypi deps/uv/uv.gyp deps/http_parser/http_parser.gyp \
               deps/zlib/zlib.gyp deps/v8/gypfiles/toolchain.gypi \
               deps/v8/gypfiles/features.gypi deps/v8/src/v8.gyp node.gyp \
-              common.gypi config.gypi
+              config.gypi
 	$(PYTHON) tools/gyp_node.py -f make
 
+config.gypi: configure
+	$(error Missing or stale $@, please run ./$<)
+
 .PHONY: install
 install: all ## Installs node into $PREFIX (default=/usr/local).
 	$(PYTHON) tools/install.py $@ '$(DESTDIR)' '$(PREFIX)'
@@ -225,7 +232,9 @@ v8:
 
 .PHONY: test
 # This does not run tests of third-party libraries inside deps.
-test: all build-addons	## Runs default tests, linters, and builds docs.
+test: all ## Runs default tests, linters, and builds docs.
+	$(MAKE) -s build-addons
+	$(MAKE) -s build-addons-napi
 	$(MAKE) -s doc-only
 	$(MAKE) -s lint
 	$(MAKE) -s cctest
@@ -235,14 +244,18 @@ test: all build-addons	## Runs default tests, linters, and builds docs.
 		$(CI_DOC)
 
 .PHONY: test-only
-test-only: all build-addons	## For a quick test, does not run linter or build docs.
+test-only: all  ## For a quick test, does not run linter or build docs.
+	$(MAKE) build-addons
+	$(MAKE) build-addons-napi
 	$(MAKE) cctest
 	$(PYTHON) tools/test.py --mode=release -J \
 		$(CI_JS_SUITES) \
 		$(CI_NATIVE_SUITES)
 
 # Used by `make coverage-test`
-test-cov: all build-addons
+test-cov: all
+	$(MAKE) build-addons
+	$(MAKE) build-addons-napi
 	# $(MAKE) cctest
 	$(PYTHON) tools/test.py --mode=release -J \
 		$(CI_JS_SUITES) \
@@ -258,53 +271,115 @@ test-valgrind: all
 test-check-deopts: all
 	$(PYTHON) tools/test.py --mode=release --check-deopts parallel sequential -J
 
-ADDON_PREREQS := \
-	common.gypi \
-	config.gypi \
-	deps/npm/node_modules/node-gyp/package.json \
-	src/node.h \
-	src/node_api.h \
-	src/node_api_types.h \
-	src/node_buffer.h \
-	src/node_object_wrap.h \
-	src/node_version.h \
-	$(wildcard deps/openssl/openssl/include/openssl/*.h) \
-	$(wildcard deps/uv/include/*.h) \
-	$(wildcard deps/v8/include/*.h) \
-	$(wildcard deps/zlib/*.h) \
+benchmark/misc/function_call/build/Release/binding.node: all \
+		benchmark/misc/function_call/binding.cc \
+		benchmark/misc/function_call/binding.gyp
+	$(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \
+		--python="$(PYTHON)" \
+		--directory="$(shell pwd)/benchmark/misc/function_call" \
+		--nodedir="$(shell pwd)"
+
+# Implicitly depends on $(NODE_EXE).  We don't depend on it explicitly because
+# it always triggers a rebuild due to it being a .PHONY rule.  See the comment
+# near the build-addons rule for more background.
+test/gc/build/Release/binding.node: test/gc/binding.cc test/gc/binding.gyp
+	$(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp rebuild \
+		--python="$(PYTHON)" \
+		--directory="$(shell pwd)/test/gc" \
+		--nodedir="$(shell pwd)"
+
+DOCBUILDSTAMP_PREREQS = tools/doc/addon-verify.js doc/api/addons.md
 
 ifeq ($(OSTYPE),aix)
-ADDON_PREREQS := $(ADDON_PREREQS) out/$(BUILDTYPE)/node.exp
+DOCBUILDSTAMP_PREREQS := $(DOCBUILDSTAMP_PREREQS) out/$(BUILDTYPE)/node.exp
 endif
 
-ADDON_DIRS := \
-	$(dir benchmark/misc/function_call/ test/gc/ \
-		$(wildcard test/addons-napi/*/binding.gyp) \
-		$(wildcard test/addons/*/binding.gyp))
-
-ADDON_FILES := \
-	$(foreach d, $(ADDON_DIRS), $(d)build/$(BUILDTYPE)/binding.node)
+test/addons/.docbuildstamp: $(DOCBUILDSTAMP_PREREQS)
+	$(RM) -r test/addons/??_*/
+	[ -x $(NODE) ] && $(NODE) $< || node $<
+	touch $@
 
-NODE_GYP := \
-	env MAKEFLAGS="-j1" \
-	$(NODE) deps/npm/node_modules/node-gyp/bin/node-gyp \
-	--loglevel="$(LOGLEVEL)" --nodedir="$(CURDIR)" --python="$(PYTHON)"
+ADDONS_BINDING_GYPS := \
+	$(filter-out test/addons/??_*/binding.gyp, \
+		$(wildcard test/addons/*/binding.gyp))
 
-define do_addon
-$(1)build/Makefile: $(1)binding.gyp common.gypi
-	$(NODE_GYP) --directory=$(1) configure
-$(1)build/Release/.buildstamp: $(1)build/Makefile $(2) $(ADDON_PREREQS)
-	$(NODE_GYP) --directory=$(1) build
-	@touch $$@
-$(1)build/Release/binding.node: $(1)build/Release/.buildstamp
-endef
+ADDONS_BINDING_SOURCES := \
+	$(filter-out test/addons/??_*/*.cc, $(wildcard test/addons/*/*.cc)) \
+	$(filter-out test/addons/??_*/*.h, $(wildcard test/addons/*/*.h))
 
-$(foreach x, $(ADDON_DIRS), \
-	$(eval $(call do_addon,$(x),$(wildcard $(x)/*.{c,cc,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.
+#	Ignore folders without binding.gyp
+#	(https://github.com/nodejs/node/issues/14843)
+	@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 \
+			--python="$(PYTHON)" \
+			--directory="$$PWD/$$dirname" \
+			--nodedir="$$PWD" || exit 1 ; \
+	done
+	touch $@
 
 .PHONY: build-addons
-build-addons: $(NODE)
-	@$(MAKE) -s $(ADDON_FILES)
+# .buildstamp needs $(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 is 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.
+#	Ignore folders without binding.gyp
+#	(https://github.com/nodejs/node/issues/14843)
+	@for dirname in test/addons-napi/*/; 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 \
+			--python="$(PYTHON)" \
+			--directory="$$PWD/$$dirname" \
+			--nodedir="$$PWD" || exit 1 ; \
+	done
+	touch $@
+
+.PHONY: build-addons-napi
+# .buildstamp needs $(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 is 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
 
 .PHONY: clear-stalled
 clear-stalled:
@@ -323,11 +398,15 @@ test-gc: all test/gc/build/Release/binding.node
 test-gc-clean:
 	$(RM) -r test/gc/build
 
+test-build: | all build-addons build-addons-napi
+
+test-build-addons-napi: all build-addons-napi
+
 .PHONY: test-all
-test-all: test/gc/build/Release/binding.node ## Run everything in test/.
+test-all: test-build test/gc/build/Release/binding.node ## Run everything in test/.
 	$(PYTHON) tools/test.py --mode=debug,release
 
-test-all-valgrind: build-addons
+test-all-valgrind: test-build
 	$(PYTHON) tools/test.py --mode=debug,release --valgrind
 
 CI_NATIVE_SUITES ?= addons addons-napi
@@ -338,7 +417,7 @@ CI_DOC := doctool
 # Build and test addons without building anything else
 # Related CI job: node-test-commit-arm-fanned
 test-ci-native: LOGLEVEL := info
-test-ci-native: $(ADDON_FILES)
+test-ci-native: | test/addons/.buildstamp test/addons-napi/.buildstamp
 	$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
 		--mode=release --flaky-tests=$(FLAKY_TESTS) \
 		$(TEST_CI_ARGS) $(CI_NATIVE_SUITES)
@@ -360,7 +439,7 @@ test-ci-js: | clear-stalled
 .PHONY: test-ci
 # Related CI jobs: most CI tests, excluding node-test-commit-arm-fanned
 test-ci: LOGLEVEL := info
-test-ci: build-addons | clear-stalled doc-only
+test-ci: | clear-stalled build-addons build-addons-napi doc-only
 	out/Release/cctest --gtest_output=tap:cctest.tap
 	$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
 		--mode=release --flaky-tests=$(FLAKY_TESTS) \
@@ -389,13 +468,13 @@ build-ci:
 run-ci: build-ci
 	$(MAKE) test-ci
 
-test-release: build-addons
+test-release: test-build
 	$(PYTHON) tools/test.py --mode=release
 
-test-debug: build-addons
+test-debug: test-build
 	$(PYTHON) tools/test.py --mode=debug
 
-test-message: all
+test-message: test-build
 	$(PYTHON) tools/test.py message
 
 test-simple: | cctest  # Depends on 'all'.
@@ -435,21 +514,23 @@ test-npm-publish: $(NODE_EXE)
 	npm_package_config_publishtest=true $(NODE) deps/npm/test/run.js
 
 .PHONY: test-addons-napi
-test-addons-napi: build-addons
+test-addons-napi: test-build-addons-napi
 	$(PYTHON) tools/test.py --mode=release addons-napi
 
 .PHONY: test-addons-napi-clean
 test-addons-napi-clean:
 	$(RM) -r test/addons-napi/*/build
+	$(RM) test/addons-napi/.buildstamp
 
 .PHONY: test-addons
-test-addons: build-addons
+test-addons: test-build test-addons-napi
 	$(PYTHON) tools/test.py --mode=release addons
 
 .PHONY: test-addons-clean
 test-addons-clean:
 	$(RM) -r test/addons/??_*/
 	$(RM) -r test/addons/*/build
+	$(RM) test/addons/.buildstamp test/addons/.docbuildstamp
 	$(MAKE) test-addons-napi-clean
 
 test-timers:
@@ -462,7 +543,9 @@ test-timers-clean:
 test-async-hooks:
 	$(PYTHON) tools/test.py --mode=release async-hooks
 
-test-with-async-hooks: build-addons
+test-with-async-hooks:
+	$(MAKE) build-addons
+	$(MAKE) build-addons-napi
 	$(MAKE) cctest
 	NODE_TEST_WITH_ASYNC_HOOKS=1 $(PYTHON) tools/test.py --mode=release -J \
 		$(CI_JS_SUITES) \
@@ -1025,32 +1108,36 @@ lint-js-ci:
 jslint-ci: lint-js-ci
 	@echo "Please use lint-js-ci instead of jslint-ci"
 
+LINT_CPP_ADDON_DOC_FILES = $(wildcard test/addons/??_*/*.cc test/addons/??_*/*.h)
 LINT_CPP_EXCLUDE ?=
 LINT_CPP_EXCLUDE += src/node_root_certs.h
+LINT_CPP_EXCLUDE += $(LINT_CPP_ADDON_DOC_FILES)
+LINT_CPP_EXCLUDE += $(wildcard test/addons-napi/??_*/*.cc test/addons-napi/??_*/*.h)
 # These files were copied more or less verbatim from V8.
 LINT_CPP_EXCLUDE += src/tracing/trace_event.h src/tracing/trace_event_common.h
 
-LINT_CPP_FILES := \
-	$(filter-out \
-		$(LINT_CPP_EXCLUDE), \
-		$(wildcard \
-			benchmark/misc/function_call/binding.cc \
-			src/*.c \
-			src/*.cc \
-			src/*.h \
-			src/*/*.c \
-			src/*/*.cc \
-			src/*/*.h \
-			test/addons/*/*.cc \
-			test/addons/*/*.h \
-			test/cctest/*.cc \
-			test/cctest/*.h \
-			test/addons-napi/*/*.cc \
-			test/addons-napi/*/*.h \
-			test/gc/binding.cc \
-			tools/icu/*.cc \
-			tools/icu/*.h \
-			))
+LINT_CPP_FILES = $(filter-out $(LINT_CPP_EXCLUDE), $(wildcard \
+	benchmark/misc/function_call/binding.cc \
+	src/*.c \
+	src/*.cc \
+	src/*.h \
+	src/*/*.c \
+	src/*/*.cc \
+	src/*/*.h \
+	test/addons/*/*.cc \
+	test/addons/*/*.h \
+	test/cctest/*.cc \
+	test/cctest/*.h \
+	test/addons-napi/*/*.cc \
+	test/addons-napi/*/*.h \
+	test/gc/binding.cc \
+	tools/icu/*.cc \
+	tools/icu/*.h \
+	))
+
+# Code blocks don't have newline at the end,
+# and the actual filename is generated so it won't match header guards
+ADDON_DOC_LINT_FLAGS=-whitespace/ending_newline,-build/header_guard
 
 .PHONY: lint-cpp
 # Lints the C++ code with cpplint.py and check-imports.py.
@@ -1062,6 +1149,10 @@ tools/.cpplintstamp: $(LINT_CPP_FILES)
 	@$(PYTHON) tools/check-imports.py
 	@touch $@
 
+lint-addon-docs: test/addons/.docbuildstamp
+	@echo "Running C++ linter on addon docs..."
+	@$(PYTHON) tools/cpplint.py --filter=$(ADDON_DOC_LINT_FLAGS) $(LINT_CPP_ADDON_DOC_FILES)
+
 cpplint: lint-cpp
 	@echo "Please use lint-cpp instead of cpplint"
 
@@ -1072,11 +1163,12 @@ lint: ## Run JS, C++, MD and doc linters.
 	@EXIT_STATUS=0 ; \
 	$(MAKE) lint-js || EXIT_STATUS=$$? ; \
 	$(MAKE) lint-cpp || EXIT_STATUS=$$? ; \
+	$(MAKE) lint-addon-docs || EXIT_STATUS=$$? ; \
 	exit $$EXIT_STATUS
 CONFLICT_RE=^>>>>>>> [0-9A-Fa-f]+|^<<<<<<< [A-Za-z]+
 
 # Related CI job: node-test-linter
-lint-ci: lint-js-ci lint-cpp lint-md
+lint-ci: lint-js-ci lint-cpp lint-md lint-addon-docs
 	@if ! ( grep -IEqrs "$(CONFLICT_RE)" benchmark deps doc lib src test tools ) \
 		&& ! ( find . -maxdepth 1 -type f | xargs grep -IEqs "$(CONFLICT_RE)" ); then \
 		exit 0 ; \