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: windows sharedlib support #7487

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@stefanmb
Member

stefanmb commented Jun 29, 2016

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

build

Description of change

Following #6994 the next step is to provide support for generating a DLL on Windows for people using Node.js as a shared library. This commit makes the necessary changes, but unfortunately it requires a floating patch to V8's gyp files. I do not believe there is any way around this issue, so I am opening the PR now to get some feedback on my approach.

Overview:

  • 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.

The last point is very important. In Windows If different libraries you link use different versions of the C++ Runtime you will get a variety of issues that are well documented here: http://siomsystems.com/mixing-visual-studio-versions/. Note that this situation can occur in multiple ways:

  • If different libraries in your program mix static and dynamic runtime libraries.
  • If different libraries in your program mix versions of Visual Studio.
  • If both situations above occur.

This also means that people using node.dll must also use the exact same Visual Studio version that Node was compiled with to build the program that links against the DLL. Further information is available here: https://www.softwariness.com/articles/visual-cpp-runtime-libraries/

The basic problem with V8 is that its gyp files are setup to always link statically against the C++ runtime unless V8 itself is being compiled as a shared library. The floating patch I provided is minimal and is inspired by qt/qtwebengine-chromium@743a641.

@stefanmb

This comment has been minimized.

Member

stefanmb commented Jun 29, 2016

@bnoordhuis I would greatly appreciate your thoughts/review.
@joshgav This is the Windows patch I promised in #6994.

@stefanmb stefanmb self-assigned this Jun 29, 2016

@mhdawson

This comment has been minimized.

Member

mhdawson commented Jun 29, 2016

Once we have some feedback, if positive we'll start the process of landing the change to deps/v8/build/toolchain.gypi to v8 master.

@mscdex mscdex added V8 Engine and removed V8 Engine labels Jun 29, 2016

@joshgav

This comment has been minimized.

Member

joshgav commented Jun 29, 2016

@joshgav

This comment has been minimized.

Member

joshgav commented Jun 29, 2016

TL;DR - modifications to Node and V8 build files to enable building Node as a DLL for Windows. Particularly useful for Electron. A change in V8 is also needed because by default it statically includes the VC runtime.

It's +34 lines mostly modifying compiler switches.

Josh Gavant
Sr. Program Manager
Microsoft | One Microsoft Way | Redmond, WA | 98052


From: Stefan Budeanu notifications@github.com
Sent: Wednesday, June 29, 2016 1:16:45 PM
To: nodejs/node
Subject: [nodejs/node] build: windows sharedlib support (DO NOT LAND YET) (#7487)

Checklist

  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines

Affected core subsystem(s)

build

Description of change

Following #6994#6994 the next step is to provide support for generating a DLL on Windows for people using Node.js as a shared library. This commit makes the necessary changes, but unfortunately it requires a floating patch to V8's gyp files. I do not believe there is any way around this issue, so I am opening the PR now to get some feedback on my approach.

Overview:

  • 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.

The last point is very important. In Windows If different libraries you link use different versions of the C++ Runtime you will get a variety of issues that are well documented here: http://siomsystems.com/mixing-visual-studio-versions/https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fsiomsystems.com%2fmixing-visual-studio-versions%2f&data=01%7c01%7cjosh.gavant%40microsoft.com%7cb9ab4f56485a4420361408d3a05a5075%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=LKLU0ztyY2C2Tozbvqcb1tOZs77OXz8ez3GymjQxQcA%3d. Note that this situation can occur in multiple ways:

  • If different libraries in your program mix static and dynamic runtime libraries.
  • If different libraries in your program mix versions of Visual Studio.
  • If both situations above occur.

This also means that people using node.dll must also use the exact same Visual Studio version that Node was compiled with to build the program that links against the DLL. Further information is available here: https://www.softwariness.com/articles/visual-cpp-runtime-libraries/https://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2fwww.softwariness.com%2farticles%2fvisual-cpp-runtime-libraries%2f&data=01%7c01%7cjosh.gavant%40microsoft.com%7cb9ab4f56485a4420361408d3a05a5075%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=n317zpyIVeP8HwteGbJ09fb5%2baTc6zLvAa6RYpDmtLs%3d

The basic problem with V8 is that its gyp files are setup to always link statically against the C++ runtime unless V8 itself is being compiled as a shared library. The floating patch I provided is minimal and is inspired by qt/qtwebengine-chromium@743a641qt/qtwebengine-chromium@743a641.


You can view, comment on, or merge this pull request online at:

#7487

Commit Summary

  • WIP: Windows Shared Lib

File Changes

  • M common.gypihttps://github.com//pull/7487/files#diff-0 (30)
  • M deps/v8/build/toolchain.gypihttps://github.com//pull/7487/files#diff-1 (6)
  • M node.gyphttps://github.com//pull/7487/files#diff-2 (2)
  • M vcbuild.bathttps://github.com//pull/7487/files#diff-3 (2)

Patch Links:

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/7487, or mute the threadhttps://github.com/notifications/unsubscribe/AEN4WM3nClbv4h4tcCXB4iKC_PJj7aEtks5qQtKtgaJpZM4JBhb5.

@mhdawson

This comment has been minimized.

Member

mhdawson commented Jul 5, 2016

@bnoordhuis , @orangemocha could you two take a look and comment. If this looks good we'll start the process of submitting change in deps/v8/build/toolchain.gypi to google master.

@@ -79,6 +79,7 @@ if /i "%1"=="without-intl" set i18n_arg=%1&goto arg-ok
if /i "%1"=="download-all" set download_arg="--download=all"&goto arg-ok
if /i "%1"=="ignore-flaky" set test_args=%test_args% --flaky-tests=dontcare&goto arg-ok
if /i "%1"=="enable-vtune" set enable_vtune_arg=1&goto arg-ok
if /i "%1"=="dll" set dll=1&goto arg-ok

This comment has been minimized.

@stefanmb

This comment has been minimized.

Member

stefanmb commented Jul 6, 2016

@orangemocha I'm not sure I follow. Could you give me a few more details on what needs to be unset? Thanks.

@orangemocha

This comment has been minimized.

Member

orangemocha commented Jul 6, 2016

@stefanmb , add

set dll=

after

set build_addons=

I am still reviewing the rest.

@stefanmb

This comment has been minimized.

Member

stefanmb commented Jul 6, 2016

@orangemocha Oops, thanks for catching that. Updated PR.

@orangemocha

View changes

deps/v8/build/toolchain.gypi Outdated
@@ -1104,7 +1104,7 @@
'VCCLCompilerTool': {
'Optimization': '0',
'conditions': [
['component=="shared_library"', {
['component=="shared_library" or node_shared=="true"', {

This comment has been minimized.

@orangemocha

orangemocha Jul 7, 2016

Member

Perhaps make this a bit more generic: something like force_dynamic_crt rather than node_shared and set that in node.gyp based on node_shared.

This comment has been minimized.

@stefanmb

stefanmb Jul 7, 2016

Member

Agreed, as it is I don't know if this change to the V8 gyp files can be accepted at all (as a floating patch) since the policy is to not patch upstream dependencies. If we have to submit the changes upstream to V8 first then force_dynamic_crt makes much more sense. Thanks.

This comment has been minimized.

@mhdawson

mhdawson Jul 7, 2016

Member

We will submit to V8, do we just need to change node_shared to force_dynamic_crt as part of doing that ?

This comment has been minimized.

@stefanmb

stefanmb Jul 7, 2016

Member

@mhdawson I would say yes, it's a 2 line change for V8 to toolchain.gypi. Let me know when that's ready and I will remove the warning from the title of this PR and push it once enough LGTMs are gathered.

@orangemocha

This comment has been minimized.

Member

orangemocha commented Jul 7, 2016

One small suggestion, otherwise I don't see any problems with this change.

@mhdawson

This comment has been minimized.

Member

mhdawson commented Jul 11, 2016

@sxa555 is working on the patch for submission to v8 master

@stefanmb

This comment has been minimized.

Member

stefanmb commented Jul 11, 2016

@mhdawson Let me know when that's done and I will edit this patch. I can land it once sufficient LGTMs are reached.

@sxa555

This comment has been minimized.

Contributor

sxa555 commented Jul 15, 2016

@stefanmb https://codereview.chromium.org/2149963002 is the submission and it's not looking like there are any conceptual objections but they wanted force_dynamic_crt to be a 0/1 value rather than a true/false string to be consistent with the other V8 options so I've made that change that. I've verified that that works with node with this added to configure:

if b(options.shared) == 1:
o['variables']['force_dynamic_crt'] = 1
else:
o['variables']['force_dynamic_crt]' = 0

kisg pushed a commit to paul99/v8mips that referenced this pull request Jul 18, 2016

[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/node#7487

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

Review-Url: https://codereview.chromium.org/2149963002
Cr-Commit-Position: refs/heads/master@{#37814}

kisg pushed a commit to paul99/v8mips that referenced this pull request Jul 18, 2016

Revert of Add force_dynamic_crt to build as static library but with /…
…MD on windows (patchset v8#3 id:40001 of https://codereview.chromium.org/2149963002/ )

Reason for revert:
Fails gyp build with chromium:
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/37051

Blocks roll:
https://codereview.chromium.org/2157903002/

Please add the trybot ios-simulator on reland.

Original issue's description:
> [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/node#7487
>
> BUG=
> R=machenbach@chromium.org, michael_dawson@ca.ibm.com
>
> Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d
> Cr-Commit-Position: refs/heads/master@{#37814}

TBR=michael_dawson@ca.ibm.com,franzih@chromium.org,sxa@uk.ibm.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Review-Url: https://codereview.chromium.org/2155073002
Cr-Commit-Position: refs/heads/master@{#37822}

kisg pushed a commit to paul99/v8mips that referenced this pull request Jul 19, 2016

[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/node#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}
@sxa555

This comment has been minimized.

Contributor

sxa555 commented Jul 19, 2016

Now landed in V8 master :-)

@stefanmb stefanmb changed the title from build: windows sharedlib support (DO NOT LAND YET) to build: windows sharedlib support Jul 19, 2016

@stefanmb stefanmb force-pushed the stefanmb:winsharedlib branch to 406d774 Jul 19, 2016

sxa555 added a commit to sxa555/node-1 that referenced this pull request Aug 21, 2016

build: windows sharedlib support
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 <alexis@janeasystems.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell added a commit that referenced this pull request Aug 24, 2016

build: windows sharedlib support
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 <alexis@janeasystems.com>
  Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #8084
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016

deps: cherry-pick 6f68f30 from v8 upstream
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@{#37814}
Cr-Commit-Position: refs/heads/master@{#37856}

PR-URL: nodejs#7802
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 16, 2016

build: configure --shared
Add configure flag for building a shared library that can be
embedded in other applications (like Electron). Add flags
--without-bundled-v8 and --without-v8-platform to control V8
dependencies used.

PR-URL: nodejs#6994
Ref: nodejs#9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Reference:  nodejs#7487

sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 16, 2016

build: cherry pick V8 change 6f68f30 for windows DLL support
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.

PR-URL: nodejs#8084
Reference:  nodejs#7487
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 16, 2016

build: configure --shared
Add configure flag for building a shared library that can be
embedded in other applications (like Electron). Add flags
--without-bundled-v8 and --without-v8-platform to control V8
dependencies used.

PR-URL: nodejs#6994
Ref: nodejs#9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Reference:  nodejs#7487

sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 16, 2016

build: windows sharedlib support
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 <alexis@janeasystems.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit to MylesBorins/node that referenced this pull request Nov 17, 2016

deps: cherry-pick 6f68f30 from v8 upstream
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@{#37814}
Cr-Commit-Position: refs/heads/master@{#37856}

Ref: nodejs#7802
Ref: nodejs#8084
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Nov 17, 2016

deps: cherry-pick 6f68f30 from v8 upstream
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 added a commit that referenced this pull request Nov 17, 2016

deps: cherry-pick 6f68f30 from v8 upstream
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>

@sxa555 sxa555 referenced this pull request Nov 18, 2016

Closed

Make --shared on AIX work (shared library support) #9675

2 of 2 tasks complete

sxa555 added a commit to sxa555/node-1 that referenced this pull request Nov 18, 2016

build: configure --shared
Add configure flag for building a shared library that can be
embedded in other applications (like Electron). Add flags
--without-bundled-v8 and --without-v8-platform to control V8
dependencies used.

PR-URL: nodejs#6994
Ref: nodejs#9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Reference:  nodejs#7487

MylesBorins added a commit that referenced this pull request Nov 18, 2016

build: configure --shared
Add configure flag for building a shared library that can be
embedded in other applications (like Electron). Add flags
--without-bundled-v8 and --without-v8-platform to control V8
dependencies used.

PR-URL: #6994
Ref: #7487
Ref: #9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Nov 18, 2016

build: windows sharedlib support
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 <alexis@janeasystems.com>
  Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: #9385
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment