build: add config.gypi to test/addons/.buildstamp #8905
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
build
2 tasks
richardlau
reviewed
Oct 3, 2016
| @@ -111,7 +111,7 @@ v8: | |||
| tools/make-v8.sh v8 | |||
| $(MAKE) -C deps/v8 $(V8_ARCH).$(BUILDTYPE_LOWER) $(V8_BUILD_OPTIONS) | |||
|
|
|||
| test: | cctest # Depends on 'all'. | |||
| test: | build-addons cctest # Depends on 'all'. | |||
There was a problem hiding this comment.
The test target doesn't actually test addons, so the addons don't need to be built?
There was a problem hiding this comment.
Ah sorry about that. I was comparing this with the Makefile in master and incorrectly added this to the test target. It looks like the correct way to run the addons test is using make test-addons. I'll remove build-addons from the test target and check that this works with the test-addons target. Thanks!
bnoordhuis
approved these changes
Oct 4, 2016
|
@bnoordhuis I don't believe we backported it, woud you like to? |
|
@danbev can you squash these commits and add the original meta data please |
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
danbev
force-pushed
the
add-config.gypi-to-addons
branch
from
9b551f4 to
a531581
Compare
I've updated this PR now and used the original meta data with an additional |
MylesBorins
pushed a commit
that referenced
this pull request
Oct 7, 2016
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 <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Ah got it now, will keep that in mind for next time. Thanks! |
MylesBorins
pushed a commit
that referenced
this pull request
Oct 10, 2016
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 <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg
pushed a commit
that referenced
this pull request
Oct 18, 2016
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 <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins
pushed a commit
that referenced
this pull request
Oct 26, 2016
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 <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
build
Description of change
This motivation for this commit is to enable a reconfiguration of the
build type (Debug/Release) to be reflected by the addons tests.
Currently, when using configure --debug, and then running the tests, the
addons build configurations (build/config.gypi) will not be updated.
Adding config.gypi to the test/addons/.buildstamp target's
prerequisites will cause the addons configurations to be updated when
config.gypi has been updated.
Also, build-addons has been added as a prerequisite to the test target
to have the addons rebuilt if the build type configuration has changed.
This is a backport of #7893.