search

mapbox / node-pre-gyp

Node.js tool for easy binary deployment of C++ addons
BSD 3-Clause "New" or "Revised" License
1.11k stars 259 forks source link

Differentiate between aix and ibmi in package versioning #593

Closed markdirish closed 1 year ago

markdirish commented 3 years ago

Fixes #592

Signed-off-by: Mark Irish mirish@ibm.com

markdirish commented 3 years ago

I have tested this on a N-API package I maintain, and on IBM i it both:

  1. Looks for a binary with ibm as the platform in the version name instead of aix
  2. Packages binaries in tarballs with ibmi as the platform in the version name instead of aix
markdirish commented 3 years ago

Appears that the only failing portion of the CI build is the comment block below mine that was indented incorrectly, but that I left untouched.

springmeyer commented 2 years ago

Thanks for providing this fix @markdirish. Because this seems like it would break existing installs (it would change resolution of existing binaries that might expect aix, right?) I think I'll need to include this in a new major version of @mapbox/node-pre-gyp. Sound good?

markdirish commented 2 years ago

I think that's right. If you update now, AIX users will be unaffected because they were aix before and will be aix after. IBM i users will go from aix to ibmi, so a user running and older version of node-pre-gyp will try to pull aix, while a package developer may have updated and now package ibmi binaries. The reverse could also be true.

Then again, I'm not sure how many IBM i binaries there are out that that use node-pre-gyp other than our (IBM i OSS team) packages, and we could always just update our packages to require a minimum of whatever version this change is added into, and then ship releases from that dependency change with the ibmi instead of aix identifier.

springmeyer commented 2 years ago

Thanks for the additional context @markdirish. I also just had a chance to read https://github.com/mapbox/node-pre-gyp/issues/592. I'm now understanding that this is likely to not break anyone out there, so is probably safe in any upcoming release.

But I have one remaining question: why not just work with the node core developers to land a patch that properly updates node such that process.platform() returned what you'd expect it to?