Skip to content

build: windows sharedlib support #7487

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

Closed
stefanmb wants to merge 5 commits into from
Closed

build: windows sharedlib support #7487

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1877277
WIP: Windows Shared Lib
stefanmb Jun 29, 2016
db00e24
WIP: Review Fix
stefanmb Jul 6, 2016
406d774
WIP: Use force_dynamic_crt v8 flag
stefanmb Jul 19, 2016
d2df772
WIP: Fix option value
stefanmb Jul 19, 2016
c191e89
WIP: Remove V8 changes
stefanmb Jul 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
WIP: Windows Shared Lib
  • Loading branch information
@stefanmb
stefanmb committed Jul 19, 2016
commit 18772770f15cc0e64ba023535955fe323d444588
30 changes: 28 additions & 2 deletions common.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,24 @@
['OS == "android"', {
'cflags': [ '-fPIE' ],
'ldflags': [ '-fPIE', '-pie' ]
}],
['node_shared=="true"', {
'msvs_settings': {
'VCCLCompilerTool': {
'RuntimeLibrary': 3, # MultiThreadedDebugDLL (/MDd)
}
}
}],
['node_shared=="false"', {
'msvs_settings': {
'VCCLCompilerTool': {
'RuntimeLibrary': 1 # MultiThreadedDebug (/MTd)
}
}
}]
],
'msvs_settings': {
'VCCLCompilerTool': {
'RuntimeLibrary': 1, # static debug
'Optimization': 0, # /Od, no optimization
'MinimalRebuild': 'false',
'OmitFramePointers': 'false',
Expand Down Expand Up @@ -115,11 +128,24 @@
['OS == "android"', {
'cflags': [ '-fPIE' ],
'ldflags': [ '-fPIE', '-pie' ]
}],
['node_shared=="true"', {
'msvs_settings': {
'VCCLCompilerTool': {
'RuntimeLibrary': 2 # MultiThreadedDLL (/MD)
}
}
}],
['node_shared=="false"', {
'msvs_settings': {
'VCCLCompilerTool': {
'RuntimeLibrary': 0 # MultiThreaded (/MT)
}
}
}]
],
'msvs_settings': {
'VCCLCompilerTool': {
'RuntimeLibrary': 0, # static release
'Optimization': 3, # /Ox, full optimization
'FavorSizeOrSpeed': 1, # /Ot, favour speed over size
'InlineFunctionExpansion': 2, # /Ob2, inline anything eligible
Expand Down
6 changes: 3 additions & 3 deletions deps/v8/build/toolchain.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,7 @@
'VCCLCompilerTool': {
'Optimization': '0',
'conditions': [
['component=="shared_library"', {
['component=="shared_library" or node_shared=="true"', {

@orangemocha orangemocha Jul 7, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@stefanmb stefanmb Jul 7, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@mhdawson mhdawson Jul 7, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@stefanmb stefanmb Jul 7, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

'RuntimeLibrary': '3', # /MDd
}, {
'RuntimeLibrary': '1', # /MTd
Expand Down Expand Up @@ -1156,7 +1156,7 @@
'StringPooling': 'true',
'BasicRuntimeChecks': '0',
'conditions': [
['component=="shared_library"', {
['component=="shared_library" or node_shared=="true"', {
'RuntimeLibrary': '3', #/MDd
}, {
'RuntimeLibrary': '1', #/MTd
Expand Down Expand Up @@ -1347,7 +1347,7 @@
'FavorSizeOrSpeed': '0',
'StringPooling': 'true',
'conditions': [
['component=="shared_library"', {
['component=="shared_library" or node_shared=="true"', {
'RuntimeLibrary': '2', #/MD
}, {
'RuntimeLibrary': '0', #/MT
Expand Down
2 changes: 1 addition & 1 deletion node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@
'NODE_SHARED_MODE',
],
'conditions': [
[ 'node_module_version!=""', {
[ 'node_module_version!="" and OS!="win"', {
'product_extension': 'so.<(node_module_version)',
}]
],
Expand Down
2 changes: 2 additions & 0 deletions vcbuild.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@orangemocha orangemocha Jul 6, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to unset dll at https://github.com/nodejs/node/pull/7487/files#diff-f1d059c92a388e475daa1fbc2a88aa3aR41

echo Warning: ignoring invalid command line option `%1`.

Expand Down Expand Up @@ -108,6 +109,7 @@ if defined noperfctr set configure_flags=%configure_flags% --without-perfctr& se
if defined release_urlbase set release_urlbase_arg=--release-urlbase=%release_urlbase%
if defined download_arg set configure_flags=%configure_flags% %download_arg%
if defined enable_vtune_arg set configure_flags=%configure_flags% --enable-vtune-profiling
if defined dll set configure_flags=%configure_flags% --shared

if "%i18n_arg%"=="full-icu" set configure_flags=%configure_flags% --with-intl=full-icu
if "%i18n_arg%"=="small-icu" set configure_flags=%configure_flags% --with-intl=small-icu
Expand Down