node-gyp icon indicating copy to clipboard operation
node-gyp copied to clipboard

Published 20 hours ago • verified publisher icon nodejs

feat: Add proper support for IBM i

Open kadler opened this issue 2 years ago • 8 comments

Checklist
  • [x] npm install && npm test passes
  • [ ] tests are included
  • [ ] documentation is changed or added
  • [x] commit message follows commit guidelines
Description of change

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.

kadler avatar Mar 03 '22 20:03 kadler

Related gpy-next PR: https://github.com/nodejs/gyp-next/pull/140

CC @ThePrez

kadler avatar Mar 03 '22 20:03 kadler

The gyp/ changes are a duplicate of nodejs/gyp-next#140. Ideally we'd merge in gyp-next and then update that here for those.

Otherwise the node-gyp parts LGTM.

Hello @richardlau and @kadler , The PR nodejs/gyp-next#140 has been merged. Can we merge this PR as well now?

dmabupt avatar Mar 28 '22 06:03 dmabupt

It looks like https://github.com/nodejs/gyp-next/pull/140 was released in gyp-next v0.11.0 so the next step would be to update gyp-next here (e.g. similar to one of the previous gyp-next updates, https://github.com/nodejs/node-gyp/pull/2521).

richardlau avatar Mar 28 '22 11:03 richardlau

It looks like nodejs/gyp-next#140 was released in gyp-next v0.11.0 so the next step would be to update gyp-next here (e.g. similar to one of the previous gyp-next updates, #2521).

Hello @richardlau , I have submitted a PR #2642 based on the patch (https://github.com/nodejs/gyp-next/compare/v0.10.0...v0.11.0.patch). Would you review it? And @kadler , maybe you need to remove the duplicated code changes in this PR to resolve the conflicts?

dmabupt avatar Apr 11 '22 02:04 dmabupt

Sorry, I didn't realize that gyp-next was an upstream of this package. If that's the case, then I think this PR is probably not necessary, since the changes would be covered by the updates in gyp-next 0.11.0. We can close this in favor of #2642 then.

kadler avatar Apr 12 '22 21:04 kadler

Sorry, I didn't realize that gyp-next was an upstream of this package. If that's the case, then I think this PR is probably not necessary, since the changes would be covered by the updates in gyp-next 0.11.0. We can close this in favor of #2642 then.

We'll still need to update the files outside of the gyp directory, which should be done in this PR.

richardlau avatar Apr 13 '22 14:04 richardlau

Ah, ok. I'll rebase once #2642 lands then.

kadler avatar Apr 13 '22 17:04 kadler

Duplicated PR landed -- https://github.com/nodejs/node/commit/b99bb57416f5eabc9d8375b93e03de21d8ed29ee So I closed feat(gyp): update gyp to v0.12.1

dmabupt avatar Apr 18 '22 02:04 dmabupt

I used update-gyp.py to update to gpy-next v0.13.0 and rebased my changes. This should obsolete #2642.

kadler avatar Aug 18 '22 15:08 kadler

I used update-gyp.py to update to gpy-next v0.13.0 and rebased my changes. This should obsolete #2642.

Closed issue feat(gyp): update gyp to v0.12.1

dmabupt avatar Sep 19 '22 06:09 dmabupt

Looks like this has been approved, can it get merged?

kadler avatar Sep 22 '22 16:09 kadler

@richardlau are you able to land?

mhdawson avatar Sep 23 '22 21:09 mhdawson