build: don't pass DESTCPU to configure unless set #13160
Conversation
If the DESTCPU wasn't manually set, the configure platform detection should be better than the Makefile one. If it was manually set, add it to the CONFIG_FLAGS. As DESTCPU no longer needs to be manually set, just set ARCH. Fixes: nodejs#13150
|
cc/ @nodejs/build @nodejs/release @bnoordhuis , PTAL (I'd really like to make sure this doesn't break anything... ). |
|
Test build: https://nodejs.org/download/test/v8.0.0-test2017052389cfb62de6/ (access restricted build: https://ci-release.nodejs.org/job/iojs+release/1696/) We should confirm that the generated binaries are right. |
| # Use Node arch names rather than V8 ones. | ||
| ifeq ($(DESTCPU),ia32) | ||
| override DESTCPU=x86 | ||
| endif |
There was a problem hiding this comment.
ia32 is the node/v8 arch name; either the comment is wrong or the logic is.
| ARCH ?= arm | ||
| else | ||
| ifeq ($(findstring aarch64,$(UNAME_M)),aarch64) | ||
| ARCH ?= aarch64 |
| endif | ||
|
|
||
| # Handle Node and V8 arch name differences for x86 and arm64. | ||
| ifeq ($(ARCH),x86) |
There was a problem hiding this comment.
This can be:
V8_ARCH = $(ARCH) # Maybe get rid of this in favor of DESTCPU
ifeq ($(ARCH),ia32)
ARCH = x86
endifAssuming I read it right.
|
Ping @gibfahn |
|
Ping @gibfahn again |
|
Closing due to long inactivity. @gibfahn please reopen if you want to pursue this further. |
|
@refack maybe you should open a separate PR if you want to pursue this? there's a bit too much wrong here and it seems to be quite stale |
Yes I've discovered that... |
If the DESTCPU wasn't manually set, the configure platform detection should be better than the Makefile one. If it was manually set, add it to the CONFIG_FLAGS.
As DESTCPU no longer needs to be manually set, just set ARCH.
Also cleans up the logic.
Fixes: #13150
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build