Skip to content

build: add support for 64-bit AIX#1807

Closed
richardlau wants to merge 1 commit intolibuv:v1.xfrom
richardlau:64bitaix
Closed

build: add support for 64-bit AIX#1807
richardlau wants to merge 1 commit intolibuv:v1.xfrom
richardlau:64bitaix

Conversation

@richardlau
Copy link
Copy Markdown
Member

@richardlau richardlau commented Apr 19, 2018

Support building 64-bit binaries on AIX via gyp. At the moment there's no automatic detection as platform.machine()/uname -m on AIX is a bit useless (e.g., on an AIX system I have access to uname -m outputs 00F460A94C00) so you need to pass -Dtarget_arch=ppc64 to gyp_uv.py to activate.

Note however that #1795 actually fails to compile on 64-bit AIX due to incorrect assumptions about the size of uv_sem_t, see nodejs/node#20129 (comment).

@richardlau
build: add support for 64-bit AIX
3ab2b1a 
Pass `-Dtarget_arch=ppc64` to `gyp_uv.py` to activate.
@richardlau
Copy link
Copy Markdown
Member Author

richardlau commented Apr 19, 2018

With regards to automatic host/target arch detection, at least one place in V8 appears to assume that if you're on AIX you want ppc64: https://github.com/nodejs/node/blob/82e475dc75751df08d80bd6e25926bd25c1794f2/deps/v8/third_party/binutils/detect_v8_host_arch.py#L63-L68

This was referenced Apr 19, 2018
Closed
Closed
@richardlau
Copy link
Copy Markdown
Member Author

richardlau commented Apr 19, 2018

I assume I also need to make an equivalent change for autotools? Anyone able to point me at where I should be editing?

@santigimeno
Copy link
Copy Markdown
Member

santigimeno commented Apr 19, 2018

Probably around here: https://github.com/libuv/libuv/blob/v1.x/Makefile.am#L330

@mhdawson
Copy link
Copy Markdown
Contributor

mhdawson commented Apr 19, 2018

The -Dtarget_arch=ppc64 must be what is allowing the 64 bit Node.js builds.

If we have to choose either 32 or 64 bit it should be 64 bit as we have only every shipped 64 bit node.js community builds, however, is it possible for us to update the CI job to set the target arch for AIX ?

@richardlau
Copy link
Copy Markdown
Member Author

richardlau commented Apr 19, 2018

@mhdawson Node.js has it's own version of common.gypi that adds the correct flags for target_arch=ppc64: https://github.com/nodejs/node/blob/94596560c2556fae614ef237ed0ff2f749b14651/common.gypi#L371-L393

There's no such equivalent (until this PR) in the libuv version of common.gypi (so you can set -Dtarget_arch=ppc64 in the CI but it won't actually change any of the compile/link flags).

@richardlau
Copy link
Copy Markdown
Member Author

richardlau commented Apr 19, 2018

Probably around here: https://github.com/libuv/libuv/blob/v1.x/Makefile.am#L330

I don't see any equivalents for the equivalent -m32 flags for ia32:

libuv/common.gypi

Lines 146 to 149 in 1e4823c

[ 'host_arch != target_arch and target_arch=="ia32"', {
'cflags': [ '-m32' ],
'ldflags': [ '-m32' ],
}],

Do we want to hardcode -maix64 into https://github.com/libuv/libuv/blob/v1.x/Makefile.am, or is the convention to pass such flags through to make using environment variables?

@mhdawson
Copy link
Copy Markdown
Contributor

mhdawson commented Apr 19, 2018

@richardlau got it.

@santigimeno
Copy link
Copy Markdown
Member

santigimeno commented Apr 19, 2018

@richardlau from other similar changes such as this it seems that for some architectures the support is only added to gyp build system. I'm sure @bnoordhuis knows better.

@richardlau richardlau mentioned this pull request Apr 19, 2018
Closed
bnoordhuis
bnoordhuis approved these changes Apr 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. CI: https://ci.nodejs.org/job/libuv-test-commit/797/

@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Apr 20, 2018

from other similar changes such as this it seems that for some architectures the support is only added to gyp build system

Yes. If someone needs aix+autotools, they'll open an issue.

mhdawson
mhdawson approved these changes Apr 20, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

cjihrig pushed a commit that referenced this pull request Apr 22, 2018
@richardlau @cjihrig
build: add support for 64-bit AIX
4f77a23 
Pass -Dtarget_arch=ppc64 to gyp_uv.py to activate.

Refs: nodejs/node#20129
Refs: #1795
PR-URL: #1807
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig
Copy link
Copy Markdown
Contributor

cjihrig commented Apr 22, 2018

Landed in 4f77a23. Thanks!

@cjihrig cjihrig closed this Apr 22, 2018
@richardlau
Copy link
Copy Markdown
Member Author

richardlau commented Apr 25, 2018

from other similar changes such as this it seems that for some architectures the support is only added to gyp build system

Yes. If someone needs aix+autotools, they'll open an issue.

So to follow up it appears that the libuv CI is using autotools on AIX 😆 . I think I've figured out the steps you need to build 64-bit AIX binaries via autotools in nodejs/build#1245 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@santigimeno santigimeno santigimeno approved these changes

+1 more reviewer

@mhdawson mhdawson mhdawson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@richardlau @santigimeno @mhdawson @bnoordhuis @cjihrig