Skip to content

build,win: re-enable ninja & multiple build optimizations #22698

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

Merged
refack merged 3 commits into from
Sep 12, 2018
Merged

build,win: re-enable ninja & multiple build optimizations #22698

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e85007f
build,win: WHOLEARCHIVE needs just lib name
refack Sep 4, 2018
7a10b86
build,win: generate single PDB file per target
refack Sep 4, 2018
54e76fb
build,win: exclude warning 4244 only for deps
refack Sep 4, 2018
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
Prev Previous commit
build,win: exclude warning 4244 only for deps 
* also unify warning exclusion directive

PR-URL: #22698
Reviewed-By: João Reis <reis@janeasystems.com>
  • Loading branch information
@refack
refack committed Sep 11, 2018
commit 54e76fb873027cc6089bf8d7359d188d3687909e
27 changes: 14 additions & 13 deletions common.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,18 +274,6 @@
'BufferSecurityCheck': 'true',
'ExceptionHandling': 0, # /EHsc
'SuppressStartupBanner': 'true',
# Disable warnings:
# - "C4251: class needs to have dll-interface"
# - "C4275: non-DLL-interface used as base for DLL-interface"
# Over 10k of these warnings are generated when compiling node,
# originating from v8.h. Most of them are false positives.
# See also: https://github.com/nodejs/node/pull/15570
# TODO: re-enable when Visual Studio fixes these upstream.
#
# - "C4267: conversion from 'size_t' to 'int'"
# Many any originate from our dependencies, and their sheer number
# drowns out other, more legitimate warnings.
'DisableSpecificWarnings': ['4251', '4275', '4267'],
'WarnAsError': 'false',
},
'VCLinkerTool': {
Expand Down Expand Up @@ -316,7 +304,20 @@
'SuppressStartupBanner': 'true',
},
},
'msvs_disabled_warnings': [4351, 4355, 4800],
# Disable warnings:
# - "C4251: class needs to have dll-interface"
# - "C4275: non-DLL-interface used as base for DLL-interface"
# Over 10k of these warnings are generated when compiling node,
# originating from v8.h. Most of them are false positives.
# See also: https://github.com/nodejs/node/pull/15570
# TODO: re-enable when Visual Studio fixes these upstream.
#
# - "C4267: conversion from 'size_t' to 'int'"
# Many any originate from our dependencies, and their sheer number
# drowns out other, more legitimate warnings.
# - "C4244: conversion from 'type1' to 'type2', possible loss of data"
# Ususaly safe. Disable for `dep`, enable for `src`
'msvs_disabled_warnings': [4351, 4355, 4800, 4251, 4275, 4244, 4267],
'conditions': [
['asan == 1 and OS != "mac"', {
'cflags+': [
Expand Down
10 changes: 10 additions & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,11 @@
'src',
'deps/v8/include',
],

# - "C4244: conversion from 'type1' to 'type2', possible loss of data"
# Ususaly safe. Disable for `dep`, enable for `src`
'msvs_disabled_warnings!': [4244],
Copy link
Copy Markdown
Member

@joaocgreis joaocgreis Sep 5, 2018

Choose a reason for hiding this comment

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

I didn't test yet, but this looks quite nice. Is it possible to simply set a much lower warning level for deps?

Copy link
Copy Markdown
Contributor Author

@refack refack Sep 5, 2018

Choose a reason for hiding this comment

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

I've been thinking about that, the problem is common.gypi is used by node-gyp so I didn't want to make a big change without further testing.

Copy link
Copy Markdown
Contributor Author

@refack refack Sep 5, 2018

Choose a reason for hiding this comment

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

This was needed in L482 as well

Copy link
Copy Markdown
Contributor Author

@refack refack Sep 5, 2018

Choose a reason for hiding this comment

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

I didn't test yet, but this looks quite nice.

output is filled with:

d:\code\node\src\node_internals.h(321): warning C4244: 'argument': conversion from 'const uint64_t' to 'const NativeT', possible loss of data


'conditions': [
[ 'node_intermediate_lib_type=="static_library" and '
'node_shared=="true" and OS=="aix"', {
Expand Down Expand Up @@ -472,6 +477,11 @@
'V8_DEPRECATION_WARNINGS=1',
'NODE_OPENSSL_SYSTEM_CERT_PATH="<(openssl_system_ca_path)"',
],

# - "C4244: conversion from 'type1' to 'type2', possible loss of data"
# Ususaly safe. Disable for `dep`, enable for `src`
'msvs_disabled_warnings!': [4244],

'conditions': [
[ 'node_code_cache_path!=""', {
'sources': [ '<(node_code_cache_path)' ]
Expand Down