Skip to content

build: refactor addon targets #17115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
joyeecheung wants to merge 1 commit into from
Closed

build: refactor addon targets #17115

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 41 additions & 42 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ NODE_EXE = node$(EXEEXT)
NODE ?= ./$(NODE_EXE)
NODE_G_EXE = node_g$(EXEEXT)
NPM ?= ./deps/npm/bin/npm-cli.js
NODE_FULL_PATH = $(PWD)/$(NODE_EXE)
Copy link
Member

@gibfahn gibfahn Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need $(NODE), and $(NODE_FULL_PATH)?

I'm guessing the reason you need the full path is because the script might do cd x; $(NODE)? If so why not just replace $(NODE) with the absolute path version?

NODE ?= $(PWD)/node$(EXEEXT)

It seems that the purpose of $(NODE_EXE) is so that you can pass a custom $NODE but still have ./node as a target. If so, while you're here, maybe add a comment to NODE_EXE to explain that?

Copy link
Member Author

@joyeecheung joyeecheung Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @refack mentioned he sets NODE before running make sometimes, so I am not sure about replacing the whole thing with a absolute path. I would prefer just replacing NODE as well. Also does this get set in the CI anywhere? (@rvagg)

Copy link
Member

@gibfahn gibfahn Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @refack mentioned he sets NODE before running make sometimes, so I am not sure about replacing the whole thing with a absolute path.

I'm pretty sure ?= means it will use the NODE passed in if set.

NODE_GYP ?= deps/npm/node_modules/node-gyp/bin/node-gyp

# Flags for packaging.
BUILD_DOWNLOAD_FLAGS ?= --download=all
Expand Down Expand Up @@ -247,22 +249,46 @@ test-valgrind: all
test-check-deopts: all
$(PYTHON) tools/test.py --mode=release --check-deopts parallel sequential -J

ADDON_PREREQS = config.gypi \
deps/npm/node_modules/node-gyp/package.json \
deps/uv/include/*.h deps/v8/include/*.h \
src/node.h src/node_buffer.h src/node_object_wrap.h src/node_version.h

build-single-addon = \
Copy link
Member

@gibfahn gibfahn Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed, but maybe just build-addon?

Copy link
Member Author

@joyeecheung joyeecheung Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gibfahn There is already build-addons so if we follow the same logic build-multiple-addon is going to have a conflict..

$(NODE_FULL_PATH) $(NODE_GYP) \
--loglevel=$(LOGLEVEL) rebuild \
--python="$(PYTHON)" \
--directory="$(1)" \
--nodedir="$(PWD)"

# Ignore folders without binding.gyp
# (https://github.com/nodejs/node/issues/14843)
# Exit if the local node build is not valid
build-multiple-addon = \
for dirname in $(1); do \
if [ ! -f $(PWD)/$$dirname/binding.gyp ]; then \
continue; \
Copy link
Member

@gibfahn gibfahn Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was already there, but silently skipping folders without a binding.gyp seems a bit error-prone to me. Don't have a better solution though (except for #12231) so 🤷‍♂️ .

Copy link
Member Author

@joyeecheung joyeecheung Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gibfahn We could add an environment variable that controls whether we want to skip this, can be done in another PR though.

fi; \
if [ ! -x $(NODE_FULL_PATH) ] || [ ! -e $(NODE_FULL_PATH) ]; then \
echo "$(NODE_FULL_PATH) is not a valid link"; \
exit 1; \
Copy link
Member

@gibfahn gibfahn Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you don't do this check? Don't you get No such file or directory: /path/to/node? If so is this needed?

Copy link
Member Author

@joyeecheung joyeecheung Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gibfahn The intent was to know a if the link is deleted, or is it just broken (out/Release/node gets deleted, like what we were seeing in #17043 ). I should probably separate those into different statments.

fi; \
printf "\nBuilding addon $(PWD)/$$dirname\n" ; \
env MAKEFLAGS="-j1" $(call build-single-addon,$(PWD)/$$dirname) || exit 1; \
Copy link
Member

@gibfahn gibfahn Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it not fail with an error if you don't include the || exit 1?

Copy link
Member Author

@joyeecheung joyeecheung Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gibfahn I think it didn't the last time I tried it, I will double check

done

benchmark/misc/function_call/build/Release/binding.node: all \
$(ADDON_PREREQS) \
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)"
$(call build-single-addon,$(PWD)/benchmark/misc/function_call)

# 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)"
test/gc/build/Release/binding.node: $(ADDON_PREREQS) \
test/gc/binding.cc test/gc/binding.gyp
$(call build-single-addon,$(PWD)/test/gc)
Copy link
Member

@gibfahn gibfahn Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth leaving as is anyway, but does this still work if you do $(call build-single-addon, test/gc)?


DOCBUILDSTAMP_PREREQS = tools/doc/addon-verify.js doc/api/addons.md

Expand All @@ -286,26 +312,12 @@ ADDONS_BINDING_SOURCES := \
# 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 \
test/addons/.buildstamp: $(ADDON_PREREQS) \
$(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
@$(call build-multiple-addon,test/addons/*)
touch $@

# .buildstamp needs $(NODE_EXE) but cannot depend on it
Expand All @@ -325,26 +337,12 @@ ADDONS_NAPI_BINDING_SOURCES := \
$(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 \
test/addons-napi/.buildstamp: $(ADDON_PREREQS) \
$(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
@$(call build-multiple-addon,test/addons-napi/*)
touch $@

# .buildstamp needs $(NODE_EXE) but cannot depend on it
Expand Down Expand Up @@ -404,7 +402,8 @@ test-ci-js: | clear-stalled
fi

test-ci: LOGLEVEL := info
test-ci: | clear-stalled build-addons build-addons-napi doc-only
test-ci: | clear-stalled test/addons/.buildstamp \
Copy link
Member Author

@joyeecheung joyeecheung Nov 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now come to think of it, why doesn't test-ci depend on $(NODE_EXE)? Or depend on build-ci? AFAICT this should work as well, and it looks more natural

test-ci: | clear-stalled build-ci test/addons/.buildstamp test/addons-napi/.buildstamp doc-only

run-ci: test-ci

cc @nodejs/build

Copy link
Member

@gibfahn gibfahn Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that mean test-ci will always run build-ci first? I would like test-ci to depend only on the things it actually needs, so we can run tests on release tarballs etc.

Copy link
Member

@gibfahn gibfahn Nov 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could have a TEST_PREREQS, that lists the files required by all the test targets. Or maybe that's what $(NODE_EXE) is, in which case using that seems fine.

test/addons-napi/.buildstamp 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) \
Expand Down