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.

Sign up for GitHub

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

Jump to bottom

deps: cherry-pick 6f68f30 from v8 upstream #7802

Closed
wants to merge 1 commit into from
Closed

deps: cherry-pick 6f68f30 from v8 upstream #7802

wants to merge 1 commit into from

Conversation

stefanmb
Copy link
Contributor

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

deps

Description of change

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=[email protected], [email protected]

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}

@stefanmb stefanmb added the v8 engine Issues and PRs related to the V8 dependency. label Jul 20, 2016
@stefanmb stefanmb self-assigned this Jul 20, 2016
@stefanmb stefanmb changed the title : cherry-pick 6f68f30 from v8 upstream deps: cherry-pick 6f68f30 from v8 upstream Jul 20, 2016
@stefanmb
Copy link
Contributor Author

Required to complete work in #7487
CI: https://ci.nodejs.org/job/node-test-pull-request/3345/

@stefanmb
deps: cherry-pick 6f68f30 from v8 upstream
e93720d 
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=
[email protected], [email protected]

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}
@stefanmb stefanmb mentioned this pull request Jul 20, 2016
Closed
2 tasks
@bnoordhuis
Copy link
Member

LGTM. V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/222/

@bnoordhuis
Copy link
Member

Node CI again too, previous one failed massively due to network errors: https://ci.nodejs.org/job/node-test-pull-request/3347/

@mhdawson
Copy link
Member

mhdawson commented Aug 4, 2016

LGTM, will land today.

mhdawson pushed a commit that referenced this pull request Aug 4, 2016
@stefanmb @mhdawson
deps: cherry-pick 6f68f30 from v8 upstream
cc18937 
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=
[email protected], [email protected]

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}

PR-URL: #7802
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

mhdawson commented Aug 4, 2016

Landed as cc18937

@mhdawson mhdawson closed this Aug 4, 2016
stefanmb added a commit that referenced this pull request Aug 5, 2016
@stefanmb
build: windows sharedlib support
88e862b 
Added "dll" option to vcbuild.bat
Insure that Unix SO name is not used on Windows (i.e. produce a .dll file)
Insure that Node and its V8 dependency link against the Visual C++ Runtime
dynamically.
Requires backported V8 patch, see PR 7802.

Ref: #7802

PR-URL: #7487
Reviewed-By: Alexis Campailla <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
Closed
@cjihrig
Copy link
Contributor

cjihrig commented Aug 10, 2016

@stefanmb @mhdawson is this supposed to land in v6? My guess is no, but I wanted to double check.

cc: @nodejs/v8

@stefanmb
Copy link
Contributor Author

@cjihrig I would say no, since we're not backporting the dependent PR: #7487.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 10, 2016

OK, thanks. Adding the "don't land on" labels.

@mhdawson
Copy link
Member

I'd like it to land on 6.x for our use and I believe other consumers like electron may want that as well.

@sxa555 can you backport #7487. to 6.X so that we have the pre-reqs to land this on 6.x

I think we should hold off on 4.x until we have testing etc. in place, but I do think we want in the next LTS to go out.

I'll remove the dont-land-on-v6.x, let me know if anybody disagress.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 11, 2016

I'm going to reapply the dont-land-on-v6.x label, because this doesn't apply cleanly. We can use a backport PR instead.

@ofrobots
Copy link
Contributor

I have included this commit in #8054. This won't need independent backporting if that lands on v6.x.

@sxa
Copy link
Member

sxa commented Aug 12, 2016

@cjihrig I believe the only issue with the application was that the V8 version numbers were different. The modifications themselves apply ok :-) I'm creating a separate PR that will do this along with the main WIndows changes to build as a DLL (although I can take that out if the 5.1 update lands quickly as @ofrobots suggests).

@sxa sxa mentioned this pull request Aug 12, 2016
Closed
2 tasks
sxa pushed a commit to sxa/node that referenced this pull request Aug 21, 2016
@stefanmb
build: windows sharedlib support
3c0193d 
Added "dll" option to vcbuild.bat
Insure that Unix SO name is not used on Windows (i.e. produce a .dll file)
Insure that Node and its V8 dependency link against the Visual C++ Runtime
dynamically.
Requires backported V8 patch, see PR 7802.

Ref: nodejs#7802

PR-URL: nodejs#7487
Reviewed-By: Alexis Campailla <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
jasnell pushed a commit that referenced this pull request Aug 24, 2016
@stefanmb @jasnell
build: windows sharedlib support
4e4c091 
Original Commit Message:
  Added "dll" option to vcbuild.bat
  Insure that Unix SO name is not used on Windows (i.e. produce a .dll file)
  Insure that Node and its V8 dependency link against the Visual C++ Runtime
  dynamically.
  Requires backported V8 patch, see PR 7802.

  Ref: #7802

  PR-URL: #7487
  Reviewed-By: Alexis Campailla <[email protected]>
  Reviewed-By: Michael Dawson <[email protected]>

PR-URL: #8084
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
@stefanmb @ofrobots
deps: cherry-pick 6f68f30 from v8 upstream
cd77ca3 
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=
[email protected], [email protected]

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}

PR-URL: nodejs#7802
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@evanlucas evanlucas mentioned this pull request Aug 26, 2016
Merged
@MylesBorins
Copy link
Contributor

why did this backport move the change from common.gypi to toolchain.gypi?

sxa pushed a commit to sxa/node that referenced this pull request Nov 16, 2016
build: windows sharedlib support
c5f3e4e 
Added "dll" option to vcbuild.bat
Ensure that Unix SO name is not used on Windows (i.e. produce a .dll file)
Ensure that Node and its V8 dependency link against the Visual C++ Runtime
dynamically.
Requires backported V8 patch

Ref: nodejs#7802

PR-URL: nodejs#7487
Reviewed-By: Alexis Campailla <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Nov 17, 2016
@stefanmb
deps: cherry-pick 6f68f30 from v8 upstream
fb635f6 
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=
[email protected], [email protected]

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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2016
@stefanmb
deps: cherry-pick 6f68f30 from v8 upstream
96e8e86 
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=
[email protected], [email protected]

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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
build: windows sharedlib support
0138b4d 
Original Commit Message:
  Added "dll" option to vcbuild.bat
  Insure that Unix SO name is not used on Windows (i.e. produce a .dll file)
  Insure that Node and its V8 dependency link against the Visual C++ Runtime
  dynamically.
  Requires backported V8 patch, see PR 7802.

  Ref: #7802

  PR-URL: #7487
  Reviewed-By: Alexis Campailla <[email protected]>
  Reviewed-By: Michael Dawson <[email protected]>

PR-URL: #9385
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@stefanmb stefanmb

Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@stefanmb @bnoordhuis @mhdawson @cjihrig @ofrobots @sxa @MylesBorins