Skip to content
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.

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

build: fix DESTCPU detection on non-Intel platforms #6310

Closed
wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

Checklist
  • the commit message follows commit guidelines
Affected core subsystem(s)

build

Description of change

make binary attempts to auto detect DESTCPU if not set, but was
assuming being on an Intel architecture (i.e. it was checking for x64
and assuming x86 otherwise).

@richardlau
Copy link
Member Author

cc @mhdawson

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Apr 20, 2016
@mhdawson mhdawson self-assigned this Apr 20, 2016
@mhdawson
Copy link
Member

Just wondering which case this covers:

+ifeq ($(findstring powerpc,$(shell uname -p)),powerpc)
+DESTCPU ?= ppc
+else

otherwise LGTM

@richardlau
Copy link
Member Author

That case covers AIX.

@jbergstroem
Copy link
Member

@nodejs/build

@richardlau
Copy link
Member Author

Note about the implementation:
I chose to extend what was already there in the makefile. Using variants of uname I don't see a way of detecting 32-bit/64-bit on AIX.

With regards to host architecture detection there are two other places this is done in the build scripts:

  • in host_arch_cc() in configure -- configure sets the target arch to the detected host arch if --dest-cpu is not specified (in the case of make binary, configure is run with --dest-cpu=$(DESTCPU))
  • in GuessArchitecture() in tools/utils.py -- This is used by tools/test.py and for some reason only selects 32-bit architectures

@richardlau
Copy link
Member Author

@mhdawson I've updated the commit so on AIX it defaults to 64-bit as we discussed on the phone.

@mhdawson
Copy link
Member

LGTM

@mhdawson
Copy link
Member

@jbergstroem just wondering if you can take a quick look and let me know if you have any concerns with this.

@joaocgreis
Copy link
Member

I can't claim to know enough of this to stamp it, but since it uses only ?= it seems quite safe to me.

However, at least ARM64 is missing:

$ uname -m
aarch64

`make binary` attempts to auto detect DESTCPU if not set, but was
assuming being on an Intel architecture.
@richardlau
Copy link
Member Author

Thanks, rebased onto latest master and added aarch64.

@MylesBorins
Copy link
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/2614/

overall the changes to the makefile LGTM

How was ppc + arm being detected and built correctly before?

@rvagg
Copy link
Member

rvagg commented May 13, 2016

sgtm

might be time to pull out all of this ARCH and DESTCPU stuff into a script (bash or perl) in tools

@jbergstroem
Copy link
Member

@rvagg we could use python's os stuff. Would align well with our build stack.

@rvagg
Copy link
Member

rvagg commented May 13, 2016

@jbergstroem sounds good.

To clarify though, that shouldn't hold up this PR if the IBM folks agree.

@thealphanerd we manually specify both ARCH and DESTCPU in all (?) our build machines so we don't get caught up by this, it's obviously a concern for end-users on any of these platforms, however.

@jbergstroem
Copy link
Member

@rvagg we don't manually specify it on all our machines; mainly release-related stuff -- I think we should avoid it where possible since we'd be testing the configure process as well. With that said, in cases we have to, we should obviously override (esp DESTCPU).

@jbergstroem
Copy link
Member

LGTM from me as well btw. Just haven't tried all variations of uname.

@mhdawson
Copy link
Member

Looks like we have enough LGTMs to land, will do that now

mhdawson pushed a commit that referenced this pull request May 13, 2016
`make binary` attempts to auto detect DESTCPU if not set, but was
assuming being on an Intel architecture.

PR-URL: #6310
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson
Copy link
Member

Landed as 830a726

@mhdawson mhdawson closed this May 13, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
`make binary` attempts to auto detect DESTCPU if not set, but was
assuming being on an Intel architecture.

PR-URL: #6310
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jun 1, 2016
`make binary` attempts to auto detect DESTCPU if not set, but was
assuming being on an Intel architecture.

PR-URL: #6310
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jun 23, 2016
`make binary` attempts to auto detect DESTCPU if not set, but was
assuming being on an Intel architecture.

PR-URL: #6310
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
`make binary` attempts to auto detect DESTCPU if not set, but was
assuming being on an Intel architecture.

PR-URL: #6310
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Jun 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants