build: refactor addon targets #17115
Conversation
nodejs-github-bot
added
the
build
joyeecheung
force-pushed
the
refactor-addon
branch
2 times, most recently
from
f95c1ba
to
f82eade
Compare
- Extracts the common bits out as ADDON_PREREQS, build-single-addon, build-multiple-addon - Changes `test-ci` to depend on the build stamps instead of the build targets, since those rely on `$(NODE_EXE)` and would trigger a build/linking every time they get run. - Checks that `./node -> out/Release/node` is not a broken link, that is, `out/Release/node` is not deleted
joyeecheung
force-pushed
the
refactor-addon
branch
from
f82eade
to
ed39b0e
Compare
| @@ -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 \ | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| build-multiple-addon = \ | ||
| for dirname in $(1); do \ | ||
| if [ ! -f $(PWD)/$$dirname/binding.gyp ]; then \ | ||
| continue; \ |
There was a problem hiding this comment.
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 🤷♂️ .
There was a problem hiding this comment.
@gibfahn We could add an environment variable that controls whether we want to skip this, can be done in another PR though.
| 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 = \ |
There was a problem hiding this comment.
Bikeshed, but maybe just build-addon?
There was a problem hiding this comment.
@gibfahn There is already build-addons so if we follow the same logic build-multiple-addon is going to have a conflict..
| fi; \ | ||
| if [ ! -x $(NODE_FULL_PATH) ] || [ ! -e $(NODE_FULL_PATH) ]; then \ | ||
| echo "$(NODE_FULL_PATH) is not a valid link"; \ | ||
| exit 1; \ |
There was a problem hiding this comment.
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?
| exit 1; \ | ||
| fi; \ | ||
| printf "\nBuilding addon $(PWD)/$$dirname\n" ; \ | ||
| env MAKEFLAGS="-j1" $(call build-single-addon,$(PWD)/$$dirname) || exit 1; \ |
There was a problem hiding this comment.
Does it not fail with an error if you don't include the || exit 1?
There was a problem hiding this comment.
@gibfahn I think it didn't the last time I tried it, I will double check
| --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) |
There was a problem hiding this comment.
Maybe worth leaving as is anyway, but does this still work if you do $(call build-single-addon, test/gc)?
| @@ -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 \ | |||
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Ping @joyeecheung @gibfahn |
|
Makes sense. I did not see that one yet. |
|
Closing since #17407 has landed (although it's about to be reverted but a fix should follow soon) |
build-multiple-addon
test-cito depend on the build stamps instead of thebuild targets, since those rely on
$(NODE_EXE)and would triggera build/linking every time they get run.
./node -> out/Release/nodeis not a broken link,that is,
out/Release/nodeis not deletedThis would probably help investigating #17043 where the link seems to get broken in the middle of a addon build.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build