build: adding config.gypi dep to addons/.buildstamp #7893

danbev · 2016-07-27T10:05:35Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

Description of change

Currently the build-addons target is called as part of the
test target, and it has test/addons/.buildstamp as a dependency.
When running ./configure --debug and later switching/
updating branches you can be in a situation where the config.gypi
files in the addons build directories are using an incorrect
value for default_configuration. Currently this can happen and you
will get test errors as there are a few test that depend on the
value of default_configuration to be Release. When this happens
it might not be obvious to someone new to the project (like me)
to correct this state. This commit attempts to make it easier
to fix this.

The suggestion here is if you find yourself in the above situation
the the following steps would be preformed:

1) make clean
2) ./configure
3) make -j4 test

The target distclean also calls clean-addons so that could be used
instead of clean in the above example. Also clean-addons could be
called directly if required.

This commit is related to #7860. Please see the dicussion in that
issue regarding the test using hard-coded paths.

addaleax · 2016-07-27T10:39:09Z

Makefile

@@ -92,6 +92,7 @@ clean:
 	-rm -rf node_modules
 	@if [ -d deps/icu ]; then echo deleting deps/icu; rm -rf deps/icu; fi
 	-rm -f test.tap
+	$(MAKE) clean-addons


You can add these as prerequisites on the targets themselves, e.g. clean: clean-addons

No problems, I've added them as dependencies now.

addaleax · 2016-07-27T10:58:29Z

CI: https://ci.nodejs.org/job/node-test-commit/4286/

Btw, it’s generally appreciated when references to Github issues/PRs in the commit message use the full URL, so that it’s easier to e.g. disambiguate from the old (https://github.com/nodejs/node-v0.x-archive) issue tracker and the link can be opened when viewing from the terminal (and probably a few other reasons as well).

Otherwise, this LGTM if @bnoordhuis and CI are happy, thanks!

danbev · 2016-07-27T11:11:28Z

Btw, it’s generally appreciated when references to Github issues/PRs in the commit message use the full URL, so that it’s easier to e.g. disambiguate from the old (https://github.com/nodejs/node-v0.x-archive) issue tracker and the link can be opened when viewing from the terminal (and probably a few other reasons as well).

Ah good to know, thanks. I'll rebase later and squash the commits, and also update the original commit message with the full URL once CI has completed (and allow time for anyone else to comment).

jasnell · 2016-07-29T16:10:34Z

LGTM but let's definitely give @bnoordhuis a chance to review also.

bnoordhuis · 2016-07-30T08:46:10Z

The rule name is slightly misleading because it doesn't actually clean. Would this work?

diff --git a/Makefile b/Makefile
index c827968..8a232c0 100644
--- a/Makefile
+++ b/Makefile
@@ -150,7 +150,8 @@ 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: deps/npm/node_modules/node-gyp/package.json \
+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 \

Currently the build-addons target is called as part of the test target, and it has test/addons/.buildstamp as a dependency. When running ./configure --debug and later switching/ updating branches you can be in a situation where the config.gypi files in the addons build directories are using an incorrect value for default_configuration. Currently this can happen and you will get test errors as there are a few test that depend on the value of default_configuration to be Release. Ben Noordhuis provided the solution for this by adding a dependency to config.gypi, which is generated when running ./configure, and will correct this situation. This commit is related to nodejs#7860. Please see the dicussion in that issue regarding the test using hard-coded paths.

danbev · 2016-07-30T10:16:59Z

The rule name is slightly misleading because it doesn't actually clean. Would this work?

That works and is a much better solution!

I'll rebase and remove the previous commits, and please feel free to change the author of this as you did all the work :) Thanks

bnoordhuis · 2016-07-30T20:10:53Z

LGTM. I don't mind the authorship; if you go through the motions of filing a PR, you are welcome to the commit. :-)

jasnell · 2016-08-01T17:55:36Z

Updated the name of the PR to match the commit. LGTM!

Currently the build-addons target is called as part of the test target, and it has test/addons/.buildstamp as a dependency. When running ./configure --debug and later switching/ updating branches you can be in a situation where the config.gypi files in the addons build directories are using an incorrect value for default_configuration. Currently this can happen and you will get test errors as there are a few test that depend on the value of default_configuration to be Release. Ben Noordhuis provided the solution for this by adding a dependency to config.gypi, which is generated when running ./configure, and will correct this situation. This commit is related to #7860. Please see the dicussion in that issue regarding the test using hard-coded paths. PR-URL: #7893 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

jasnell · 2016-08-01T18:00:47Z

Landed in eeaff74! thank you!

Currently the build-addons target is called as part of the test target, and it has test/addons/.buildstamp as a dependency. When running ./configure --debug and later switching/ updating branches you can be in a situation where the config.gypi files in the addons build directories are using an incorrect value for default_configuration. Currently this can happen and you will get test errors as there are a few test that depend on the value of default_configuration to be Release. Ben Noordhuis provided the solution for this by adding a dependency to config.gypi, which is generated when running ./configure, and will correct this situation. This commit is related to #7860. Please see the dicussion in that issue regarding the test using hard-coded paths. PR-URL: #7893 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2016-09-30T17:29:24Z

do people think this should be backported?

bnoordhuis · 2016-09-30T17:31:35Z

Yes.

MylesBorins · 2016-09-30T17:35:54Z

it is going to need a manual backport, @bnoordhuis or @danbev would you be willing to help with that?

danbev · 2016-09-30T18:05:21Z

it is going to need a manual backport, @bnoordhuis or @danbev would you be willing to help with that?

I'd be happy to help. I don't know how much time I'll have this weekend though, but should be able to take a look on Monday-Tuesday next week if that is soon enough?

bnoordhuis · 2016-09-30T18:05:53Z

That's fine, there's no rush.

danbev · 2016-10-03T06:06:24Z

@thealphanerd Could you point me which branch this should be applied?
Would this simply be a matter of creating a pull request against that branch? Thanks

MylesBorins · 2016-10-03T06:10:34Z

@danbev v4.x-staging. Indeed just make a PR against it 👍🏽

danbev · 2016-10-03T10:49:06Z

@thealphanerd Thanks for your help! I've created this PR for this now.

Currently the build-addons target is called as part of the test target, and it has test/addons/.buildstamp as a dependency. When running ./configure --debug and later switching/ updating branches you can be in a situation where the config.gypi files in the addons build directories are using an incorrect value for default_configuration. Currently this can happen and you will get test errors as there are a few test that depend on the value of default_configuration to be Release. Ben Noordhuis provided the solution for this by adding a dependency to config.gypi, which is generated when running ./configure, and will correct this situation. This commit is related to nodejs#7860. Please see the dicussion in that issue regarding the test using hard-coded paths. Ref: nodejs#7893

Currently the build-addons target is called as part of the test target, and it has test/addons/.buildstamp as a dependency. When running ./configure --debug and later switching/ updating branches you can be in a situation where the config.gypi files in the addons build directories are using an incorrect value for default_configuration. Currently this can happen and you will get test errors as there are a few test that depend on the value of default_configuration to be Release. Ben Noordhuis provided the solution for this by adding a dependency to config.gypi, which is generated when running ./configure, and will correct this situation. This commit is related to #7860. Please see the dicussion in that issue regarding the test using hard-coded paths. Ref: #8905 PR-URL: #7893 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 27, 2016

addaleax reviewed Jul 27, 2016
View reviewed changes

danbev force-pushed the clean-test-addons branch from 80416d0 to 95309d8 Compare July 30, 2016 10:17

jasnell changed the title ~~build: adding a clean-addons target to Makefile~~ build: adding config.gypi dep to addons/.buildstamp Aug 1, 2016

jasnell closed this Aug 1, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

danbev deleted the clean-test-addons branch September 7, 2016 08:30

MylesBorins added the lts-watch-v4.x label Sep 30, 2016

danbev mentioned this pull request Oct 3, 2016

build: add config.gypi to test/addons/.buildstamp #8905

Closed

2 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Oct 10, 2016

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: adding config.gypi dep to addons/.buildstamp #7893

build: adding config.gypi dep to addons/.buildstamp #7893

danbev commented Jul 27, 2016

addaleax Jul 27, 2016

danbev Jul 27, 2016

addaleax commented Jul 27, 2016 •

edited

Loading

danbev commented Jul 27, 2016

jasnell commented Jul 29, 2016

bnoordhuis commented Jul 30, 2016

danbev commented Jul 30, 2016

bnoordhuis commented Jul 30, 2016

jasnell commented Aug 1, 2016

jasnell commented Aug 1, 2016

MylesBorins commented Sep 30, 2016

bnoordhuis commented Sep 30, 2016

MylesBorins commented Sep 30, 2016

danbev commented Sep 30, 2016

bnoordhuis commented Sep 30, 2016

danbev commented Oct 3, 2016

MylesBorins commented Oct 3, 2016

danbev commented Oct 3, 2016

build: adding config.gypi dep to addons/.buildstamp #7893

build: adding config.gypi dep to addons/.buildstamp #7893

Conversation

danbev commented Jul 27, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax Jul 27, 2016

Choose a reason for hiding this comment

danbev Jul 27, 2016

Choose a reason for hiding this comment

addaleax commented Jul 27, 2016 • edited Loading

danbev commented Jul 27, 2016

jasnell commented Jul 29, 2016

bnoordhuis commented Jul 30, 2016

danbev commented Jul 30, 2016

bnoordhuis commented Jul 30, 2016

jasnell commented Aug 1, 2016

jasnell commented Aug 1, 2016

MylesBorins commented Sep 30, 2016

bnoordhuis commented Sep 30, 2016

MylesBorins commented Sep 30, 2016

danbev commented Sep 30, 2016

bnoordhuis commented Sep 30, 2016

danbev commented Oct 3, 2016

MylesBorins commented Oct 3, 2016

danbev commented Oct 3, 2016

addaleax commented Jul 27, 2016 •

edited

Loading