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

doc: add policy for executables #51994

Closed
wants to merge 7 commits into from

Conversation

GeoffreyBooth
Copy link
Member

Following up #51918, this PR tries to define a policy regarding what commands the Node installation adds to a user’s PATH; currently it adds node, npm, npx and corepack. This PR would define a policy around potentially adding any more.

I wanted to draft what I thought would be the consensus or majority view regarding this, but I honestly don’t know what that would be in this case. So rather than take a guess at what I think would win approval, I just wrote what could fairly describe the status quo. The rule in this PR allows what Node currently provides, but it would prohibit #51886 or #51931. Maybe that’s what we want, but I can easily see us deciding that no, we do want a yarn executable shipped with Node that either uses built-in Corepack to install Yarn (#51886) or downloads Corepack and then uses Corepack to install Yarn (#51931). If so, though, what would a policy look like to allow that?

In either case, we’re probably stuck indefinitely with whatever executables we add, since it would be very disruptive to someday remove them, even as a breaking change (see npm).

@nodejs/tsc @nodejs/corepack @nodejs/npm @nodejs/package-maintenance

@GeoffreyBooth GeoffreyBooth added doc Issues and PRs related to the documentations. npm Issues and PRs related to the npm client dependency or the npm registry. labels Mar 6, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@richardlau
Copy link
Member

I'll read the proposed text tomorrow (a bit late here) but for some additional historical context, here's the discussion when npx was added: #13640

@GeoffreyBooth
Copy link
Member Author

for some additional historical context, here’s the discussion when npx was added: #13640

A major concern there seemed to be whether npx was already registered in various package manager registries. It wasn’t, so that turned out to be nothing to worry about; however in both yarn and pnpm‘s cases they are commonly registered. The installation docs for pnpm list five of them.

MoLow
MoLow previously requested changes Mar 7, 2024
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

IMHO this does not describe the status quo.
running code that is downloaded after installation is the essence of npm, corepack and npx.
in the case of npx that is almost the only thing it does.
even node -e "import('http://somthing/a.js')" runs downloaded code.

@GeoffreyBooth
Copy link
Member Author

@MoLow I took out the discussion of downloading and found a different way to phrase it. What do you think of this?

@wesleytodd I think this should be clearer?

@MoLow
Copy link
Member

MoLow commented Mar 7, 2024

@GeoffreyBooth this now kind of states the obvious. read this sentence without the erased part:

The distribution will only include executables whose names refer to software that is vendored within the Node.js distribution.

@MoLow
Copy link
Member

MoLow commented Mar 7, 2024

I think this PR focuses too much on the implementation details instead of the problems the distribution policy tries to solve.
I cant come up with a better suggestion though, I do have to admit

@MoLow MoLow dismissed their stale review March 7, 2024 16:38

reworded

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Mar 7, 2024

this now kind of states the obvious. read this sentence without the erased part:

Well but the erased part is kind of the point 😄 As I wrote in the top post, my intent with the proposed initial language is to define a policy that could reflect the status quo. We don’t currently ship a yarn executable that isn’t really Yarn, but rather a placeholder to download and install Yarn. If the status quo is what we want, so therefore we would prevent such placeholders from being created (either by enabling Corepack or my suggested alternative) then what I’ve written here hopefully expresses that. I’ve also struggled with the wording so I’m very open to improvements.

And again, I’m not really sure that this is what either I or we want. I opened this PR to spark the discussion to try to reach a consensus on how we feel about “placeholder” executables such as the yarn example I just mentioned. The exact wording of this PR only really matters if the status quo is what we want; if we decide we want something else, then the current wording won’t land, and so it doesn’t matter how well written it is.

@MoLow
Copy link
Member

MoLow commented Mar 7, 2024

We don’t currently ship a yarn executable that isn’t really Yarn, but rather a placeholder to download and install Yarn. If the status quo is what we want, so therefore we would prevent such placeholders from being created (either by enabling Corepack or my suggested alternative) then what I’ve written here hopefully expresses that. I’ve also struggled with the wording so I’m very open to improvements.

all this is an implementation detail. As a user, if it looks like yarn and sounds like yarn I consider that I have yarn on my machine. the existence of a jumper binary is just an abstraction that not many people are aware of.

@GeoffreyBooth
Copy link
Member Author

all this is an implementation detail. As a user, if it looks like yarn and sounds like yarn I consider that I have yarn on my machine.

Agreed. What I’m trying to say in this text is that we aren’t shipping things in our distribution that look like yarn and sound like yarn but aren’t yarn.

I’m not trying to argue that that should be our policy, just that if there were to be a policy to lock in the status quo, here’s what it would be. I think if there seems to be consensus that this is what we want our policy to be, we can continue wordsmithing this; but until then, perhaps let’s focus our discussion on what policy we want? @MoLow is what I’ve described here the policy that you think we want, regardless of how much better it could possibly be expressed?

@MoLow
Copy link
Member

MoLow commented Mar 7, 2024

I’m not trying to argue that that should be our policy, just that if there were to be a policy to lock in the status quo, here’s what it would be. I think if there seems to be consensus that this is what we want our policy to be, we can continue wordsmithing this; but until then, perhaps let’s focus our discussion on what policy we want? @MoLow is what I’ve described here the policy that you think we want, regardless of how much better it could possibly be expressed?

the thing is the status quo didn't necessarily take shape by a well-defined policy, it is more a sequence of events, wich is why it is so hard to come up with the current policy and wanted changes to it.
I would maybe phrase the situation as

The Node.js distribution includes several executables, such as `node`, that
are intended to be run directly by users and are vendored into the distribution.
Some other executables that are not vendored directly by Node.js are available
for execution via thin wrappers that obtain them lazily

what I am trying to say is we currently have a situation we (maybe?) want to document, not a policy

@GeoffreyBooth
Copy link
Member Author

But “Some other executables that are not vendored directly by Node.js are available for execution via thin wrappers that obtain them lazily” is not true today, is it? That’s what the “enable Corepack” or alternative PR would create.

Or are you saying that this is what you propose the policy should be, to enable one or the other of those PRs?

@MoLow
Copy link
Member

MoLow commented Mar 7, 2024

I agree, that isn't the current policy.
as I said, generalizing the current situation into a concrete policy feels like squaring the circle to me

@GeoffreyBooth
Copy link
Member Author

So I’ve been thinking about this, and about:

Where we added Yarn 1 to our Docker image some years ago. I think the experience here is instructive: it’s a breaking change now for us to either remove Yarn from that image or to make the yarn command map to Corepack/latest Yarn. And that sucks.

Multiply this by however many tools we might want to provide stubs for, and the potential similar issues that might arise for each one:

  • Say pnpm decides that they want to have the pnpm stub no longer point at Corepack, but at pnpm’s installer script. Do we make that change? As semver-major? What about users who prefer the old way?
  • Say pnpm releases a new major version. Should the stub download that new version immediately, even though the Node version hasn’t changed, or should the stub keep downloading the same major version of pnpm until the next Node major?

And so on. The more I think about it, the more that I think that what’s proposed in this PR should be our policy (regardless of whether or not it reflects the status quo or how we got here, which don’t matter). I just don’t think we want to be in the business of managing all the potential breaking changes or security issues of the projects we provide stubs for. It’s very easy for users to install these other projects, whether via npm install or npx corepack or other methods; obviously a stub would be even easier, but I don’t think the management headaches that we would need to take on are worth it to provide this convenience to users.

@aduh95
Copy link
Contributor

aduh95 commented Mar 11, 2024

it’s a breaking change now for us to […] make the yarn command map to Corepack

Is it? It shouldn't

@GeoffreyBooth
Copy link
Member Author

Is it? It shouldn’t

Why wouldn’t it be? Currently Yarn is baked into the image. Mapping it to Corepack instead is a breaking change because:

  • Users would have to add the packageManager field to ensure that Corepack downloads Yarn 1, I assume? Though if the enabled-by-default Corepack automatically installs Yarn 1 then that’s it’s own issue, in that it would be a breaking change to have that start automatically installing the latest Yarn.
  • The CI would need to expect and enable the package manager to be dynamically downloaded, rather than assuming that it’s baked into the image. It’s very possible that users’ CI might be configured to download only from internal sources, like a company’s internal npm registry, and not allow downloading code from the open internet.

But regardless, even if somehow this example isn’t a breaking change, I think we can agree that breaking changes along these lines are likely if not inevitable if we get into the business of shipping placeholders for external projects.

@GeoffreyBooth GeoffreyBooth added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Mar 11, 2024
@lukekarrys
Copy link
Member

I think the experience here is instructive: it’s a breaking change now

The npm team has experienced this as well. npm is a dependency of Node.js but since it is fully exposed to a user, any breaking change of npm could be considered a breaking change for Node as well.

In order to not force npm major versions to be coupled with Node major versions (even though that is a future goal), we came up with the following guidelines for which npm breaking changes should be considered Node breaking changes: https://github.com/npm/cli/wiki/Integrating-with-node. We also opened issues in nodejs/Release for npm 9 and npm 10 to gather feedback on our planned breaking changes before releasing them.

I wanted to point all this out to show 1) that there is some prior art here and 2) documenting a policy about how breaking changes are treated in exposed dependencies is important.

@arcanis
Copy link
Contributor

arcanis commented Mar 12, 2024

That may well be the case for npm, but that's mostly because projects managed by npm aren't version-locked on npm itself. It's one of the very things that Corepack addresses, by locking package manager versions on a by-project basis.

@lukekarrys
Copy link
Member

That may well be the case for npm, but that's mostly because projects managed by npm aren't version-locked on npm itself.

I think this is the case for npm because it is bundled. Even if npm had similar behavior as Corepack, as long as it were possible to use npm without locking the version, then updating to a new major version of npm within Node could be considered a breaking change.

I should have clarified above, I don't think binaries exposed through Corepack should be subject to the same breaking change policy. Only that a policy for executables in Node should include a discussion about breaking changes in those executables. And I think the specific case of Yarn 1 in an official Docker image falls under this example.

@bnb
Copy link
Contributor

bnb commented Mar 13, 2024

@GeoffreyBooth you indirectly / unintentionally bring up a good point: despite its usage, as far as I understand it corepack is directly installing software that's technically EOL and code using it is "legacy code" that should be moved off of (yarnpkg/yarn#8583 (comment), second result on Google for yarn EOL). At one point, I had to actually block a major corporation from publicly forking Yarn v1 because of this, instead getting them to work with package mangers directly.

Putting aside any judgement on that one way or another, a guide like this should probably also include verbiage around what's okay to ship and our expectations of support for what we're shipping. If the stance of corepack is that Yarn v1 - EOL software by every measure - should be supported and we generally agree with that, this document should include reasoning behind that stance.

@aduh95
Copy link
Contributor

aduh95 commented Mar 13, 2024

Calling Yarn v1 EOL will certainly derail this conversation, FWIW the last release of Yarn v1 is 1.22.22, published 3 days ago.

corepack is directly installing software that's technically EOL

I mean, like npm, Corepack will install what the user asked for, including versions which are EOL (as long as they are still on the registry).

@bnb
Copy link
Contributor

bnb commented Mar 13, 2024

Calling Yarn v1 EOL will certainly derail this conversation, FWIW the last release of Yarn v1 is 1.22.22, published 3 days ago.

the README literally tells you to migrate to berry and says the code is "mostly kept for historical purposes". This is also restated multiple times throughout issues over the span of years, and is shown by the release log - 4 days, 4 months, 2 years for the three most recent patch versions. I don't feel like that's controversial, and I'm not trying to posit as such - the project effectively says it ¯\_(ツ)_/¯

I mean, like npm, Corepack will install what the user asked for, including versions which are EOL (as long as they are still on the registry).

$ corepack enable
$ yarn -v
1.22.22

I didn't ask for anything, this is the default.

Again, not trying to stir anything up with this - I just specifically think that this document should outline why we're not following the package's own advice and using the latest thing that they recommend users migrate to. I have an understanding of why some people choose Yarn v1 over newer tooling from the project, but I think if we're making that decision for people we should document that choice here.

@aduh95
Copy link
Contributor

aduh95 commented Mar 13, 2024

I didn't ask for anything, this is the default.

It is the default, just like npx yarn -v would also return 1.22.22. We can discuss changing those defaults, but worth noting they were picked by the maintainers of Yarn.

@GeoffreyBooth
Copy link
Member Author

I just specifically think that this document should outline why we're not following the package's own advice

The current proposal is to forbid such executables, so unless we decide on something else then this isn't something we need to address.

@aduh95 @joyeecheung I reworded per your notes, please let me know what you think of this.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I think this is too strong. I care about reproducibility of the execution environment, not if the software is downloaded on the fly or not.
This already happens for Node.js http headers and node-gyp.

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Mar 13, 2024

I think this is too strong. I care about reproducibility of the execution environment, not if the software is downloaded on the fly or not.

What would you propose instead?

This PR doesn’t really have anything to do with reproducibility. One can obviously have executables that download pinned versions of tools, and that would provide reproducibility. We can also have reproducibility today without adding these placeholder executables. I just don’t see how the reproducibility question is relevant.

This already happens for Node.js http headers and node-gyp.

I don’t understand this. We don’t ship a headers executable that I’m aware of. We also don’t ship a node-gyp executable as far as I’m aware; running node-gyp or which node-gyp in the Node Docker image return errors. Neither of these tools would be affected by the policy proposed in this PR.

@GeoffreyBooth
Copy link
Member Author

Closing in favor of #52107

@GeoffreyBooth GeoffreyBooth deleted the executable-policy branch March 15, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. npm Issues and PRs related to the npm client dependency or the npm registry. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.