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

[RRFC] Add libc fields to select optionalDependencies should be installed or skipped #438

Closed
Tracked by #642
Brooooooklyn opened this issue Aug 25, 2021 · 31 comments
Labels

Comments

@Brooooooklyn
Copy link

Brooooooklyn commented Aug 25, 2021

Motivation ("The Why")

Example

For my packages published as prebuilt native addons, I config the native packages cross-platform as optionalDependencies to let npm choose the suitable one to download. And I can avoid written postinstall scripts to download them in this way.
Like canvas package:

The @napi-rs/canvas package is published for users depending on it directly:

{
  "name": "@napi-rs/canvas",
  "version": "0.1.6",
  "optionalDependencies": {
    "@napi-rs/canvas-linux-x64-gnu": "0.1.6",
    "@napi-rs/canvas-linux-x64-musl": "0.1.6",
    "@napi-rs/canvas-darwin-x64": "0.1.6",
    "@napi-rs/canvas-win32-x64": "0.1.6",
    "@napi-rs/canvas-linux-aarch64-gnu": "0.1.6",
    "@napi-rs/canvas-linux-aarch64-musl": "0.1.6"
  }
}

And the packages include prebuilt native addons:

{
  "name": "@napi-rs/canvas-linux-x64-musl",
  "version": "0.1.6",
  "os": ["linux"],
  "cpu": ["x64"],
  "main": "skia.linux-x64-musl.node",
  "files": ["skia.linux-x64-musl.node"]
}
{
  "name": "@napi-rs/canvas-linux-x64-gnu",
  "version": "0.1.6",
  "os": ["linux"],
  "cpu": ["x64"],
  "main": "skia.linux-x64-gnu.node",
  "files": ["skia.linux-x64-gnu.node"]
}

In the macOS/Windows/FreeBSD this mechanism works fine, but on the Linux systems, npm will download two native binding packages, both gnu and musl. It will significantly increase the install size. The install size of @napi-rs/canvas on linux-x64 systems is 37mb which could be the half of it.

References

@Brooooooklyn Brooooooklyn changed the title [RRFC] Add libc fields to select optionalDependencies should be install or skip [RRFC] Add libc fields to select optionalDependencies should be installed or skipped Aug 25, 2021
@hyj1991
Copy link

hyj1991 commented Nov 8, 2021

Also as process.versions.modules, npm should also add modules field to support downloading the appropriate package of the current nodejs version, such as:

{
  "name": "example",
  "version": "0.0.1",
  "optionalDependencies": {
    // for node-v8.x
    "example-linux-x64-v57-gnu": "0.0.1",
    // for node-v10.x
    "example-linux-x64-v64-gnu": "0.0.1",
    // for node-v12.x
    "example-linux-x64-v72-gnu": "0.0.1",  
    // for node-v14.x
    "example-linux-x64-v83-gnu": "0.0.1",  
    // for node-v16.x
    "example-linux-x64-v93-gnu": "0.0.1",  
}

@isaacs
Copy link
Contributor

isaacs commented Nov 10, 2021

If node.js put more of that info in process.versions (as suggested in nodejs/node#39877) then it would be a relatively straightforward thing to provide a way to select based on it.

The other workaround is to use a preinstall script in each optional dep that checks the environment and exits with a non-zero status code if it's not suitable. This might be a good idea anyway, just to prevent someone using it in a place where it'll never work. If an optional dependency fails its install scripts, npm will clean it up. (Ie, remove it along with any dependencies that are no longer needed as a result of its removal.)

For selecting based on node version, you can use "engines": {"node":"<semver range>"} and npm will avoid even attempting to install optional deps with mismatched engine requirements.

(For non-optional deps, a failing install script will fail and roll back the entire install, and in the case of a mismatched engines.node range, it'll warn but continue, unless the user has specified --engines-strict.)

@Brooooooklyn
Copy link
Author

The other workaround is to use a preinstall script in each optional dep that checks the environment and exits with a non-zero status code if it's not suitable. This might be a good idea anyway, just to prevent someone using it in a place where it'll never work.

It won't be a suitable solution if #488 landed

@Brooooooklyn
Copy link
Author

@styfle here is glibcVersionCompiler and glibcVersionRuntime in process.report.getReport().header. Could we approach this feature via the glibcVersionRuntime filed?

@styfle
Copy link

styfle commented Dec 28, 2021

@Brooooooklyn Yeah that sounds like it could work 👍

I could imagine a new package.json field with "glibcVersionRuntime": ">=2 <3" kind of like engines and that would only install the glibc version of the package.

However, I'm not sure how this would work for the musl version. How would you negate it? "glibcVersionRuntime": "<=0"?

I'm almost thinking we need to expose family instead like detect-libc does. That way each optional dependency can match on the correct family.

@Brooooooklyn
Copy link
Author

In my opinion, libc runtime is part of the system, not Node.js engine. So I prefer a filed libc: ['glibc', 'musl', 'msvc'] like os and cpu.

And I don't like the idea that introduce version restrict to the libc runtime. The version of libc is just like the version of OS. They do not follow the semver, and lack documentation to list the compatible matrix.

@styfle
Copy link

styfle commented Jan 1, 2022

I like the libc: ['glibc', 'musl', 'msvc'] idea 👍

Now the question is, does it make sense to implement this in Node.js core or should this be implemented in npm?

The advantage of Node.js core is that other package managers could easily implement the feature (yarn, pnpm, etc).

The disadvantage of Node.js core is that the user must update both node and npm in order to use this feature.

@lovell
Copy link

lovell commented Jan 2, 2022

Hello, I help maintain the detect-libc module mentioned here, which is now a dependency of quite a few install-time tools. It has not been updated in over 4 years and I'd like to simplify/modernise it, especially if doing so would help get this useful feature added to npm itself.

There's a list of possible improvements in lovell/detect-libc#14 that might be of interest.

@arcanis
Copy link

arcanis commented Jan 10, 2022

While a little off-topic, I'm open to add support for a libc field in Yarn.

Main issue I'd have is the difficulty around detecting it (detect-libc is a good start, but it's currently sync; overall I'd prefer if this information came from Node, since it should be possible to get it at build-time), and decide whether the field should also expose individual majors (ie libc: ['glibc2']).

@Brooooooklyn
Copy link
Author

/cc @zkochan

@Brooooooklyn
Copy link
Author

overall I'd prefer if this information came from Node, since it should be possible to get it at build-time), and decide whether the field should also expose individual majors (ie libc: ['glibc2']).

Node.js already has the process.report.getReport() API to detect glibc versions: https://nodejs.org/api/report.html#diagnostic-report

{
  "header": {
    "reportVersion": 1,
    "nodejsVersion": "v12.0.0-pre",
    "glibcVersionRuntime": "2.17",
    "glibcVersionCompiler": "2.17",
    "wordSize": "64 bit",
    "arch": "x64",
    "platform": "linux",
    "componentVersions": {
      "node": "12.0.0-pre",
      "v8": "7.1.302.28-node.5",
      "uv": "1.24.1",
      "zlib": "1.2.11",
      "ares": "1.15.0",
      "modules": "68",
      "nghttp2": "1.34.0",
      "napi": "3",
      "llhttp": "1.0.1",
      "openssl": "1.1.0j"
    }
  }
}

But only for glibc systems. While I think it's enough for the native addons:

The postinstall parts in this pull request should be implemented in package managers.

@arcanis
Copy link

arcanis commented Jan 11, 2022

Right, the implementation of the skip mechanism will be up to each package manager, I'm not too worried about that.

My point is that knowing whether the system uses the glibc or musl should ideally come from either process.versions or process.report.getReport(), not from an external package (or at least, not long term). Spawning external process just for that seems a waste, considering that Node is supposed to have been built against the libc and should thus have the information available already.

@zkochan
Copy link

zkochan commented Jan 12, 2022

cc @Jarred-Sumner, I know he did some optimizations around optional dependencies in Bun

@Jarred-Sumner
Copy link

cc @Jarred-Sumner, I know he did some optimizations around optional dependencies in Bun

Bun skips postinstall while retaining compatibility with native dependencies that have binary executables by allowlisting some packages to have the first dependency matching os/cpu constraints to become the source where "bin" is linked from. If you install esbuild on macOS x64, then the "bin" field in esbuild reads from esbuild-darwin-64's folder instead of esbuild. This is currently enabled by default for turbo & esbuild, but can be enabled for other packages with the --link-native-bins packageName flag. This also disables skipping optionalDependencies for the package (though it skips downloading tarballs if the platform doesn't match). esbuild & turbo's postinstall work by overwriting the package's executable with the platform-specific binary

Giving some packages special treatment is not great, but there currently is no way to specify this behavior in package.json or in the registry API response. This approach also only works with native executables, not native bindings

Instead of that, here is what I would propose

"nativeDependencies" field in package.json where the keys are something like target triples.

{
  "name": "@napi-rs/canvas",
  "version": "0.1.6",
  "nativeDependencies": {
    "linux-x64-gnu": {
        "@napi-rs/canvas-linux-x64-gnu": "0.1.6"
    },
    "linux-x64-musl": {
        "@napi-rs/canvas-linux-x64-musl": "0.1.6"
    },
    "darwin-x64" : {
        "@napi-rs/canvas-darwin-x64": "0.1.6"
    },
    "win32-x64" : {
        "@napi-rs/canvas-win32-x64": "0.1.6"
    },
    "linux-aarch64-gnu": {
        "@napi-rs/canvas-linux-aarch64-gnu": "0.1.6"
    },
    "linux-aarch64-musl": {
        "@napi-rs/canvas-linux-aarch64-musl": "0.1.6"
    }
  }
}

This would tell package managers they need to install the most specific matching package.

On Ubuntu 18.04 AMD64, a package manager would try:

  • linux-x64-gnu
  • linux-x64
  • linux

Ideally, these dependencies would not be allowed to have their own dependencies. That would enable package managers to improve installation times by skipping extra registry API metadata requests when the exact version of the native dependency is specified. This means adding additional native targets won't impact package installation time.

@Brooooooklyn
Copy link
Author

"nativeDependencies" field in package.json where the keys are something like target triples.

I like this idea.

Ideally, these dependencies would not be allowed to have their own dependencies.

And this.

I think we can design a different installation logic for native packages than for normal npm packages, considering that native packages are platform-related.

The best solution I can imagine is:

  • For native packages authors, put all binaries into single npm packages. This will reduce a lot of works in CI for moving binaries cross build matrix.
  • For users, package managers will partially download the content of the published native npm package, by detecting the target triple on the installation host or config (for cross-compiling).

I think this RRFC should be turned into add nativeDependencies for prebuilt native packages rather than add libc fields

What do you think?

@arcanis
Copy link

arcanis commented Jan 14, 2022

There's already an open RFC discussing this feature, here:

It's still waiting for npm to join the discussion there, and I'm not interested to contribute to another RFC around this topic until they do.

The libc field is small enough that it requires little work and formal consensus, so that's what we're going to implement in the meantime.

@styfle
Copy link

styfle commented Jan 14, 2022

"nativeDependencies" field in package.json where the keys are something like target triples.

What if none of these match the current platform? Can it fallback to a wasm variant and how would you define that?

The libc field is small enough that it requires little work and formal consensus, so that's what we're going to implement in the meantime.

I'm happy with that for now! Its the least amount of changes to solve the original issue 👍
Is there a Yarn PR we can follow along?

@arcanis
Copy link

arcanis commented Jan 14, 2022

Is there a Yarn PR we can follow along?

Yes, it would be

@Brooooooklyn
Copy link
Author

What if none of these match the current platform? Can it fallback to a wasm variant and how would you define that?

We can define platform triple like wasm32

@ruyadorno
Copy link
Contributor

Relates to: #519

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Feb 9, 2022
@ruyadorno
Copy link
Contributor

Action item from our OpenRFC call: Let's follow up with the idea laid out in this RRFC and work on a sep RFC that can encompass more things that can possibly affect whether an optional dependency gets installed or not. This new RFC can complement the work from #519 and provide a better package distributions experience.

@lukekarrys lukekarrys added the Agenda will be discussed at the Open RFC call label Jul 12, 2022
@darcyclarke
Copy link
Contributor

@styfle lets do it. We can chat about this tomorrow (ie. in the next RFC call) but I think this is something we can agree on given the previous implementations.

@styfle
Copy link

styfle commented Jul 13, 2022

Sounds good! I watched the recording and seems like there was no opposition 👍

https://youtu.be/UDftNDbx0nM?t=935

@styfle
Copy link

styfle commented Oct 28, 2022

@darcyclarke Is anyone implementing this feature? Whats the roadmap look like?

@styfle
Copy link

styfle commented Jul 24, 2023

I just confirmed this was fixed in npm/npm-install-checks#54 and npm/npm-install-checks#57 which landed in npm@9.6.5, so I think this can be closed

Update, seems to be not working.

@lovell
Copy link

lovell commented Oct 9, 2023

Has anyone confirmed that this feature is working as intended for people using the latest npm CLI, or are there further code or dependency updates required?

A popular package that has taken advantage of the new libc field is @swc/core, which defines optionalDependencies on @swc/core-linux-x64-musl and @swc/core-linux-x64-gnu (amongst others).

However the latest version of npm appears to be installing both the glibc and musl variants on Linux regardless of libc.

musl
$ docker run -it --rm node:20-alpine /bin/sh

# npm install -g npm@latest
# npm -v
10.2.0

# npm install @swc/core
added 5 packages in 11s

# npm ls --depth 1
`-- @swc/core@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-darwin-arm64@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-darwin-x64@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-linux-arm-gnueabihf@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-linux-arm64-gnu@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-linux-arm64-musl@1.3.92
  +-- @swc/core-linux-x64-gnu@1.3.92
  +-- @swc/core-linux-x64-musl@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-win32-arm64-msvc@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-win32-ia32-msvc@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-win32-x64-msvc@1.3.92
  +-- @swc/counter@0.1.2
  +-- UNMET OPTIONAL DEPENDENCY @swc/helpers@^0.5.0
  `-- @swc/types@0.1.5

# ls -al node_modules/@swc/
total 28
drwxr-xr-x    7 root     root          4096 Oct  9 19:53 .
drwxr-xr-x    3 root     root          4096 Oct  9 19:53 ..
drwxr-xr-x    3 root     root          4096 Oct  9 19:52 core
drwxr-xr-x    2 root     root          4096 Oct  9 19:52 core-linux-x64-gnu
drwxr-xr-x    2 root     root          4096 Oct  9 19:52 core-linux-x64-musl
drwxr-xr-x    2 root     root          4096 Oct  9 19:52 counter
drwxr-xr-x    2 root     root          4096 Oct  9 19:52 types
glibc
$ docker run -it --rm node:20-bullseye /bin/sh

# npm install -g npm@latest
# npm -v
10.2.0

# npm install @swc/core
added 5 packages in 9s

# npm ls --depth 1
`-- @swc/core@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-darwin-arm64@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-darwin-x64@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-linux-arm-gnueabihf@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-linux-arm64-gnu@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-linux-arm64-musl@1.3.92
  +-- @swc/core-linux-x64-gnu@1.3.92
  +-- @swc/core-linux-x64-musl@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-win32-arm64-msvc@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-win32-ia32-msvc@1.3.92
  +-- UNMET OPTIONAL DEPENDENCY @swc/core-win32-x64-msvc@1.3.92
  +-- @swc/counter@0.1.2
  +-- UNMET OPTIONAL DEPENDENCY @swc/helpers@^0.5.0
  `-- @swc/types@0.1.5

# ls -al node_modules/@swc/
total 28
drwxr-xr-x 7 root root 4096 Oct  9 20:09 .
drwxr-xr-x 3 root root 4096 Oct  9 20:09 ..
drwxr-xr-x 3 root root 4096 Oct  9 20:09 core
drwxr-xr-x 2 root root 4096 Oct  9 20:09 core-linux-x64-gnu
drwxr-xr-x 2 root root 4096 Oct  9 20:09 core-linux-x64-musl
drwxr-xr-x 2 root root 4096 Oct  9 20:09 counter
drwxr-xr-x 2 root root 4096 Oct  9 20:09 types

I really want to take advantage of this feature so hopefully this is a case of me missing something obvious.

@wraithgar
Copy link
Member

libc is not overridable yet, only cpu and os.

This PR is where the libc override is happening. The underlying dependency supports it and landed in latest today, the PR now needs to rebase and drop the deps update and it should land. npm/cli#6817

@styfle
Copy link

styfle commented Jan 4, 2024

@wraithgar I think we should reopen this issue.

The original post says that the libc field should be used to avoid postinstall by only installing the correct optionalDendencies.

I just tried the latest npm and latest sharp and it is still installing both musl and gnu binaries but alpine should only install musl binaries.

Dockerfile

FROM node:20-alpine3.18

RUN mkdir /app
WORKDIR /app
RUN npm i -g npm@10.2.5
RUN npm add sharp@0.33.1
RUN ls -lA node_modules/@img

build command

docker build -t example --progress=plain --no-cache .

output

drwxr-xr-x    3 root     root          4096 Jan  4 18:27 sharp-libvips-linux-arm64
drwxr-xr-x    3 root     root          4096 Jan  4 18:27 sharp-libvips-linuxmusl-arm64
drwxr-xr-x    3 root     root          4096 Jan  4 18:27 sharp-linux-arm64
drwxr-xr-x    3 root     root          4096 Jan  4 18:27 sharp-linuxmusl-arm64

@styfle
Copy link

styfle commented Jan 10, 2024

@styfle
Copy link

styfle commented Jan 24, 2024

I confirmed that it is fixed in npm@10.4.0 🎉

styfle added a commit to styfle/packagephobia that referenced this issue Jan 24, 2024
Bumps to [`npm@10.4.0`](https://github.com/npm/cli/releases/tag/v10.4.0)
which adds a fix for [arborist](npm/cli#7126)
which will avoid installing multiple libc binaries and [correctly select
glibc vs musl](npm/rfcs#438) from the
optionalDependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

13 participants