Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(27)

Issue 2149963002: Add force_dynamic_crt to build as static library but with /MD on windows (Closed)

Created:
4 years, 5 months ago by sxa
Modified:
4 years, 5 months ago
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -3 lines) Patch
M gypfiles/toolchain.gypi View 1 2 3 4 chunks +4 lines, -3 lines 0 comments Download

Messages

Total messages: 29 (12 generated)
sxa
Please review this minor change to V8's GYP files which is required for a new ...
4 years, 5 months ago (2016-07-14 16:03:11 UTC) #1
Michael Achenbach
lgtm ~ suggestions Please shorten the CL description a bit and remove phrases containing personal ...
4 years, 5 months ago (2016-07-14 18:41:18 UTC) #3
sxa
Done both of those - cheers
4 years, 5 months ago (2016-07-15 14:31:23 UTC) #5
Michael Achenbach
Gotcha: Also the subject line. Just for consistency. It's only used for emails.
4 years, 5 months ago (2016-07-15 14:41:27 UTC) #6
Michael Achenbach
lgtm
4 years, 5 months ago (2016-07-16 08:02:06 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2149963002/40001
4 years, 5 months ago (2016-07-17 15:11:38 UTC) #10
commit-bot: I haz the power
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_compile_dbg_ng/builds/222305)
4 years, 5 months ago (2016-07-17 17:29:27 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2149963002/40001
4 years, 5 months ago (2016-07-18 07:05:00 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 5 months ago (2016-07-18 07:12:00 UTC) #16
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/9cf88c1c364cf76c1e745aa63196768435e8ef5d Cr-Commit-Position: refs/heads/master@{#37814}
4 years, 5 months ago (2016-07-18 07:13:00 UTC) #18
Michael Achenbach
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2155073002/ by machenbach@chromium.org. ...
4 years, 5 months ago (2016-07-18 09:18:57 UTC) #19
Michael Achenbach
https://codereview.chromium.org/2149963002/diff/40001/gypfiles/standalone.gypi File gypfiles/standalone.gypi (right): https://codereview.chromium.org/2149963002/diff/40001/gypfiles/standalone.gypi#newcode36 gypfiles/standalone.gypi:36: 'force_dynamic_crt%': 0, This default is not seen by embedded ...
4 years, 5 months ago (2016-07-18 09:20:01 UTC) #20
sxa
Unless I'm mistaken we should be ok to just move the definition from standalone.gypi to ...
4 years, 5 months ago (2016-07-18 16:32:43 UTC) #21
Michael Achenbach
On 2016/07/18 16:32:43, sxa wrote: > Unless I'm mistaken we should be ok to just ...
4 years, 5 months ago (2016-07-18 19:37:49 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2149963002/60001
4 years, 5 months ago (2016-07-19 09:32:22 UTC) #25
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 5 months ago (2016-07-19 09:53:27 UTC) #27
commit-bot: I haz the power
4 years, 5 months ago (2016-07-19 09:55:14 UTC) #29
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/6f68f30ba595e92ab4630a9f1967ca4baa6f6536
Cr-Commit-Position: refs/heads/master@{#37856}

Powered by Google App Engine
This is Rietveld 408576698