build,test: make building addon tests less fragile #17407

bnoordhuis · 2017-11-30T23:42:35Z

get rid of recursive make for building the node binary
use module name 'binding' in addons.md and addons-napi/*/binding.gyp
make addons-verify.js write out dependencies for generated add-ons
make gyp write out dependencies for all static deps

This is prep work for a dusted off and super-powered version of #12231.

make finally works as intended now:

$ make
make: Nothing to be done for 'all'.

And make -j8 test (finally!) builds add-ons in parallel.

~~CI: https://ci.nodejs.org/job/node-test-pull-request/11830/~~
CI: https://ci.nodejs.org/job/node-test-pull-request/11831/

gibfahn · 2017-12-01T14:19:20Z

Awesome, I'll get that CI stuff working post-haste

make finally works as intended now:

Can you elaborate? Is the point that if nothing has changed it doesn't still do a load of stuff?

richardlau · 2017-12-01T17:25:13Z

tools/gyp/pylib/gyp/generator/make.py

@@ -2227,3 +2234,10 @@ def CalculateMakefilePath(build_file, base_name):
  root_makefile.write(SHARED_FOOTER)

  root_makefile.close()
+
+  # Hack to get rid of $(obj)/path/to/foo.o deps that node.gyp adds manually.


Should we float the gyp changes in a separate commit in case we lose them in a future gyp update?

Good idea. I'll split it out before landing.

bnoordhuis · 2017-12-01T17:57:43Z

Can you elaborate? Is the point that if nothing has changed it doesn't still do a load of stuff?

Yep. Right now it always does this:

$ make
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C out BUILDTYPE=Release V=1
  touch d271701384a263aa7913b46019f582dfe64aaf59.intermediate
  LD_LIBRARY_PATH=/Users/bnoordhuis/src/v1.x/out/Release/lib.host:/Users/bnoordhuis/src/v1.x/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8/src/inspector; mkdir -p /Users/bnoordhuis/src/v1.x/out/Release/obj/gen/src/inspector/protocol /Users/bnoordhuis/src/v1.x/out/Release/obj/gen/include/inspector; python ../../third_party/inspector_protocol/CodeGenerator.py --jinja_dir ../../third_party --output_base "/Users/bnoordhuis/src/v1.x/out/Release/obj/gen/src/inspector" --config inspector_protocol_config.json
  LD_LIBRARY_PATH=/Users/bnoordhuis/src/v1.x/out/Release/lib.host:/Users/bnoordhuis/src/v1.x/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8/src/inspector; mkdir -p /Users/bnoordhuis/src/v1.x/out/Release/obj/gen/src; python ../../third_party/inspector_protocol/CheckProtocolCompatibility.py --stamp "/Users/bnoordhuis/src/v1.x/out/Release/obj/gen/src/js_protocol.stamp" js_protocol.json
if [ ! -r node -o ! -L node ]; then ln -fs out/Release/node node; fi

richardlau · 2017-12-02T00:32:47Z

Makefile

@@ -601,11 +530,8 @@ docopen: $(apidocs_html)
 docclean:
 	$(RM) -r out/doc

-build-ci:
+run-ci:


cc @nodejs/build in case there are any jobs directly running the build-ci target.

richardlau · 2017-12-02T00:54:39Z

Makefile

+$(NODE_G_EXE): out/Debug/node out/Makefile
+	ln -fs $< $@
+
+out/Release/node: $(ALL_DEPS)


Does $(ALL_DEPS) include out/Makefile? Since this target recursively executes make it should have a dependency on the makefile.

joyeecheung · 2017-12-03T13:15:55Z

Makefile

@@ -601,11 +530,8 @@ docopen: $(apidocs_html)
 docclean:
 	$(RM) -r out/doc

-build-ci:
+run-ci:


For some reason the CI is complaining

+ make run-ci -j 2 make -C out BUILDTYPE=Release V=1 make[1]: *** out: No such file or directory. Stop. Makefile:91: *** Missing or stale config.gypi, please run ./configure. Stop. Build step 'Execute shell' marked build as failure

BridgeAR · 2017-12-11T05:52:11Z

Ping @bnoordhuis

This seems pretty nice but since the Makefile is a place where things change often, I hope we can get this done fast and land it.

bnoordhuis · 2017-12-11T11:10:51Z

@BridgeAR IIRC, it needs some CI tweaks and @gibfahn was going to look at that.

gibfahn · 2017-12-11T12:14:12Z

@BridgeAR IIRC, it needs some CI tweaks and @gibfahn was going to look at that.

I don't recall saying I was going to look at this. I'm happy to, but the CI we were talking about was a build with cmake.

CI fails as this needs a rebase. https://ci.nodejs.org/job/node-test-commit/14743/console

bnoordhuis · 2017-12-15T21:43:51Z

Rebased and I decided to check in the documentation add-ons. They don't change often enough to go through the trouble of generating them dynamically. Too race-y, too special.

~~https://ci.nodejs.org/job/node-test-commit/14876/ - mostly green except for an inscrutable AIX issue.~~
https://ci.nodejs.org/job/node-test-commit/14888/

bnoordhuis · 2017-12-16T22:07:22Z

Glorious green! Please (re)review. The biggest change from the previous changeset:

Check in auto-generated add-on tests from doc/api/addons.md. The
files change rarely and generating them dynamically causes no end of
race conditions and special-casing during the build.

richardlau · 2017-12-17T00:27:11Z

Makefile

              deps/zlib/zlib.gyp deps/v8/gypfiles/toolchain.gypi \
              deps/v8/gypfiles/features.gypi deps/v8/src/v8.gyp node.gyp \
-              config.gypi
+              common.gypi


Is config.gypi intentionally dropped here?

Yes, but it's no longer necessary. I've pushed up a commit that restores it.

I take back what I wrote - it's still necessary because of the config.gypi: configure rule below. But since it's correct to depend on config.gypi here, I'll just remove the other rule.

gibfahn · 2017-12-17T12:28:11Z

doc/api/addons.md

@@ -1,3 +1,6 @@
+<!--
+Run `node tools/doc/addon-verify.js` when you change examples in this document.


Is there a way we could validate this? Maybe a test that contains the shasum of everything inside a js code block in this file, that you have to update when it changes.

I guess tools/doc/addon-verify.js would be the place to do it.

I've added a commit that adds a --check flag to addon-verify.js and calls that from make test.

richardlau · 2017-12-18T15:01:13Z

Makefile

-test: all ## Default test target. Runs default tests, linters, and builds docs.
-	$(MAKE) -s build-addons
-	$(MAKE) -s build-addons-napi
+check-doc-addons: $(NODE)


This should be marked as phony.

bnoordhuis · 2017-12-19T15:28:44Z

~~https://ci.nodejs.org/job/node-test-commit/14948/~~
https://ci.nodejs.org/job/node-test-commit/14949/

richardlau · 2017-12-19T16:21:51Z

With the config.gypi rule removed, we now get this which is probably less helpful for newcomers:

-bash-4.2$ git clean -fdX
-bash-4.2$ make
make: *** No rule to make target `config.gypi', needed by `out/Makefile'.  Stop.
-bash-4.2$

Is there a reason the makefile doesn't just run configure to generate config.gypi? In most cases it should yield sensible defaults(?).

bnoordhuis · 2017-12-19T16:25:14Z

Is there a reason the makefile doesn't just run configure to generate config.gypi?

It used to and it was supremely annoying because it overwrote your settings. I removed it a few years ago.

edit: to expand on that, you'd get situations like these:

$ ./configure --prefix=/path/to/dir
# edit some files, time passes
$ make -j8 install
# runs configure again but without --prefix and installs to the wrong location

gibfahn · 2017-12-19T16:44:34Z

With the config.gypi rule removed, we now get this which is probably less helpful for newcomers:

Is there a reason we can't have the config.gypi rule with these changes? I'm a fan of the more helpful error message.

Is there a reason the makefile doesn't just run configure to generate config.gypi?

It used to and it was supremely annoying because it overwrote your settings. I removed it a few years ago.

How about only generating a new config.gypi if there isn't one there already?

Ensure `common.tmpDir` exists before trying to chdir into it. Fixes a "ENOENT: no such file or directory, uv_chdir" error when the temporary directory is removed before running the test. PR-URL: #17407 Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Make GYP write a .deps file in the top-level directory that we can use in the Makefile to get a proper dependency chain for the `node` target. Preparatory work for getting rid of recursive make invocations. PR-URL: nodejs#17407 Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Make the script synchronous and clean it up. This is preparatory work for follow-ups commits. PR-URL: nodejs#17407 Reviewed-By: Richard Lau <riclau@uk.ibm.com>

* Get rid of recursive `make` when building the node binary. An earlier commit makes GYP write out rules that we can use for proper dependency tracking. * Use module name 'binding' in addons.md and addons-napi/*/binding.gyp. This massively simplifies the logic for generating the build rules. * Check in auto-generated add-on tests from `doc/api/addons.md`. The files change rarely and generating them dynamically causes no end of race conditions and special-casing during the build. PR-URL: nodejs#17407 Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Add a `--check` flag to `tools/doc/addon-verify.js` and use that in the `make test` target to determine if the generated files in `test/addons` are fresh or stale. PR-URL: nodejs#17407 Reviewed-By: Richard Lau <riclau@uk.ibm.com>

Ensure `common.tmpDir` exists before trying to chdir into it. Fixes a "ENOENT: no such file or directory, uv_chdir" error when the temporary directory is removed before running the test. PR-URL: nodejs#17407 Reviewed-By: Richard Lau <riclau@uk.ibm.com>

This reverts commit 2cb9e2a. Reverted along with d9b59de as this introduces freshness checks that are too stringent without the comprehensive dependency checking of introduced in d9b59de so `make test` won't work with this. Ref: nodejs#17407 PR-URL: nodejs#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>

This reverts commit d9b59de. 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: nodejs#17407 PR-URL: nodejs#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>

This reverts commit c668263. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs. Ref: nodejs#17407 PR-URL: nodejs#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>

This reverts commit 920c132. Reverted along with d9b59de as this breaks compilation from downloadable source tarballs. Ref: nodejs#17407 PR-URL: nodejs#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>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 30, 2017

richardlau reviewed Dec 1, 2017

View reviewed changes

richardlau reviewed Dec 2, 2017

View reviewed changes

joyeecheung reviewed Dec 3, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

joyeecheung mentioned this pull request Dec 11, 2017

build: refactor addon targets #17115

Closed

4 tasks

bnoordhuis force-pushed the better-addon-building branch 2 times, most recently from 266bb7e to 3416edf Compare December 15, 2017 20:36

bnoordhuis force-pushed the better-addon-building branch from fc07832 to e496053 Compare December 16, 2017 20:31

richardlau reviewed Dec 17, 2017

View reviewed changes

bnoordhuis force-pushed the better-addon-building branch from e496053 to 05ca6c7 Compare December 17, 2017 10:46

gibfahn reviewed Dec 17, 2017

View reviewed changes

bnoordhuis force-pushed the better-addon-building branch from 05ca6c7 to 2151423 Compare December 17, 2017 22:30

richardlau reviewed Dec 18, 2017

View reviewed changes

bnoordhuis force-pushed the better-addon-building branch from 2151423 to 39931b4 Compare December 19, 2017 15:26

richardlau approved these changes Dec 19, 2017

View reviewed changes

This was referenced Dec 25, 2017

Addon build failures on debian8-64 test-rackspace-debian8-x64-2 nodejs/build#1049

Closed

n-api: add API for asynchronous functions #17809

Closed

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added dont-land-on-v6.x labels Mar 20, 2018

Trott mentioned this pull request Aug 12, 2018

doc: require two approvals to land changes #22255

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build,test: make building addon tests less fragile #17407

build,test: make building addon tests less fragile #17407

bnoordhuis commented Nov 30, 2017 •

edited

Loading

gibfahn commented Dec 1, 2017

richardlau Dec 1, 2017

bnoordhuis Dec 1, 2017

bnoordhuis commented Dec 1, 2017

richardlau Dec 2, 2017

richardlau Dec 2, 2017

joyeecheung Dec 3, 2017

BridgeAR commented Dec 11, 2017

bnoordhuis commented Dec 11, 2017

gibfahn commented Dec 11, 2017 •

edited

Loading

bnoordhuis commented Dec 15, 2017 •

edited

Loading

bnoordhuis commented Dec 16, 2017

richardlau Dec 17, 2017

bnoordhuis Dec 17, 2017

bnoordhuis Dec 19, 2017

gibfahn Dec 17, 2017 •

edited

Loading

bnoordhuis Dec 17, 2017

richardlau Dec 18, 2017

bnoordhuis Dec 19, 2017

bnoordhuis commented Dec 19, 2017 •

edited

Loading

richardlau commented Dec 19, 2017

bnoordhuis commented Dec 19, 2017 •

edited

Loading

gibfahn commented Dec 19, 2017

		@@ -1,3 +1,6 @@
		<!--
		Run `node tools/doc/addon-verify.js` when you change examples in this document.

build,test: make building addon tests less fragile #17407

build,test: make building addon tests less fragile #17407

Conversation

bnoordhuis commented Nov 30, 2017 • edited Loading

gibfahn commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Dec 11, 2017

bnoordhuis commented Dec 11, 2017

gibfahn commented Dec 11, 2017 • edited Loading

bnoordhuis commented Dec 15, 2017 • edited Loading

bnoordhuis commented Dec 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Dec 19, 2017 • edited Loading

richardlau commented Dec 19, 2017

bnoordhuis commented Dec 19, 2017 • edited Loading

gibfahn commented Dec 19, 2017

bnoordhuis commented Nov 30, 2017 •

edited

Loading

gibfahn commented Dec 11, 2017 •

edited

Loading

bnoordhuis commented Dec 15, 2017 •

edited

Loading

gibfahn Dec 17, 2017 •

edited

Loading

bnoordhuis commented Dec 19, 2017 •

edited

Loading

bnoordhuis commented Dec 19, 2017 •

edited

Loading