-
Notifications
You must be signed in to change notification settings - Fork 664
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
Remove cpu_features dependency #1083
Comments
Related |
With no disrespect to anyone offering their-written software for free, please kindly do not make a widely used and important module ssh2 dependent on any clearly undermaintained packages like cpu-features. |
@PRR24 I'm not sure how you arrived at "clearly undermaintained" for |
@mscdex While that is technically true, unfortunately NPM installs it by default and shows a big fat warning when it fails. |
Well, mscdex/cpu-features#4 you explicitly state that you have not have time to push the 0.0.3 update in last 23 days. It is hard to call a library that fails to install on Linux in standard Node environment for weeks, anything but undermaintained. Again, with no disrespect -- while such latency is IMO fully acceptable for the cpu-features library, it is a problem for the library as important and widely used as ssh2. And IMO it does not matter, if the library is optional of not, ssh2 is a library that because of its nature should shine and scream of quality and reliability and current situation undermines it a lot. I kindly ask you to take my input not as a mindless rant, but a constructive criticism. I do appreciate your work. |
@PRR24 maybe try apologising instead. Calling someone’s open source work “undermaintained” is downright rude. The words “With no disrespect” are not a magic spell that makes being rude ok. @WeeJeWel cpu-features is included for good reasons. The error message is only a problem if someone doesn’t read the all-caps |
Failure to build the cpu_features dependency can cause CI builds to fail, even though npm knows the dependency is optional. If you're using electron-build, turning off "build dependencies from source" is an effective temporary workaround. In package.json:
|
This is an issue for me because when I try to create a lambda it fails due to this package having unexpected syntax. See what happens if you create a project, add this package as a dependency, create a serverless.yml file, and run |
@bmcilw1 I don't see the connection between a vague "unexpected syntax" error and |
I didn't specify, but I can recreate it shortly. UPDATE - here we go. First, I create a new node project and add ssh2 and sls as dependencies. Then, I run
So from that, I know that this package has not been installed correctly. I then try to create a deployment with UPDATE 2 - It seems like I can't recreate the issue. I'm assuming this was due to some corrupt file state, though it WAS still throwing the same error even after deleting the node_modules dir. Thanks for the response. |
@priceaj Looks like other people have the same issues with ssh2 that we have. |
For those who are willing to test a new prebuilt addon system for me that may help alleviate some of the concerns raised (namely needing to have a build environment available for most common platforms), see #1114. |
The issue in cpu_features was fixed in 0.7.0 just released: mscdex/cpu-features#7 |
1.8.0 which includes mscdex/cpu-features#7 was released, so this can be closed now: https://github.com/mscdex/ssh2/releases/tag/v1.8.0 |
The
cpu_features
dependency does not install on M1 Macs.We use
homey -> dockerode -> docker-modem -> ssh2 -> cpu_features
in our module, which is installed usingnpm i -g homey
, as usual.Unfortunately, all M1 Mac users get a big error and they think the install failed.
The text was updated successfully, but these errors were encountered: