Skip to content
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.

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

build: fix cctest compilation #16887

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 8, 2017

Currently the cctest target compiles sources files even though they are
compiled for the node target. This is my fault as when I worked on the
task of getting the cctest to use the object files from the node target
I missed a few sources that were being included from node.gypi. This
also effects the build time as these sources are compiled twice.

This commit moves the conditions in question into the node target in
node.gyp. With this commit there should be no object files in
out/Release/obj.target/cctest/src/ (the path will vary depending on the
operating system being used).

cc @nodejs/build

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

build, doc

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 8, 2017
@danbev
Copy link
Contributor Author

danbev commented Nov 8, 2017

@danbev
Copy link
Contributor Author

danbev commented Nov 8, 2017

@danbev
Copy link
Contributor Author

danbev commented Nov 8, 2017

Hopefully fixed windows: https://ci.nodejs.org/job/node-test-pull-request/11315/

@danbev
Copy link
Contributor Author

danbev commented Nov 9, 2017

@danbev danbev force-pushed the fix_cctest_sources_compilation branch from 136a0d1 to 34e96af Compare November 9, 2017 10:11
@@ -325,6 +325,11 @@ Next add the test to the `sources` in the `cctest` target in node.gyp:
...
],
```
Note that the only sources that should be included in the cctest are actual
Copy link
Member

Choose a reason for hiding this comment

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

"cctest target"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that should be cctest target, I'll update this.

node.gyp Outdated
# For tests
'./deps/openssl/openssl.gyp:openssl-cli',
],
# Do not let unused OpenSSL symbols to slip away
Copy link
Member

Choose a reason for hiding this comment

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

Since you're here: s/to slip/slip/ - although this comment could be removed since it's superfluous: it just says what the lines below do, not why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove the comment then if that is alright. Thanks

@danbev danbev force-pushed the fix_cctest_sources_compilation branch from b48a9e2 to a3e4332 Compare November 10, 2017 08:02
@danbev
Copy link
Contributor Author

danbev commented Nov 10, 2017

@danbev danbev force-pushed the fix_cctest_sources_compilation branch from a3e4332 to 2e9391a Compare November 13, 2017 06:40
Currently the cctest target compiles sources files even though they are
compiled for the node target. This is my fault as when I worked on the
task of getting the cctest to use the object files from the node target
I missed a few sources that were being included from node.gypi. This
also effects the build time as these sources are compiled twice.

This commit moves the conditions in question into the node target in
node.gyp. With this commit there should be no object files in
out/Release/obj.target/cctest/src/ (the path will vary depending on the
operating system being used).
@danbev
Copy link
Contributor Author

danbev commented Nov 13, 2017

danbev added a commit that referenced this pull request Nov 13, 2017
Currently the cctest target compiles sources files even though they are
compiled for the node target. This is my fault as when I worked on the
task of getting the cctest to use the object files from the node target
I missed a few sources that were being included from node.gypi. This
also effects the build time as these sources are compiled twice.

This commit moves the conditions in question into the node target in
node.gyp. With this commit there should be no object files in
out/Release/obj.target/cctest/src/ (the path will vary depending on the
operating system being used).

PR-URL:#16887
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@danbev
Copy link
Contributor Author

danbev commented Nov 13, 2017

Landed in f5809d8

@danbev danbev closed this Nov 13, 2017
@danbev danbev deleted the fix_cctest_sources_compilation branch November 13, 2017 07:52
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
Currently the cctest target compiles sources files even though they are
compiled for the node target. This is my fault as when I worked on the
task of getting the cctest to use the object files from the node target
I missed a few sources that were being included from node.gypi. This
also effects the build time as these sources are compiled twice.

This commit moves the conditions in question into the node target in
node.gyp. With this commit there should be no object files in
out/Release/obj.target/cctest/src/ (the path will vary depending on the
operating system being used).

PR-URL:#16887
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging or v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

danbev added a commit to danbev/node that referenced this pull request Nov 21, 2017
Currently the cctest target compiles sources files even though they are
compiled for the node target. This is my fault as when I worked on the
task of getting the cctest to use the object files from the node target
I missed a few sources that were being included from node.gypi. This
also effects the build time as these sources are compiled twice.

This commit moves the conditions in question into the node target in
node.gyp. With this commit there should be no object files in
out/Release/obj.target/cctest/src/ (the path will vary depending on the
operating system being used).

Refs: nodejs#16887
PR-URL:nodejs#16887
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
Currently the cctest target compiles sources files even though they are
compiled for the node target. This is my fault as when I worked on the
task of getting the cctest to use the object files from the node target
I missed a few sources that were being included from node.gypi. This
also effects the build time as these sources are compiled twice.

This commit moves the conditions in question into the node target in
node.gyp. With this commit there should be no object files in
out/Release/obj.target/cctest/src/ (the path will vary depending on the
operating system being used).

Refs: #16887
PR-URL: #16887
Backport-PR-URL: #17174
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
Currently the cctest target compiles sources files even though they are
compiled for the node target. This is my fault as when I worked on the
task of getting the cctest to use the object files from the node target
I missed a few sources that were being included from node.gypi. This
also effects the build time as these sources are compiled twice.

This commit moves the conditions in question into the node target in
node.gyp. With this commit there should be no object files in
out/Release/obj.target/cctest/src/ (the path will vary depending on the
operating system being used).

Refs: #16887
PR-URL: #16887
Backport-PR-URL: #17174
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants