build: build a x64 build by default on Windows#11537
build: build a x64 build by default on Windows#11537seishun merged 1 commit intonodejs:masterfrom seishun:x64-default
Conversation
nodejs-github-bot
added
the
build
gibfahn
left a comment
gibfahn
left a comment
There was a problem hiding this comment.
LGTM (and this seems like a sensible suggestion).
| set config=Release | ||
| set target=Build | ||
| set target_arch=x86 | ||
| set target_arch=x64 |
There was a problem hiding this comment.
Would this be a semver-major change? I'm not really sure how we treat breaking tooling changes.
There was a problem hiding this comment.
#10156 was semver-major, so I guess this one is too. I'd be fine with that though since vcbuild is mostly used to build master anyway.
There was a problem hiding this comment.
Is it possible to detect if the OS is 64-bit like the Makefile does on the Unix platforms?
There was a problem hiding this comment.
The configure script checks some environment variables on Windows: https://github.com/nodejs/node/blob/master/configure#L735-L748
There was a problem hiding this comment.
I'm not sure it's necessary to detect whether the OS is 64-bit, defaulting to 64-bit seems reasonable and straightforward.
Detecting the ARCH from environment variables seems flaky, and what you really want is to be sure that you're going to get 64/32 bit reliably. I think anyone building 32-bit is probably aware that they'll have to set something.
cc/ @nodejs/platform-windows
There was a problem hiding this comment.
I agree, it's better to have the same behavior independent of the platform.
There was a problem hiding this comment.
Will it be obvious why the build fails if run on 32-bit Windows? ¯_(ツ)_/¯
In any case #11500 points out that specifying the architecture is currently undocumented regardless of whatever the default is.
gibfahn
added
the
semver-major
5 participants
Checklist
Affected core subsystem(s)
build
I'm quite sure most people are interested in x64 builds in 2017, and defaulting to x86 causes confusion, see for example #11500.