FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build,test: add proper support for IBM i by dmabupt · Pull Request #42689 · nodejs/node · GitHub

/ node Public

build,test: add proper support for IBM i - #42689

Closed
dmabupt wants to merge 3 commits into
nodejs:mainfrom
dmabupt:py39_test
ConversationCommits (3)ChecksFiles changed
Closed

build,test: add proper support for IBM i#42689
dmabupt wants to merge 3 commits into
nodejs:mainfrom
dmabupt:py39_test

Conversation

dmabupt commented Apr 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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:

build,test: add proper support for IBM i
c6ab4d5 
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.

nodejs-github-bot commented Apr 11, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

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

nodejs-github-bot added build Issues and PRs related to build files or the CI. 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 Apr 11, 2022
dmabupt added the ibm i Issues and PRs related to the IBM i platform. label Apr 11, 2022
dmabupt marked this pull request as draft April 11, 2022 03:45

kadler commented Apr 12, 2022

Copy link
Copy Markdown

These changes depend on related changes in nodejs/gyp-next#140 being pulled in (now released in gyp-next 0.11.0) as well as those changes being merged in to node-gyp and then merged in here.

The v8 build changes use process.platform() in Python, which has always returned "OS400" regardless of the Python version, so I believe we are not dependent on those changes (though @V-for-Vasili is going to push the changes to distinguish AIX and OS400 upstream regardless).

richardlau commented Apr 12, 2022

Copy link
Copy Markdown
Member

These changes depend on related changes in nodejs/gyp-next#140 being pulled in (now released in gyp-next 0.11.0) as well as those changes being merged in to node-gyp and then merged in here.

#42625 updated gyp-next here and included the IBM i changes in nodejs/gyp-next#140.

Reviewing the separate node-gyp changes are on my list of things to do.

targos commented Nov 8, 2022

Copy link
Copy Markdown
Member

Is this going to be finished at some point?

kadler commented Nov 10, 2022

Copy link
Copy Markdown

I forgot about this and thought it had already been merged. We definitely want to get this in for Node 20. The gyp-next changes have already been pulled in as part of #44605 @dmabupt can you please bring it out of draft and rebase as-needed?

We'll also need to get npm to update their copy of node-gyp with at least v9.2.0 and then get that version pulled in here.

kadler commented Nov 10, 2022 •
edited
Loading

Copy link
Copy Markdown

Looks like npm/cli already pulled in node-gyp 9.3.0 as of 9.1, but the version shipped here is 8.19.x. I'm not sure if that can be upgraded to 9.1 or if we'll need npm to update node-gyp in the 8.19 branch.

targos commented Nov 11, 2022

Copy link
Copy Markdown
Member

npm will be soon updated to v9.x see #45395

kadler commented Nov 17, 2022

Copy link
Copy Markdown

Looks like npm has been upgraded to 9.1.2 so we should be good to go. @dmabupt can you unmark as draft and fix up conflicts please.

targos marked this pull request as ready for review November 18, 2022 06:09

dmabupt commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

I created a new PR #46510 to track this issue. Close this one.

dmabupt closed this Feb 6, 2023
dmabupt deleted the py39_test branch February 13, 2023 02:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. ibm i Issues and PRs related to the IBM i platform. 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.

5 participants

Back | FazBrowse Home | New Git URL