Skip to content

Skip Build Unless Installing on IBM i #54

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.

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

Closed
abmusse opened this issue Jan 17, 2019 · 17 comments
Closed

Skip Build Unless Installing on IBM i #54

abmusse opened this issue Jan 17, 2019 · 17 comments
Labels
enhancement New feature or request major

Comments

@abmusse
Copy link
Member

abmusse commented Jan 17, 2019

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
Copy link
Member Author

abmusse commented Jan 23, 2019

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.

@abmusse
Copy link
Member Author

abmusse commented Jan 23, 2019

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


commit 6a16b3b

@abmusse
Copy link
Member Author

abmusse commented Jan 24, 2019

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 abmusse added major enhancement New feature or request labels Jan 24, 2019
dmabupt added a commit that referenced this issue Jan 25, 2019
config: Moved the repository to GitHub
@dmabupt
Copy link
Contributor

dmabupt commented Jan 25, 2019

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

@dmabupt dmabupt closed this as completed Jan 27, 2019
@ThePrez
Copy link
Member

ThePrez commented Feb 7, 2019

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

@ThePrez ThePrez reopened this Feb 7, 2019
@kadler
Copy link
Member

kadler commented Feb 7, 2019

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
Copy link
Member

ThePrez commented Feb 7, 2019

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

@abmusse
Copy link
Member Author

abmusse commented Feb 7, 2019

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

image

image

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.

image

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
Copy link
Member

kadler commented Feb 7, 2019

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

@abmusse
Copy link
Member Author

abmusse commented Feb 7, 2019

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 [email protected]: 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
Copy link
Contributor

dmabupt commented Feb 11, 2019

PR #57 is created.

@ThePrez
Copy link
Member

ThePrez commented Feb 22, 2019

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
Copy link
Member

kadler commented Feb 22, 2019

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

@abmusse
Copy link
Member Author

abmusse commented Feb 22, 2019

@ThePrez
Copy link
Member

ThePrez commented Feb 22, 2019

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

@silverwind
Copy link

silverwind commented Feb 23, 2019

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 sindresorhus/internal-ip#25.

@dmabupt
Copy link
Contributor

dmabupt commented Feb 25, 2019

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

@dmabupt dmabupt closed this as completed Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major
Projects
None yet
Development

No branches or pull requests

5 participants