silverwind / default-gateway

Get the default network gateway, cross-platform.
86 stars 19 forks source link

Added support for AIX platform (for issue #5) #9

Closed patrickhrastnik closed 5 years ago

patrickhrastnik commented 5 years ago

The script aix.js is compatible with AIX on PPC64 architecture. If some time in future AIX on another architecture should be supported, then add separate code paths, depending on the architecture.

Solves issue #5

silverwind commented 5 years ago

I'm really worried about adding a C++ dependency, even if optional because if I remember correctly package installation can under some circumstances fail if a optional dependency fails to build. I assume you looked around if there is a pure JS alternative? Can we be sure that a build it not attempted if platform is not AIX?

patrickhrastnik commented 5 years ago

I tried everything me and my boss could come up with to gather the information using vanilla js, but no other approach worked :/ I understand your worries. Would it be possible to manage this situation with a pre-install script that only installs idb-connector if it‘s aix on ppc64? I have no such experience, but I read somewhere there‘s an event hook for this... Another possibility I have to figure out: It may be possible to require the package directly from the system. There‘s supposed to be some directory the package is installed globally.

silverwind commented 5 years ago

It may actually be fine. The package specifies os in package.json and if npm documentation is to be trusted, this means the package won't install on other platforms. Still, I will inspect their build conditions more closely. A shame that node packages can not have platform-specific dependencies easily.

patrickhrastnik commented 5 years ago

In terms of the optional dependency on idb-connector: According to npm, optional dependencies don't cause the installation to fail if they don't build. And the idb-connector states in it's package.json "os": {"aix"} as well. So there shouldn't be an issue with this... Referencing a global package may cause some trouble in case the relative path to the package changes, and it might depend on the system as well, where you can find the package. And, as well, I couldn't find the global package path on our system. Is it okay for you if we leave it like this, or do you insist on finding another solution?

silverwind commented 5 years ago

Referencing a global package may cause some trouble in case the relative path to the package changes

Not sure what you mean. We only have local dependencies (in node_modules), what could be the issue?

Is it okay for you if we leave it like this, or do you insist on finding another solution?

It's fine as it is.

patrickhrastnik commented 5 years ago

I tested it on our aix (as/400) system and on my windows machine. Didn‘t cause problems as far as I was able to test it. I assumed the Travis CI takes care of the other systems (Linux etc) as I have no Linux machine available.

silverwind commented 5 years ago

Travis isn't actually in use (e.g. tests are skipped) because last I checked, their VMs did not have network interfaces, but that may have changed recently.

patrickhrastnik commented 5 years ago

Okay, so we need to run separate tests on other systems manually as well? Do you have systems you can test the package on?

There changes shouldn‘t interfer with the previous code at all, since I put it into a separate file... but ofc, a package used that much needs to be tested properly.

silverwind commented 5 years ago

No need to run tests elsewhere, the change can possibly only affect AIX. I will look into enabling tests on travis again later.

patrickhrastnik commented 5 years ago

Is there anything left to do for me?

silverwind commented 5 years ago

I merged it to master now, with additional tweaks in https://github.com/silverwind/default-gateway/commit/f7903f51c0563049e4a703694ddddcf43fbd0642. Can you please test current master on your machine?

patrickhrastnik commented 5 years ago

Tested -> works as expected