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

ansible: arm cross compiler updates for Node.js 18 #2912

Merged
merged 6 commits into from
Apr 8, 2022

Conversation

richardlau
Copy link
Member

Add a new RHEL 8 based container for cross compilation of armv7l
binaries for Node.js 18 using the recently added cross compiler
toolchain based on gcc 8 with glibc 2.28.

Adds a Debian 10 (Buster) container to the Pi's -- even though the Pi's
are running Debian 10 the tests are run in containers and prior to this
commit were running in Debian 9, which is on glibc 2.24.


The plan would be, at least initially, to continue testing older versions of Node.js in the stretch container but use the buster container for Node.js 18 onwards. Or do we think stretch is too old now we should move everything?

Add a new RHEL 8 based container for cross compilation of armv7l
binaries for Node.js 18 using the recently added cross compiler
toolchain based on gcc 8 with glibc 2.28.

Adds a Debian 10 (Buster) container to the Pi's -- even though the Pi's
are running Debian 10 the tests are run in containers and prior to this
commit were running in Debian 9, which is on glibc 2.24.
@richardlau
Copy link
Member Author

@rvagg I might need a bit of help with the Pi's. I just tried to Ansible test-requireio_williamkapke-debian10-arm64_pi3-3 and got the following error:

TASK [package-upgrade : upgrade installed packages] ********************************************************************************************************
fatal: [test-requireio_williamkapke-debian10-arm64_pi3-2]: FAILED! => {"changed": false, "msg": "Failed to update apt cache: W:This must be accepted explicitly before updates for this repository can be applied. See apt-secure(8) manpage for details., E:Repository 'http://mirror.aarnet.edu.au/pub/raspbian/raspbian buster InRelease' changed its 'Suite' value from 'stable' to 'oldstable', W:This must be accepted explicitly before updates for this repository can be applied. See apt-secure(8) manpage for details., E:Repository 'http://archive.raspberrypi.org/debian buster InRelease' changed its 'Suite' value from 'testing' to 'oldstable'"}

PLAY RECAP *************************************************************************************************************************************************
test-requireio_williamkapke-debian10-arm64_pi3-2 : ok=21   changed=3    unreachable=0    failed=1    skipped=6    rescued=0    ignored=0

FWIW I've tested the cross compilation bit of this PR in a copy of the arm-fanned and cross compile job: https://ci.nodejs.org/view/MyJobs/job/richardlau-node-test-commit-arm-fanned/6/

https://ci.nodejs.org/job/node-test-binary-arm-12+/15125/, is what alerted me to the tests being run in a Debian 9 container despite the Pi's running Debian 10 e.g. https://ci.nodejs.org/job/node-test-binary-arm-12+/15125/RUN_SUBSET=addons,label=pi3-docker/console

21:08:47 + docker exec node-ci-stretch /bin/sh -c 'cd /home/iojs/build/workspace/node-test-binary-arm && . /home/iojs/build/workspace/node-test-binary-arm/node-ci-exec'
21:08:54 ./node: /lib/arm-linux-gnueabihf/libm.so.6: version `GLIBC_2.27' not found (required by ./node)
21:08:54 ./node: /lib/arm-linux-gnueabihf/libc.so.6: version `GLIBC_2.25' not found (required by ./node)
21:08:54 ./node: /lib/arm-linux-gnueabihf/libc.so.6: version `GLIBC_2.28' not found (required by ./node)

(but hey it shows we did link against glibc 2.28 🙂.)

export CC_host="ccache gcc-${gcc_host_version} -m32"
export CXX_host="ccache g++-${gcc_host_version} -m32"
if [ "$host_os" = "rhel8" ]; then
export CC_host="ccache gcc -m32"
Copy link
Member

Choose a reason for hiding this comment

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

you're sure this is going to be OK? do we not have the option to version this? IIRC we added the host version because it was picking the wrong version because it happened to be the default on the machine, so we went with the version specifier. But maybe that's not an option with rhel in this setup?

Copy link
Member Author

Choose a reason for hiding this comment

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

The RHEL 8 option would be GCC toolsets (the equivalent of devtoolsets in CentOS/RHEL 7). In those cases it's the path to the toolset that changes, not the name of the binary. I could add a check if the gcc version is not 8 (the default in RHEL 8) and attempt to load a GCC toolset, although the container hasn't installed any extra ones.

Refs: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/developing_c_and_cpp_applications_in_rhel_8/additional-toolsets-for-development_developing-applications

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated this to attempt to load a gcc-toolset if the host gcc version doesn't match.

@rvagg
Copy link
Member

rvagg commented Apr 1, 2022

Yeah .. the out of date Pi's .. that's a problem and unfortunately it'll be on me to deal with that. I haven't had to touch them for so long, apparently they've been pretty stable. But I really do need to get around to giving them a cleanup and update and I guess setting up the new containers should be on the TODO list since it's so overdue. Leave this one with me, I'm pretty backlogged at the moment but maybe I'll find time to do this in the next ~week as a distraction from everything else.

Copy link
Member

@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

Add a Debian 10 based armv7l container to be used as an extra way
to test the armv7l cross compiled binaries in addition to the Pi's.
@richardlau
Copy link
Member Author

I'm currently experimenting with armv7l containers running on the Altras as an extra/alternative/fallback means of testing the cross compiled armv7l binaries. I've set up https://ci.nodejs.org/job/node-test-binary-armv7l/ (based on node-test-binary-arm-12+ but fans onto our armv7l containers instead of the Pi's) and hooked it into my test job,https://ci.nodejs.org/job/richardlau-node-test-commit-arm-fanned/.

If this works out (I'm retesting as ran into nodejs/node#27862) then I propose adding this new job as an addition to run in parallel with the existing node-test-binary-arm-12+. We could temporarily put a condition in the fanned job to prevent running node-test-binary-arm-12+ for Node.js 18 until the Pi's get upgraded.

@richardlau richardlau marked this pull request as ready for review April 3, 2022 19:18
@richardlau
Copy link
Member Author

https://ci.nodejs.org/job/node-test-binary-armv7l/ looks to be working (tried Node.js 12, 14, 16 and master). I've gone ahead and added it to https://ci.nodejs.org/job/node-test-commit-arm-fanned/ and will keep an eye on the job. We are down to just two online Pi 2's in the CI (#2914) which is causing jobs to back up so having the containerized armv7l testing in place would be a good backup in case we'd need to temporarily disable https://ci.nodejs.org/job/node-test-binary-arm-12+/ in general (and not just for 18 until the containers on the Pi's are updated).

Marking this as ready to review on the basis that this could land and we switch over the cross compiler and test in the new armv7l container job now for Node.js 18 with the view to also test on the Pi's later when they are updated (i.e. we wouldn't be blocked by the Pi's).

@richardlau
Copy link
Member Author

richardlau commented Apr 7, 2022

Based on #2914 (comment), I'm going to go ahead with the backup plan (#2912 (comment)) of merging this so we're building with the updated toolchain but temporarily prevent https://ci.nodejs.org/job/node-test-binary-arm-12+/ from running on Node.js 18/master until the Pi's get updated. We'll have armv7l coverage in the https://ci.nodejs.org/job/node-test-binary-armv7l/ (armv7l containers on the Ampere Altras) which is already running on PR's.

I'll do this tomorrow (Friday 8 April) unless I hear any objections.

@richardlau richardlau merged commit 578f40a into nodejs:master Apr 8, 2022
@richardlau
Copy link
Member Author

FTR this is the condition I've put on https://ci.nodejs.org/job/node-test-binary-arm-12+/ in https://ci.nodejs.org/job/node-test-commit-arm-fanned/:

image

We can remove this condition when the Pi's are updated.

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

Successfully merging this pull request may close these issues.

3 participants