Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2149963002:
Add force_dynamic_crt to build as static library but with /MD on windows (Closed)
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: https://github.com/nodejs/node/pull/7487
BUG=
R=machenbach@chromium.org, michael_dawson@ca.ibm.com
Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d
Committed: https://crrev.com/6f68f30ba595e92ab4630a9f1967ca4baa6f6536
Cr-Original-Commit-Position: refs/heads/master@{#37814}
Cr-Commit-Position: refs/heads/master@{#37856}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Adjust force_dynamic_crt to be a number instead of a true/false string as per review #Patch Set 3 : [build] Add force_dynamic_crt option to build a static library with /MD on windows #
Total comments: 1
Patch Set 4 : Move default definition of force_dynamic_crt from standalone.gypi to toolchain.gypi #Messages
Total messages: 29 (12 generated)
Please review this minor change to V8's GYP files which is required for a new feature being added to Node.js.
machenbach@chromium.org changed reviewers: + franzih@chromium.org
lgtm ~ suggestions Please shorten the CL description a bit and remove phrases containing personal pronouns. Rather add details which are only important for the review here to the messages. Please also strip details from the subject line + first line of CL desc. Maybe prefix the subject with e.g. [build] https://codereview.chromium.org/2149963002/diff/1/gypfiles/standalone.gypi File gypfiles/standalone.gypi (right): https://codereview.chromium.org/2149963002/diff/1/gypfiles/standalone.gypi#ne... gypfiles/standalone.gypi:36: 'force_dynamic_crt%': 'false', nit: it's convention to use 0 and 1 in gyp for switches like this.
Description was changed from ========== Add force_dynamic_crt option to build a static library with /MD on windows for embedding in a DLL in Node.js As discussed in the Node.js community issue https://github.com/nodejs/node/pull/6994 we are working to add the option to build Node.js as a shared library. We've done this for linux and are working toward that for windows in: https://github.com/nodejs/node/pull/7487 The change requires a patch to v8 and per the Node.js policy we need to land this in v8 master first. For most platforms I don't need any changes to V8 to make this work, but on Windows we need to ensure that the correct version of the Windows C++ runtime is used on the compile options to allow it to be linked correctly. V8 already has a "shared_library" component, but what we need is a set of lib files that do not constitute a full standalone shared library version of V8, but has the compiler flags (/MD* instead of /MT*) to allow it to be statically incorporated into our DLL. This review add support for an extra "force_dynamic_crt" definition that we can set that would cause V8's build to use the correct flags. We believe this will not affect any existing users of v8 but will allow the Node.js community to build a shared library on Windows. Note for reviewers: As it stands, this patch does not include anything to allow V8's own standalone build process to change force_dynamic_crt from the default value. BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com ========== to ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com ==========
Done both of those - cheers
Gotcha: Also the subject line. Just for consistency. It's only used for emails.
Description was changed from ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com ========== to ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com ==========
lgtm
The CQ bit was checked by sxa@uk.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com ========== to ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com ========== to ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Cr-Commit-Position: refs/heads/master@{#37814} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Cr-Commit-Position: refs/heads/master@{#37814}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2155073002/ by machenbach@chromium.org. The reason for reverting is: Fails gyp build with chromium: https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bu... Blocks roll: https://codereview.chromium.org/2157903002/ Please add the trybot ios-simulator on reland..
Message was sent while issue was closed.
https://codereview.chromium.org/2149963002/diff/40001/gypfiles/standalone.gypi File gypfiles/standalone.gypi (right): https://codereview.chromium.org/2149963002/diff/40001/gypfiles/standalone.gyp... gypfiles/standalone.gypi:36: 'force_dynamic_crt%': 0, This default is not seen by embedded chromium builds. Need a default in toolchain.gypi.
Message was sent while issue was closed.
Unless I'm mistaken we should be ok to just move the definition from standalone.gypi to toolchain.gypi as I believe standalone.gypi will always result in toolchain.gypi being included too. Let me know if that assumption is invalid.
Message was sent while issue was closed.
Description was changed from ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Cr-Commit-Position: refs/heads/master@{#37814} ========== to ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Cr-Commit-Position: refs/heads/master@{#37814} ==========
On 2016/07/18 16:32:43, sxa wrote: > Unless I'm mistaken we should be ok to just move the definition from > standalone.gypi to toolchain.gypi > as I believe standalone.gypi will always result in toolchain.gypi being included > too. Let me know if > that assumption is invalid. lgtm, sounds right. I added also the bot that failed on the roll. Ignore the gn version - that was a mistake.
The CQ bit was checked by sxa@uk.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Cr-Commit-Position: refs/heads/master@{#37814} ========== to ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Cr-Commit-Position: refs/heads/master@{#37814} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Cr-Commit-Position: refs/heads/master@{#37814} ========== to ========== [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: https://github.com/nodejs/node/pull/7487 BUG= R=machenbach@chromium.org, michael_dawson@ca.ibm.com Committed: https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Committed: https://crrev.com/6f68f30ba595e92ab4630a9f1967ca4baa6f6536 Cr-Original-Commit-Position: refs/heads/master@{#37814} Cr-Commit-Position: refs/heads/master@{#37856} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6f68f30ba595e92ab4630a9f1967ca4baa6f6536 Cr-Commit-Position: refs/heads/master@{#37856} |
