Skip Build Unless Installing on IBM i

IBM / nodejs-idb-connector

A JavaScript (Node.js) library for communicating with Db2 for IBM i, with support for queries, procedures, and much more. Uses traditional callback-style syntax

MIT License

37 stars 23 forks source link

Skip Build Unless Installing on IBM i #54

Closed abmusse closed 5 years ago

abmusse commented 5 years ago

Original report by David Russo (Bitbucket: DavidRusso, GitHub: DavidRusso).

I work on a package that installs on multiple platforms, including IBM i. I've put idb-connector under optionalDependencies in my package.json -- this allows NPM to proceed with installing my package on non-IBM i platforms, where idb-connector obviously does not work. Programming logic in my package then selects the appropriate database connector for the appropriate platform.

This allows me to install on any platform, but I get a lot of unnecessary output and error messages when installing on non-IBM i platforms because NPM goes through the motions of trying to build idb-connector. To see what I mean, try 'npm install profoundjs' on a Windows PC.

I'm wondering if it's possible to set up so that NPM does not try to build/install idb-connector unless installing on IBM i.

Maybe it's just a matter of setting the 'os' key in idb-connector's package.json?

https://docs.npmjs.com/files/package.json#os

abmusse commented 5 years ago

Original comment by David Russo (Bitbucket: DavidRusso, GitHub: DavidRusso).

Thanks, this will be a nice improvement! I'll try it out when it's released.

abmusse commented 5 years ago

Original comment by Xu Meng (Bitbucket: mengxumx, GitHub: dmabupt).

commit 6a16b3b

abmusse commented 5 years ago

Original comment by Xu Meng (Bitbucket: mengxumx, GitHub: dmabupt).

The host operating system is determined by process.platform which currently return aix on IBM i. So the AIX system will still install it ( while many other os not). We may let it return the true os name in future Node.js releases. But for now, seems we can only use aix to distinguish.

dmabupt commented 5 years ago

Hello @DavidRusso , I have upgraded idb-connector to 1.1.8 which contains the changes. Btw, we have moved the repository along with the pre-compiled binaries to GitHub. So more tests are appreciated

ThePrez commented 5 years ago

This change does not work for AIX. Can you make it for AIX too?

kadler commented 5 years ago

I'm not sure how it could not work on AIX, since it only references "aix" for which builds to skip. Or do AIX builds put the version in the OS string?

ThePrez commented 5 years ago

The build is not skipped on AIX as it should be, and proceeds to fail.

abmusse commented 5 years ago

In our package.json we specify "os": "aix"

For us process.platform returns "aix" so

install script continues
searches for pre built binaries and cannot find any matching "package_name": "{module_name}-v{version}-{node_abi}-ibmi-{arch}.tar.gz"
Then falls back to build from source and fails.

os.type() returns OS400 appropriately.

For it to skip on AIX we would need to dig into how process.platform is determined or find another work around.

For now we can simply state package does not support AIX in readme.

I've seen some packages override install script which is probably not a good thing.

Example is Appmetrics

kadler commented 5 years ago

Why does it need to work on AIX in the first place? This requires libdb400, which isn't available on AIX anyway.

abmusse commented 5 years ago

Right it cannot work on AIX. The failure is expected

Our package.json whitelists AIX as our only supported platform

npm install idb-connector

On every other platform will output something like

Unsupported platform for idb-connector
npm ERR! code EBADPLATFORM
npm ERR! notsup Unsupported platform for idb-connector@1.1.8: wanted {"os":"aix","arch":"any"}

but on AIX it will actually try to build the project and fail.

I guess we should just clearly state in our readme this package is only for IBM i

dmabupt commented 5 years ago

PR #57 is created.

ThePrez commented 5 years ago

This is holding up React.js support on IBM i, so I consider this a high-priority item (or working to find alternative implementation for default-gateway)

kadler commented 5 years ago

@ThePrez I'm trying to figure how that could be. Can you link to that issue?

abmusse commented 5 years ago

@kadler https://github.com/silverwind/default-gateway/issues/5 https://github.com/silverwind/default-gateway/issues/10

ThePrez commented 5 years ago

We could skip the native parts in the .gyp file based on an "os400" condition. That would be pretty clean

silverwind commented 5 years ago

npm has issues with honoring the os property in some cases, so it it is not enough to prevent a build on non-AIX. Anyways, I have now decided to only accept AIX support in my module if it is a pure-JS implementation. Native modules are just not worth the headaches they bring in a cross-platform module.

Regarding React: We decided to remove the os property entirely, so dependency installs will not fail on AIX, allowing dependants to treat the thrown unsupported platform error themselves. The fix is pending at https://github.com/sindresorhus/internal-ip/pull/25.

dmabupt commented 5 years ago

idb-connector v1.1.9 is released now. It takes the code from PR #58