-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: Remove building against a shared V8 #1331
Conversation
+1 |
Jenkins run here: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/433/ |
], | ||
|
||
'include_dirs': [ | ||
'src', | ||
'tools/msvs/genfiles', | ||
'deps/uv/src/ares', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated stylistic change.
This action is to encourage packagers to not build against a shared V8 library since even minor bumps of V8 can create issues. For people that know what they're doing, revert this commit.
..also cater for the v8 platform changes
c6429ca
to
f534a48
Compare
@bnoordhuis this commit re-adds v8 as a dependency to cctest (as well as the main target). Also, add the v8 platform stuff. It's not needed for cctest at the moment, but might as well add it? |
Agreed and LGTM assuming the CI is happy: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/449/ |
Looks happy to me. |
This might make the EPEL package harder to maintain down the line, since they strip out all bundled deps; I am not sure if we care about this, though. |
@chrisdickinson yeah. I was extra careful so this patch is easily reverted moving forward. If upstream distributions really know what they're doing (which is hard with v8), they should just revert this commit and proceed as usual. They will also run into linking issues with v8_platform. I guess the "nice" thing here would be to create a patch that sorts this out, but at the same time that'd probably be higher quality coming from packagers. |
This action is to encourage packagers to not build against a shared V8 library since even minor bumps of V8 can create issues. PR-URL: #1331 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Landed in d726a17. Thanks for the review/comments. |
I am assuming this is semver-minor... @bnoordhuis would you consider it so? |
I don't, for the reasons I outlined here. But if other people feel it's semver-minor, that's fine. |
This action is to encourage packagers to not build against a shared V8 library since even minor bumps of V8 can create issues. For people that know what they're doing -- revert this commit.
/R=@bnoordhuis, ?