fix: nan and cpu-features should be really optional

[belgattitude]

Changes

Moving "nan" and "cpu-features" to peerDependencies (optional). While "optionalDependencies" looks the way to do it, it's actually not really what's intended here (imho)

Will fix

Prevent errors (in ci or whenever cmake is not there) and make install faster when you actually don't need cpu-features or nan.

Related

• Move optionalDependencies to peerDependencies #1030
• Error installing ssh2 v1.4.0 on CentOS 7 #1074
• How do I use ssh2 in a way which will not try to build cpu-features? #1055
• npm i ssh2 error  #1015
  (...)

Breaking changes ?

Moving to peerDependencies is the way to go (see context below, I can give other examples or threads).

If a dependent package used to rely on on 'nan' or 'cpu-features' without actually declare them in their deps, they'll have to do.

I have no idea how many packages actually rely on them, but as ssh2 is widely used (https://www.npmjs.com/package/ssh2), it might be safe to create a major version and add to the migration something like:

If you were relying on cpu-features or nan, they won't be installed by default. You can explicitly add them to your deps: yarn add [email protected] and/or yarn add nan@^2.15.0

Context

"optionalDependencies" might not be what we're looking.

• npm v6: https://docs.npmjs.com/cli/v6/configuring-npm/package-json#optionaldependencies
• npm v7: https://docs.npmjs.com/cli/v7/configuring-npm/package-json#optionaldependencies
• yarn v1: https://classic.yarnpkg.com/en/docs/dependency-types#toc-optionaldependencies
• yarn berry: https://yarnpkg.com/configuration/manifest#optionalDependencies

Quoting yarn doc:

Similar to the dependencies field, except that these entries will not be required to build properly should they have any build script. Note that such dependencies must still be resolvable and fetchable (otherwise we couldn't store it in the lockfile, which could lead to non-reproducible installs) - only the build step is optional.

This field is usually not what you're looking for, unless you depend on the fsevents package. If you need a package to be required only when a specific feature is used then use an optional peer dependency. Your users will have to satisfy it should they use the feature, but it won't cause the build errors to be silently swallowed when the feature is needed.

Per definition "optional" will still install the packages, and often the build step is actually failing.... (in CI for example)

Why peerDependencies and meta ?

To achieve the expected goal, the idea is to use peerDependencies and peerDependenciesMeta (with optional). It's supported in a consistent way by mainstream package managers:

• npm v6 - https://docs.npmjs.com/cli/v6/configuring-npm/package-json#peerdependencies
• npm v7 - https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependencies (+ meta)
• yarn v1 - https://classic.yarnpkg.com/en/docs/dependency-types#toc-peerdependencies
• yarn berry - https://yarnpkg.com/configuration/manifest#peerDependencies
• pnpm - https://pnpm.io/package_json#peerdependenciesmeta

[mscdex]

A few points:

• The optionalDependencies really are optional. If they fail to build, they do not cause the installation of ssh2 to fail.

• Node v10.16.0 is the current minimum supported version for ssh2. Node v10.16.0 is bundled with npm v6.9.0. This means that not only does it not support peerDependenciesMeta (which is only available since npm v6.11.0), but it also doesn't support the installation of peerDependencies by default (which is only available since npm v7.x according to the npm documentation).

• The peerDependencies feature is designed for plugins and other similar type packages. That is not the case here with nan and cpu-features and any warnings and such that may be printed by npm would be confusing.

• The npm documentation's simple description of optionalDependencies matches exactly what we're trying to achieve here:

  If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies object.

The whole point of these optional dependencies are to improve the end user's runtime performance out of the box. If their installations are not automatically attempted, users will not know (or possibly want) to do perform steps manually to get the performance benefits.

The other thing is, it was either for node-gyp 8.0 or possibly npm 8.0 that verbose build errors for optionalDependencies were/are going to be hidden anyway according to a Github issue comment I read the other day from Isaac himself I believe (I cannot find a link to this at the moment), so this should be even less of a problem as time goes on.

[mscdex]

Also, at least npm already provides flags for users who for whatever reason take issue with seeing build errors during npm install, including --no-optional and --quiet/--silent (which can all be set in your .npmrc as well if you want them to be more global and permanent).

[belgattitude]

Thanks for the comments, here's few remarks

Node v10.16.0... with npm v6.9.0. ... not support peerDependenciesMeta (only available in v6.11.0), but it also doesn't support the installation of peerDependencies by default

"doesn't support peerDependencies by default" -> that's actually doing the same as "optional" true. So no difference in intended behaviour, am i wrong ?

PS: IMHO, node 10 is unsupported -> remove node 10 support in ci -> ssh2 v6 would make sense.

The peerDependencies feature is designed for plugins and other similar type packages.

Not sure about this on npm.... But peerDeps are inherited dependencies somehow, i find them a bit everywhere nowadays.

Maybe this https:/npm/rfcs/blob/main/implemented/0025-install-peer-deps.md#motivation makes it a bit clearer.

That is not the case here with nan and cpu-features and any warnings and such that may be printed by npm would be confusing.

You mean more confusing than failing on build cause node-gyp is not there ? 😄

(and that for all packages that depends on ssh2 and that even if they actually not use those nan and cpu-features)

The npm documentation's simple description of optionalDependencies matches exactly what we're trying to achieve here.

Again the real thing is that the install actually breaks, but thanks to the def of optionalDependencies, it's allowed to.

But why do we need to fail ? error log, time (on ci this taking 15-20s on each run to try a build - i use testcontainer-node)

Am I missing something ? Do we really need this ?

optionalDependencies were/are going to be hidden anyway according to a Github issue comment I read the other day from Isaac... so this should be even less of a problem as time goes on. ... --quiet/--silent...

Yes possible, can't find an rfc for it but some things looks going in the same direction: https:/npm/rfcs/blob/main/implemented/0030-no-install-optional-peer-deps.md#prior-art.

--no-optional

I don't think this will gain wide adoption across package managers. By nature that would make a lock change depending on how you install ?

Yarn 2+ dropped it seems: yarnpkg/berry#2412 (comment)

I can't really argue about which format to give, and I'm just learning node... coming from different background.

Cause I don't really know the project, an important question for me:

Are cpu-features and/or nan supposed to be exposed externally by consuming packages ? Just to be sure

edit, i.e: a code that would call https:/mscdex/cpu-features/blob/master/lib/index.js

[belgattitude]

Ah I see, not really external but used as optimizations (assuming built succeed) https:/mscdex/ssh2/blob/master/lib/protocol/constants.js.

Nice optims btw :)

[mscdex]

You mean more confusing than failing on build cause node-gyp is not there ?

Yes, especially considering that npm clearly indicates in its logging that it's skipping the optional dependency when there's a build failure. Also, it wouldn't be because of missing node-gyp (that's bundled with npm), it would be a compiler error or other missing build infrastructure.

I don't think this will gain wide adoption across package managers. By nature that would make a lock change depending on how you install ?

I only focus on npm because that's what's bundled with node and I don't think every package maintainer should be concerned with the idiosyncrasies of each package manager that exists out there.

Ah I see, not really external but used as optimizations (assuming built succeed)

FWIW the addon bundled in ssh2 allows for decrypting in place (something node core does not currently support) as well as removing a bunch of javascript and c++ layers in between that node core has. These not only help improve speed but also help with memory usage.

As far as cpu-features goes and as you probably already saw, it is used to determine an optimal set/order of ciphers once on process startup.

The main driving force behind these optimizations was because of complaints users had made in the past about transfer speeds compared to other solutions. So in order to help alleviate those issues I implemented the aforementioned optimizations. Either way someone is evidently bound to complain and I'm a performance-oriented dev anyway, so I'm more inclined to stick with the way things currently are, especially given that like I said, npm/node-gyp is supposedly going to be hiding build errors for optional dependencies anyway.

[mscdex]

Another tidbit that's possibly worth throwing into this discussion is there is always the possibility of having pre-built versions of these optional dependencies, even though I'm personally not particularly fond of downloading/trusting binary blobs from npm, but if others would be happy with that then that might be worth exploring. I've yet to explore using Github Actions to generate such assets that could then be stored somewhere, perhaps a Github release as a download or something like that.

[belgattitude]

Wow nice explanation ! Great I love this thread and yes in that case it is optionalDependencies.

So here's another question...

Would it make sense to find an alternative to cpu-features made in js/node ?

I'll explain a bit the idea below:

There's two conditions here, we would have to be able to

1. Detect x86.

I think it's possible to make it work.

PS: Need to know if node.arch would be sufficient. At least they give insights about what architecture node was compiled. There's some projects trying to detect that from runtime (better-arch and to some extend os-name, envinfo, systeminformation where we could find the necessary experience)

2. Detect AVE.

AFAIK the only way in node would be to compare to a list of cpu architecture. Let's say something in the lines of "i7, i9, amd have all AVE"... The fallback would be to go "don't support AVE".

Just the same fallback when the cpu-features fail to build (which I imagine is happening a lot in real installs).

Of course that wouldn't be 100% exact and this list might have to be updated...

---

This is just ideas, of course...

And I would be pleased to assist / explore / create P/R in this direction.

Does it sound good to you ? Or is it something that you wouldn't like to consider ?

[belgattitude]

the possibility of having pre-built versions of these optional dependencies, even though I'm personally not particularly fond of downloading/trusting binary blobs from npm, but if others would be happy with that then that might be worth exploring.

Yes this is possible and quite exciting.

My experience with binaries (esbuild, swc, sharp, sentry) is that they seems to need a lot of work... So many arch, linux style (alpine musl, arm...) and I've been struggling to make those work lately (on alpine mostly).

PS: interesting too yarnpkg/berry#2751, but it's very early stage.

[mscdex]

Would it make sense to find an alternative to cpu-features made in js/node ?

Believe me, if it would be that easy, that's what I would've done in the first place. However you'll quickly find that if you start down that path, not only will you find yourself re-implementing fallbacks that Google's cpu_features already includes, but you'll also find that the only real (and cross platform) way to get reliable CPU info (especially CPU features) is at the C/C++ layer (even with that there are lots of edge cases and other considerations to be made that cpu_features already takes care of/smooths over).

I'd much rather not have to maintain something of that scale, so that's why I chose to leverage cpu_features instead.

My experience with binaries (esbuild, swc, sharp, sentry) is that they seems to need a lot of work... So many arch, linux style (alpine musl, arm...) and I've been struggling to make those work lately (on alpine mostly).

There are already modules that help with this sort of thing to some degree, such as node-pre-gyp. Github Actions already supports the big 3 OSes anyway (Linux, macOS, and Windows) and making various builds (32/64-bit, glibc/musl, etc.) on each platform where needed shouldn't be that difficult. There might even be existing tooling that takes care of all of most of this, I just haven't investigated it.

[belgattitude]

way to get reliable CPU info (especially CPU features) is at the C/C++ layer

Agree, but devils advocate saying --- "get reliable CPU" different than "get reliable node-gyp build" :)

And I sense if we had usage stats, we would be surprised to see that most those optims aren't enabled at all.

It's more about percentage of users that will effectively get the optims...

Just saying, don't take any of this badly and I'm no expert (just discovering node for a few months)

There are already modules that help with this sort of thing to some degree, such as node-pre-gyp.

Yep or the new kids on the block, like napi.rs which prisma use... They have a boilerplate too (I never tried, but I'd like to) and I guess it's possible to include things like https://crates.io/crates/cpufeatures... Not sure of the size though.

But it's quite a work to do at first.

I don't know if I would go there for now.

[mscdex]

Agree, but devils advocate saying --- "get reliable CPU" different than "get reliable node-gyp build" :)

And I sense if we had usage stats, we would be surprised to see that most those optims aren't enabled at all.

It's more about percentage of users that will effectively get the optims...

I don't understand what you're trying to say here.

and I guess it's possible to include things like https:/anrieff/libcpuid... Not sure of the size though.

Switching to another compiled library would not change anything.

At any rate, I feel like I've already laid out my position and believe there are no changes needed regarding the optional dependencies. I've given potential, existing solutions for npm users who have issues with seeing build errors for the optional dependencies that are not required to successfully use/install ssh2.

My suggestion at this point is to create issues on your favorite package managers' issue trackers about hiding optional dependency build errors if you're really concerned about seeing them.

[belgattitude]

I don't understand what you're trying to say here.

That maybe a js/node "non-perfect" solution would improve the actual detection for people not having cmake, and so on...

your favorite package managers' issue trackers about hiding optional dependency build errors if you're really concerned about seeing them.

It took me time to understand that's a real optionnalDependency, was mistaken thinking it a was a peer. Now I know and it's perfectly fine.

Sorry for misunderstandings if any. Happy day