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.

Sign up for GitHub

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

Jump to bottom

build,test: add proper support for IBM i #46510

Closed
wants to merge 1 commit into from
Closed

build,test: add proper support for IBM i #46510

wants to merge 1 commit into from

Conversation

dmabupt
Copy link
Contributor

@dmabupt dmabupt commented Feb 6, 2023 •
edited

Python 3.9 on IBM i now properly returns "os400" for sys.platform
instead of claiming to be AIX as it did previously. While the IBM i PASE
environment is compatible with AIX, it is a subset and has numerous
differences which makes it beneficial to distinguish, however this means
that it now needs explicit support here.

Related PR:

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/net
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dependencies Pull requests that update a dependency file. dont-land-on-v14.x gyp Issues and PRs related to the GYP tool and .gyp build files libuv Issues and PRs related to the libuv dependency or the uv binding. needs-ci PRs that need a full CI run. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Feb 6, 2023
@dmabupt dmabupt mentioned this pull request Feb 6, 2023
Closed
@gireeshpunathil gireeshpunathil added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 6, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5164/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5164/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5164/

@nodejs-github-bot
Copy link
Collaborator

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

@richardlau
Copy link
Member

I tried to build this on our CI and this is failing on the IBM i instances we have: https://ci.nodejs.org/job/node-test-commit-ibmi/1051/nodes=ibmi73-ppc64/console
I believe the IBM i machine that it ran on is using Python 3.6.12 (or at least that is what configure found) and this PR has removed the parts of the gyp files that differentiate between AIX and IBM i when older versions of Python 3 are in use.

I can see a difference in the link commands for icupkg:
Previously:

  g++-10 -pthread -Wl,-bbigtoc -maix64 -Wl,-blibpath:/QOpenSys/pkgs/lib:/QOpenSys/usr/lib -Wl,-brtl  -o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/icupkg /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/icupkg/deps/icu-small/source/tools/icupkg/icupkg.o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/icupkg/tools/icu/no-op.o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/tools/icu/libicutools.a

with this PR (note the incorrect blibpath):

  g++-10 -pthread -Wl,-bbigtoc -maix64 -Wl,-blibpath:/usr/lib:/lib:/opt/freeware/lib/pthread/ppc64  -o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/icupkg /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/icupkg/deps/icu-small/source/tools/icupkg/icupkg.o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/icupkg/tools/icu/no-op.o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/tools/icu/libicutools.a

@github-actions github-actions bot mentioned this pull request Feb 7, 2023
Open
18 tasks
@dmabupt
build,test: add proper support for IBM i
ea9bdee 
Python 3.9 on IBM i now properly returns "os400" for sys.platform
instead of claiming to be AIX as it did previously. While the IBM i PASE
environment is compatible with AIX, it is a subset and has numerous
differences which makes it beneficial to distinguish, however this means
that it now needs explicit support here.
@dmabupt
Copy link
Contributor Author

dmabupt commented Feb 7, 2023 •
edited

I tried to build this on our CI and this is failing on the IBM i instances we have: https://ci.nodejs.org/job/node-test-commit-ibmi/1051/nodes=ibmi73-ppc64/console I believe the IBM i machine that it ran on is using Python 3.6.12 (or at least that is what configure found) and this PR has removed the parts of the gyp files that differentiate between AIX and IBM i when older versions of Python 3 are in use.

I can see a difference in the link commands for icupkg: Previously:

  g++-10 -pthread -Wl,-bbigtoc -maix64 -Wl,-blibpath:/QOpenSys/pkgs/lib:/QOpenSys/usr/lib -Wl,-brtl  -o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/icupkg /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/icupkg/deps/icu-small/source/tools/icupkg/icupkg.o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/icupkg/tools/icu/no-op.o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/tools/icu/libicutools.a

with this PR (note the incorrect blibpath):

  g++-10 -pthread -Wl,-bbigtoc -maix64 -Wl,-blibpath:/usr/lib:/lib:/opt/freeware/lib/pthread/ppc64  -o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/icupkg /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/icupkg/deps/icu-small/source/tools/icupkg/icupkg.o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/icupkg/tools/icu/no-op.o /home/IOJS/build/workspace/node-test-commit-ibmi/nodes/ibmi73-ppc64/out/Release/obj.target/tools/icu/libicutools.a

Yes, I haven't tested it with python 3.6. I just changed the patch to support python 3.6 as well.

@richardlau
Copy link
Member

IBM i CI run: https://ci.nodejs.org/job/node-test-commit-ibmi/1053/ (FWIW the runs on main currently have failing tests, so this run will likely fail but we're looking to see that the compilation is still successful and the test results are similar to main).

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 7, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 7, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5167/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5167/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5167/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5168/

@kadler
Copy link

kadler commented Feb 7, 2023

I tried to build this on our CI and this is failing on the IBM i instances we have: https://ci.nodejs.org/job/node-test-commit-ibmi/1051/nodes=ibmi73-ppc64/console I believe the IBM i machine that it ran on is using Python 3.6.12 (or at least that is what configure found) and this PR has removed the parts of the gyp files that differentiate between AIX and IBM i when older versions of Python 3 are in use.

We should definitely get Python 3.9 installed on the CI systems since Python 3.6 went out of support at the end of 2021.

mhdawson
mhdawson approved these changes Feb 10, 2023
View reviewed changes
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

@dmabupt dmabupt closed this by deleting the head repository Feb 20, 2023
@dmabupt dmabupt mentioned this pull request Feb 20, 2023
Merged
@dmabupt
Copy link
Contributor Author

dmabupt commented Feb 20, 2023 •
edited

Oops! I deleted my forked repo (to change the 'master' branch to 'main' to keep align with this) then the PR is closed. I opened a new PR #46739.

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

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dependencies Pull requests that update a dependency file. gyp Issues and PRs related to the GYP tool and .gyp build files libuv Issues and PRs related to the libuv dependency or the uv binding. needs-ci PRs that need a full CI run. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dmabupt @nodejs-github-bot @richardlau @kadler @cjihrig @gireeshpunathil @mhdawson