build,win: re-enable ninja & multiple build optimizations by refack · Pull Request #22698 · nodejs/node · GitHub

Checklist

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

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/797/pipeline

We currently publish the pdb, e.g. https://nodejs.org/dist/v10.9.0/win-x64/node_pdb.zip so this is semver-major?

cc @nodejs/platform-windows @nodejs/diagnostics

See also

node/vcbuild.bat

Lines 372 to 378 in 4cea01a

    
           echo Creating node_pdb.7z 
        
           del node_pdb.7z > nul 2> nul 
        
           7z a -mx9 -t7z node_pdb.7z node.pdb > nul 
        
           echo Creating node_pdb.zip 
        
           del node_pdb.zip  > nul 2> nul 
        
           7z a -mx9 -tzip node_pdb.zip node.pdb > nul

which is only run on the CI for releases (not for regular PR's).

Ohh. This generates a .pdb file but only a single one during linking.

This will exhaust the memory available in some machines (including our CI). Ref 6cfedf0 and #12184 (comment) . I tested this extensively when I made this change, to find the best combination that uses a reasonable number of threads, and this was the best. CL will use one thread for each processor, but since some of our projects are quite small this helps a lot. If we did it the other way around, not limiting here and limiting in CL, V8 would keep compiling long after everything else finished.

On my 4 CPU box this does not affect compilation time.

I'll continue testing this, but meanwhile I'll remove this from this PR.

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

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.

This was needed in L482 as well

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

CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20559/ ✔️

Could you describe this a bit more? Why are these required to re-enable ninja builds?

Could you describe this a bit more? Why are these required to re-enable ninja builds?

'DebugInformationFormat': 1, # /Z7 embed info in .obj files
That's to enable clcache (https://github.com/frerich/clcache#how-clcache-works)
Merging the msvs_disabled_warnings, and adding 4244 to all targets, except node and
node_lib
removing the \lib\ prefix from /WHOLEARCHIVE:<(PRODUCT_DIR)\\lib\\:
1. It's unnecessary, WHOLEARCHIVE just needs the name of the library, not it's full path
2. when building with ninja (instead of MSBuild) the libraries are outputted to: <(PRODUCT_DIR)\\obj\\, so having a wrong full path breaks linking.

This looks mostly good. @refack just two notes about the commit messages:

build,win: enable clcache: can you reword this to describe what it actually does? Something like build,win: generate a single PDB at link time and then mention clcache in the description.
build,win: unify warning exclusion: Focusing on the removed warning makes more sense to me here, something like: build,win: remove C4244 only for deps.

https://ci.nodejs.org/job/node-test-commit-windows-fanned/20664/

@joaocgreis updated messages
@nodejs/platform-windows @nodejs/build-files PTAL

Looks great, thanks @refack!

* Fixes ninja build PR-URL: nodejs#22698 Reviewed-By: João Reis <reis@janeasystems.com>

* previusly set to generate a .pdb file for each .obj file * enables clcache PR-URL: nodejs#22698 Reviewed-By: João Reis <reis@janeasystems.com>

* also unify warning exclusion directive PR-URL: nodejs#22698 Reviewed-By: João Reis <reis@janeasystems.com>

landed in
e85007f
7a10b86
54e76fb

* Fixes ninja build PR-URL: #22698 Reviewed-By: João Reis <reis@janeasystems.com>

* previusly set to generate a .pdb file for each .obj file * enables clcache PR-URL: #22698 Reviewed-By: João Reis <reis@janeasystems.com>

* also unify warning exclusion directive PR-URL: #22698 Reviewed-By: João Reis <reis@janeasystems.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Sep 5, 2018

refack changed the title build,win: multiple build optimizations build,win: re-enable ninja & multiple build optimizations Sep 5, 2018

richardlau reviewed Sep 5, 2018

View reviewed changes

Comment thread vcbuild.bat Outdated

refack force-pushed the win-build-cleanup branch 2 times, most recently from a8638d0 to e705961 Compare September 5, 2018 00:37

richardlau reviewed Sep 5, 2018

View reviewed changes

joaocgreis reviewed Sep 5, 2018

View reviewed changes

refack force-pushed the win-build-cleanup branch 2 times, most recently from 209a88a to 84bed12 Compare September 5, 2018 13:45

refack force-pushed the win-build-cleanup branch from 84bed12 to 6c8cb91 Compare September 11, 2018 18:24

joaocgreis approved these changes Sep 11, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 11, 2018

refack added 3 commits September 11, 2018 19:58

build,win: WHOLEARCHIVE needs just lib name

e85007f

* Fixes ninja build PR-URL: nodejs#22698 Reviewed-By: João Reis <reis@janeasystems.com>

build,win: generate single PDB file per target

7a10b86

* previusly set to generate a .pdb file for each .obj file * enables clcache PR-URL: nodejs#22698 Reviewed-By: João Reis <reis@janeasystems.com>

build,win: exclude warning 4244 only for deps

54e76fb

* also unify warning exclusion directive PR-URL: nodejs#22698 Reviewed-By: João Reis <reis@janeasystems.com>

refack force-pushed the win-build-cleanup branch from 6c8cb91 to 54e76fb Compare September 11, 2018 23:59

refack merged commit 54e76fb into nodejs:master Sep 12, 2018

refack deleted the win-build-cleanup branch September 12, 2018 00:01

targos pushed a commit that referenced this pull request Sep 12, 2018

build,win: WHOLEARCHIVE needs just lib name

b3bc642

* Fixes ninja build PR-URL: #22698 Reviewed-By: João Reis <reis@janeasystems.com>

targos pushed a commit that referenced this pull request Sep 12, 2018

build,win: generate single PDB file per target

dbe27d7

* previusly set to generate a .pdb file for each .obj file * enables clcache PR-URL: #22698 Reviewed-By: João Reis <reis@janeasystems.com>

targos pushed a commit that referenced this pull request Sep 12, 2018

build,win: exclude warning 4244 only for deps

b4a79ac

* also unify warning exclusion directive PR-URL: #22698 Reviewed-By: João Reis <reis@janeasystems.com>

targos mentioned this pull request Sep 18, 2018

Release proposal: v10.11.0 #22932

Merged

	echo Creating node_pdb.7z
	del node_pdb.7z > nul 2> nul
	7z a -mx9 -t7z node_pdb.7z node.pdb > nul

	echo Creating node_pdb.zip
	del node_pdb.zip > nul 2> nul
	7z a -mx9 -tzip node_pdb.zip node.pdb > nul

build,win: re-enable ninja & multiple build optimizations by refack · Pull Request #22698 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

build,win: re-enable ninja & multiple build optimizations#22698refack merged 3 commits intonodejs:masternodejs/node:masterfrom refack:win-build-cleanuprefack/node:win-build-cleanupCopy head branch name to clipboard

Conversation

refack commented Sep 5, 2018

Checklist

Uh oh!

nodejs-github-bot commented Sep 5, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardlau Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Sep 6, 2018

Uh oh!

refack commented Sep 6, 2018

Uh oh!

joaocgreis commented Sep 8, 2018

Uh oh!

refack commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joaocgreis left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

build,win: re-enable ninja & multiple build optimizations#22698
refack merged 3 commits into
nodejs:masterfrom
refack:win-build-cleanup

richardlau Sep 5, 2018 •
edited
Loading

refack commented Sep 5, 2018 •
edited
Loading

refack commented Sep 11, 2018 •
edited
Loading