Skip to content
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

Closed
WeeJeWel opened this issue Oct 22, 2021 · 14 comments
Closed

Remove cpu_features dependency #1083

WeeJeWel opened this issue Oct 22, 2021 · 14 comments

Comments

@WeeJeWel
Copy link

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 using npm i -g homey, as usual.

Unfortunately, all M1 Mac users get a big error and they think the install failed.

@mscdex
Copy link
Owner

mscdex commented Oct 22, 2021

Related ssh2 discussion about the dependency in general: #1080
Related PR to add support for M1 in cpu_features: google/cpu_features#150

@PRR24
Copy link

PRR24 commented Oct 27, 2021

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.

@mscdex
Copy link
Owner

mscdex commented Oct 27, 2021

@PRR24 I'm not sure how you arrived at "clearly undermaintained" for cpu-features. Also, cpu-features is an optional dependency that it no way affects the successful installation of ssh2.

@WeeJeWel
Copy link
Author

Also, cpu-features is an optional dependency that it no way affects the successful installation of ssh2.

@mscdex While that is technically true, unfortunately NPM installs it by default and shows a big fat warning when it fails.

@PRR24
Copy link

PRR24 commented Oct 27, 2021

@PRR24 I'm not sure how you arrived at "clearly undermaintained" for cpu-features. Also, cpu-features is an optional dependency that it no way affects the successful installation of ssh2.

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.

@mikl
Copy link

mikl commented Nov 5, 2021

@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 SKIPPING OPTIONAL DEPENDENCY npm helpfully prints. And as mscdex pointed out, this is just a temporary inconvenience that will be solved once the upstream library adds support for your fancy new Mac.

@diamondap
Copy link

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:

    "build": {
		...
        "buildDependenciesFromSource": false,
        ...
    },

@thebrianbug
Copy link

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 sls deploy. It breaks with Unexpected Syntax due to this package.

@mscdex
Copy link
Owner

mscdex commented Nov 12, 2021

@bmcilw1 I don't see the connection between a vague "unexpected syntax" error and ssh2's dependency on cpu-features

@thebrianbug
Copy link

thebrianbug commented Nov 12, 2021

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 npm install. I get some errors on Mac.

> cpu-features@0.0.2 install /Users/brianmcilwain/source/ah-applications-nx/node_modules/cpu-features
> node-gyp rebuild

  ACTION Configuring dependencies /Users/brianmcilwain/source/ah-applications-nx/node_modules/cpu-features/deps/cpu_features/build/Makefile
/bin/sh: cmake: command not found
make: *** [/Users/brianmcilwain/source/ah-applications-nx/node_modules/cpu-features/deps/cpu_features/build/Makefile] Error 127
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/Users/brianmcilwain/.nvm/versions/node/v14.16.1/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:194:23)
gyp ERR! stack     at ChildProcess.emit (events.js:315:20)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:277:12)
gyp ERR! System Darwin 21.1.0
gyp ERR! command "/Users/brianmcilwain/.nvm/versions/node/v14.16.1/bin/node" "/Users/brianmcilwain/.nvm/versions/node/v14.16.1/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /Users/brianmcilwain/source/ah-applications-nx/node_modules/cpu-features
gyp ERR! node -v v14.16.1
gyp ERR! node-gyp -v v5.1.0
gyp ERR! not ok 

So from that, I know that this package has not been installed correctly. I then try to create a deployment with sls deploy and was getting an "unexpected token" error. Now that I try again, it somehow seems to be working again. I still get the build error thrown above, but not the unexpected token error. So maybe this was a red herring, or weird file state on my machine somehow? Sorry if that's the case 🤷

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.

@worksofliam
Copy link

@priceaj Looks like other people have the same issues with ssh2 that we have.

@mscdex
Copy link
Owner

mscdex commented Jan 6, 2022

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.

@technicalpickles
Copy link

The issue in cpu_features was fixed in 0.7.0 just released: mscdex/cpu-features#7

@technicalpickles
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants