Skip to content

Commit 72bd659

Browse files
committed
build: improve Makefile linking
It appears that our Makefile was performing some redundant build steps in CI, adding time to each CI run. This tries to improve things. Signed-off-by: James M Snell <jasnell@gmail.com>
1 parent cb9b870 commit 72bd659

1 file changed

Lines changed: 27 additions & 10 deletions

File tree

‎Makefile‎

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,23 @@ EXEEXT := $(shell $(PYTHON) -c \
8181

8282
NODE_EXE = node$(EXEEXT)
8383
NODE_G_EXE = node_g$(EXEEXT)
84+
85+
# Recursive CI targets set this after the main build has completed. Only the
86+
# expected recursive goals may omit the addon dependency on $(NODE_EXE).
87+
NODEJS_INTERNAL_CI_AFTER_BUILD :=
88+
ADDON_NODE_EXE_PREREQ := $(NODE_EXE)
89+
ifeq ($(origin NODEJS_INTERNAL_CI_AFTER_BUILD),command line)
90+
ifeq ($(NODEJS_INTERNAL_CI_AFTER_BUILD),1)
91+
ifneq ($(MAKELEVEL),0)
92+
ifeq ($(words $(MAKECMDGOALS)),1)
93+
ifneq ($(filter build-ffi-tests test-ci,$(MAKECMDGOALS)),)
94+
ADDON_NODE_EXE_PREREQ :=
95+
endif
96+
endif
97+
endif
98+
endif
99+
endif
100+
84101
NPM ?= ./deps/npm/bin/npm-cli.js
85102

86103
# Release build of node.
@@ -459,7 +476,7 @@ test/addons/.buildstamp: $(ADDONS_PREREQS) \
459476
# .buildstamp is out of date and need a rebuild.
460477
# Just goes to show that recursive make really is harmful...
461478
# TODO(bnoordhuis) Force rebuild after gyp update.
462-
build-addons: | $(NODE_EXE) test/addons/.buildstamp ## Build addons for Node.js.
479+
build-addons: | $(ADDON_NODE_EXE_PREREQ) test/addons/.buildstamp ## Build addons for Node.js.
463480

464481
JS_NATIVE_API_BINDING_GYPS := \
465482
$(filter-out test/js-native-api/??_*/binding.gyp, \
@@ -483,7 +500,7 @@ test/js-native-api/.buildstamp: $(ADDONS_PREREQS) \
483500
# .buildstamp is out of date and need a rebuild.
484501
# Just goes to show that recursive make really is harmful...
485502
# TODO(bnoordhuis) Force rebuild after gyp or node-gyp update.
486-
build-js-native-api-tests: | $(NODE_EXE) test/js-native-api/.buildstamp ## Build JS Native-API tests.
503+
build-js-native-api-tests: | $(ADDON_NODE_EXE_PREREQ) test/js-native-api/.buildstamp ## Build JS Native-API tests.
487504

488505
NODE_API_BINDING_GYPS := \
489506
$(filter-out test/node-api/??_*/binding.gyp, \
@@ -507,7 +524,7 @@ test/node-api/.buildstamp: $(ADDONS_PREREQS) \
507524
# .buildstamp is out of date and need a rebuild.
508525
# Just goes to show that recursive make really is harmful...
509526
# TODO(bnoordhuis) Force rebuild after gyp or node-gyp update.
510-
build-node-api-tests: | $(NODE_EXE) test/node-api/.buildstamp ## Build Node-API tests.
527+
build-node-api-tests: | $(ADDON_NODE_EXE_PREREQ) test/node-api/.buildstamp ## Build Node-API tests.
511528

512529
BENCHMARK_NAPI_BINDING_GYPS := $(wildcard benchmark/napi/*/binding.gyp)
513530

@@ -540,7 +557,7 @@ ifndef NOSQLITE
540557
# directly because it calls make recursively. The parent make cannot know
541558
# if the subprocess touched anything so it pessimistically assumes that
542559
# .buildstamp is out of date and need a rebuild.
543-
build-sqlite-tests: | $(NODE_EXE) test/sqlite/.buildstamp ## Build SQLite tests.
560+
build-sqlite-tests: | $(ADDON_NODE_EXE_PREREQ) test/sqlite/.buildstamp ## Build SQLite tests.
544561
else
545562
build-sqlite-tests:
546563
endif
@@ -552,10 +569,10 @@ FFI_BINDING_SOURCES := \
552569
$(wildcard test/ffi/*/*.def)
553570

554571
ifndef NOFFI
555-
# Depends on $(NODE_EXE) as order-only to avoid ETXTBSY on AIX when make
556-
# tries to execute node while it is still being linked in parallel.
572+
# Normally depends on $(NODE_EXE) as order-only to avoid ETXTBSY on AIX when
573+
# make tries to execute node while it is still being linked in parallel.
557574
test/ffi/.buildstamp: $(ADDONS_PREREQS) \
558-
$(FFI_BINDING_GYPS) $(FFI_BINDING_SOURCES) | $(NODE_EXE)
575+
$(FFI_BINDING_GYPS) $(FFI_BINDING_SOURCES) | $(ADDON_NODE_EXE_PREREQ)
559576
@$(call run_build_addons,"$$PWD/test/ffi",$@)
560577
else
561578
test/ffi/.buildstamp:
@@ -660,7 +677,7 @@ test-ci: | clear-stalled bench-addons-build build-addons build-js-native-api-tes
660677
build-ci: ## Build everything (CI).
661678
$(PYTHON) ./configure --verbose $(CONFIG_FLAGS)
662679
$(MAKE)
663-
$(MAKE) build-ffi-tests
680+
$(MAKE) build-ffi-tests NODEJS_INTERNAL_CI_AFTER_BUILD=1
664681

665682
.PHONY: run-ci
666683
# Run by CI tests, exceptions:
@@ -673,7 +690,7 @@ build-ci: ## Build everything (CI).
673690
# Using -j1 as the sub target in `test-ci` already have internal parallelism.
674691
# Refs: https://github.com/nodejs/node/pull/23733
675692
run-ci: build-ci ## Build and run all tests (CI).
676-
$(MAKE) test-ci -j1
693+
$(MAKE) test-ci -j1 NODEJS_INTERNAL_CI_AFTER_BUILD=1
677694

678695
.PHONY: test-release
679696
.PHONY: test-debug
@@ -1476,7 +1493,7 @@ bench bench-all: bench-addons-build
14761493
$(warning Please use benchmark/run.js or benchmark/compare.js to run the benchmarks.)
14771494

14781495
.PHONY: bench-addons-build
1479-
bench-addons-build: | $(NODE_EXE) benchmark/napi/.buildstamp ## Build required addons for benchmark before running it.
1496+
bench-addons-build: | $(ADDON_NODE_EXE_PREREQ) benchmark/napi/.buildstamp ## Build required addons for benchmark before running it.
14801497

14811498
.PHONY: bench-addons-clean
14821499
.NOTPARALLEL: bench-addons-clean

0 commit comments

Comments
 (0)