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

src: add glibcCompiler and glibcRuntime to process.versions #41338

Closed
wants to merge 3 commits into from

Conversation

styfle
Copy link
Member

@styfle styfle commented Dec 27, 2021

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Dec 27, 2021
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 For a couple of reasons:

  1. Just supporting glibc isn't very useful, especially within the original context of package installation. Adding support for other platforms makes things even more complicated (e.g. Windows). That is a lot to maintain in node core for little benefit IMO.
  2. process.versions is typically a static list of values, something where libc runtime information doesn't really fit in

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, I was just about to suggest that this could be added to the process.report feature, but then I saw that in #39877 that was actually already mentioned as something that exists :) No strong feelings about whether this should be implemented here, though, but I see @mscdex’s points.

src/node_metadata.cc Outdated Show resolved Hide resolved
src/node_metadata.cc Outdated Show resolved Hide resolved
styfle and others added 2 commits December 27, 2021 18:36
Co-authored-by: Anna Henningsen <github@addaleax.net>
@Brooooooklyn
Copy link

@mscdex So what's your suggestions here, where can we put this information into? The installation size is a big problem and we don't want to avoid it by postinstall scripts like vercel/next.js#32850 and Brooooooklyn/canvas#401

Just supporting glibc isn't very useful, especially within the original context of package installation.

But installation in the other platform doesn't have any problem. On macOS, Windows, and BSD, there is only one libc runtime.

For native addon packages, the only problem with distributing via optionalDependencies at the moment is that two different sets of libc runtime addons are downloaded repeatedly on Linux.

@devsnek
Copy link
Member

devsnek commented Dec 28, 2021

why does this need to be added here if it's currently in process.report.getReport()?

@Brooooooklyn
Copy link

@devsnek ok, I see, thanks for your explain!

@mscdex
Copy link
Contributor

mscdex commented Dec 28, 2021

@mscdex So what's your suggestions here, where can we put this information into? The installation size is a big problem and we don't want to avoid it by postinstall scripts like vercel/next.js#32850 and Brooooooklyn/canvas#401

My suggestion is that this doesn't need to belong in node core and is solvable in userland.

Additionally, the only real information you need here is the runtime libc information, not the compile-time libc information. Obtaining this information does not require anything from node core because it's a property of the entire OS and libraries like detect-libc show some of the possible avenues for finding the information you need.

While solving the issue of avoiding the need for build environments to install addons is important, I don't think there are any other use cases for obtaining libc information.

But installation in the other platform doesn't have any problem. On macOS, Windows, and BSD, there is only one libc runtime.

I'm not talking about just the name of the libc runtime, even obtaining libc runtime versions and other information properly is not always straightforward (especially on Windows).

For native addon packages, the only problem with distributing via optionalDependencies at the moment is that two different sets of libc runtime addons are downloaded repeatedly on Linux.

That seems like a design flaw with the packages in question that can easily be dealt with in userland. For example, I've recently created my own system for consuming prebuilt addons that downloads the appropriate binary on installation instead of including them in the module/package itself. This avoids the problem you're describing.

@Brooooooklyn
Copy link

That seems like a design flaw with the packages in question that can easily be dealt with in userland.

I don't like postinstall scripts because it is

  1. Unsafe.
  2. Binaries is uncontrolled from lockfile, which may break cache logic in many CI/CD platforms.
  3. And it also introduces useless dependencies to download binary.
  4. The binaries are not easy to download in many regions which don't have a stable network to connect the CDN where native addon binaries are stored.

Moreover, if npm/rfcs#488 is landed, the postinstall solution puts more effort on the user.

My suggestion is that this doesn't need to belong in node core and is solvable in userland.

Now the situation is npm/rfcs#488 is killing the postinstall scripts. And why are you still suggesting native packages authors solve this problem in userland via lifecycle scripts.

@mscdex
Copy link
Contributor

mscdex commented Dec 28, 2021

I don't like postinstall scripts because it is

Technically install, but ok....

  1. Unsafe.

Only if you are installing packages you don't trust. Trusting scripts is no different than trusting the dependency code you'll be using at runtime.

  1. Binaries is uncontrolled from lockfile, which may break cache logic in many CI/CD platforms.

Can you explain what you mean by this? A lock file would include the specific version of the module installed. If your addon loader/downloader is downloading version-specific binaries, there isn't an issue that I can see.

  1. And it also introduces useless dependencies to download binary.

I think this is far from being a common scenario. Most addons come with some kind of JS-level API that interfaces with the binding and don't just merely export (or set as "main" in package.json) the binding directly, so the dependency is not "useless." Additionally, the dependency should still have a source-based fallback (whether the source is included in the package or not) if a suitable binary is not available, making the dependency even less "useless".

  1. The binaries are not easy to download in many regions which don't have a stable network to connect the CDN where native addon binaries are stored.

I don't see this as any different than the solution you proposed where you would conditionally install another separate package from npm's registry based on the system's libc runtime. In the particular case of my solution, it's downloading from github rather than npm.

Now the situation is npm/rfcs#488 is killing the postinstall scripts. And why are you still suggesting native packages authors solve this problem in userland via lifecycle scripts.

Because it's the only sensible solution that's available. If scripts are disabled by default, then addon authors that utilize binaries will just have to amend their READMEs to include whatever relevant npm command line argument to re-enable scripts for the particular install. If npm ends up not providing such interfaces to re-enable (selectively or otherwise) scripts, or provide some appropriate alternative, then either people will have to resort to hacks like installing dependencies from binding.gyp for example or moving to an entirely different package manager that is more flexible.

Anyway, this particular discussion is diverging from the subject of this PR and is probably better discussed elsewhere? My point still stands that detecting libc runtimes is better suited for userland for the reasons I stated previously.

@styfle
Copy link
Member Author

styfle commented Dec 29, 2021

why does this need to be added here if it's currently in process.report.getReport()?

Oh good point! This already works today since Node.js 11:

process.report.getReport().header.glibcVersionRuntime

A better API would be exposing family: "glibc" or family: "musl" like the issue #39877 proposed.

I'm not sure if Node.js currently detects musl though so that would need to be implemented 🤔

@styfle styfle mentioned this pull request Jan 3, 2022
8 tasks
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request Jan 3, 2022
In Next.js [12.0.1](
https://packagephobia.com/result?p=next@12.0.1), musl support was added which caused linux to install both glibc and musl binaries.

This PR adds the `install` script to prevent installing unused binaries, reducing the install size by 47MB.

We originally thought this could be added to Node.js core and thus npm but [it was rejected](nodejs/node#41338).

Note getReport() works on Node.js [`>=11.8.0`](https://nodejs.org/api/process.html#processreportgetreporterr) which is safe to use since Next.js requires [`"node": ">=12.22.0"`](https://github.com/vercel/next.js/blob/265f71e225ed18fcb099c28cf1b5c83519acc3b0/packages/next/package.json#L280).
natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022
In Next.js [12.0.1](
https://packagephobia.com/result?p=next@12.0.1), musl support was added which caused linux to install both glibc and musl binaries.

This PR adds the `install` script to prevent installing unused binaries, reducing the install size by 47MB.

We originally thought this could be added to Node.js core and thus npm but [it was rejected](nodejs/node#41338).

Note getReport() works on Node.js [`>=11.8.0`](https://nodejs.org/api/process.html#processreportgetreporterr) which is safe to use since Next.js requires [`"node": ">=12.22.0"`](https://github.com/vercel/next.js/blob/265f71e225ed18fcb099c28cf1b5c83519acc3b0/packages/next/package.json#L280).
@bnoordhuis
Copy link
Member

#39877 has been closed so I'll go ahead and close this out as well.

Apropos detecting glibc at npm install time: seems like a broken approach to me. What if I rsync my project from a glibc system to a musl system? The try/catch solution in #39877 (comment) is the most robust.

I'm not sure if Node.js currently detects musl though so that would need to be implemented 🤔

@styfle musl (intentionally) has nothing to aid feature detection, its rationale being it "only implements standard functionality."

@bnoordhuis bnoordhuis closed this Jul 5, 2022
@styfle
Copy link
Member Author

styfle commented Jul 5, 2022

What if I rsync my project from a glibc system to a musl system?

You could make the same argument for "What If I rsync my project from macOS to Linux"? I don't think that scenario needs to be considered–its safe to say the platform you install and build on should also be the platform you run on.

That being said, the purpose of this PR was not so much to get glibc/musl detection into Node.js core–it was to get it supported in npm. However, the npm team suggested supporting in core so that all package mangers could reuse the same logic (there is a proposal for complex installation distributions that would benefit from this npm/rfcs#519).

Since both yarn and pnpm have implemented it in userland, it seems like npm could too without waiting for full blown distributions. Related comment: npm/rfcs#438 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime information about libc version and type
7 participants