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

Fix V8 patch #9610

Merged
merged 2 commits into from
Nov 17, 2016
Merged

Fix V8 patch #9610

merged 2 commits into from
Nov 17, 2016

Conversation

MylesBorins
Copy link
Contributor

This is a particularly weird fix and I ask that @nodejs/lts @nodejs/ctc @nodejs/v8 all take a look and make sure that I'm not off here.

While auditing a backport we were digging into a V8 change that apparantly landed in v6.x during the upgrade to V8 5.1 in cd77ca3, a backport of ofrobots@33c9578

Unfortunately that commit didn't change anything other than the patch number of V8 as the to change had already landed in v6.x as 92ecbc4

This wouldn't be an issue, except for the fact that neither of these commits backported the exact changeset from the V8 tracker, specifically the changes to deps/v8/build/standalone.gypi . The change is instead found inside of toolchain.gypi with no explanation as to why this was done.

The original backport ofrobots@33c9578 also references the wrong commit sha from V8, but that is pretty minor compared to missing part of the change set.

Phew.

So this PR attempts to rectify everything, as long as there is anything that actually needs to be rectified. To be honest I am a bit confused by the subtle difference. If I am mistaken and the changeset that we currently have is what we want I will close this, but we should try to avoid landing V8 changes without the proper meta data going forward so we can avoid confusion like this.

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Nov 15, 2016
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. It's odd the patch in v6.x deviates from the original but it's also fairly harmless.

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

I still don't understand the full sequence of events, but this is mostly harmless. On V8 master the force_dynamic_crt variable initialization is in toolchain.gypi. LGTM, no strong feelings either way.

@MylesBorins
Copy link
Contributor Author

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM . given that this improves consistently across releases.

Myles Borins and others added 2 commits November 17, 2016 14:49
Original Commit Message:
"build: cherry pick V8 change for windows DLL support"

This reverts commit 92ecbc4.

The original commit did not include the entire changeset
Original commit message:

[build] Add force_dynamic_crt option to build a static library with /…
…MD on windows

Adds option to build a V8 library statically, but with the options on
windows that allows it to be subsequently included in another DLL. On
Windows this is required for it to correclty link against the correct
C++ runtime. Require for our Node.js shared library build.

Reference:  nodejs#7487

BUG=
R=machenbach@chromium.org, michael_dawson@ca.ibm.com

Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d
Review-Url: https://codereview.chromium.org/2149963002
Cr-Original-Commit-Position: refs/heads/master@{nodejs#37814}
Cr-Commit-Position: refs/heads/master@{nodejs#37856}

Ref: nodejs#7802
Ref: nodejs#8084
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins merged commit fb635f6 into nodejs:v6.x-staging Nov 17, 2016
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
Original Commit Message:
"build: cherry pick V8 change for windows DLL support"

This reverts commit 92ecbc4.

The original commit did not include the entire changeset

PR-URL: #9610
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
Original commit message:

[build] Add force_dynamic_crt option to build a static library with /…
…MD on windows

Adds option to build a V8 library statically, but with the options on
windows that allows it to be subsequently included in another DLL. On
Windows this is required for it to correclty link against the correct
C++ runtime. Require for our Node.js shared library build.

Reference:  #7487

BUG=
R=machenbach@chromium.org, michael_dawson@ca.ibm.com

Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d
Review-Url: https://codereview.chromium.org/2149963002
Cr-Original-Commit-Position: refs/heads/master@{#37814}
Cr-Commit-Position: refs/heads/master@{#37856}

Ref: #7802
Ref: #8084
PR-URL: #9610
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins
Copy link
Contributor Author

landed in a8840bb...96e8e86

@MylesBorins
Copy link
Contributor Author

UGHHHH

this is failing on v4.x with the error

gyp: name 'force_dynamic_crt' is not defined while evaluating condition 'component=="shared_library" or force_dynamic_crt==1' in /Users/thealphanerd/code/node/v4.x/deps/v8/tools/gyp/v8.gyp while loading dependencies of /Users/thealphanerd/code/node/v4.x/node.gyp while trying to load /Users/thealphanerd/code/node/v4.x/node.gyp
Error running GYP

Likely need another patch to get this to work. Will open backport specific for v4

@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
@MylesBorins MylesBorins deleted the fix-v8-patch branch November 14, 2017 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants