-
Notifications
You must be signed in to change notification settings - Fork 30k
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 “placeholder” executables #52107
doc: add policy for “placeholder” executables #52107
Conversation
Review requested:
|
doc/contributing/distribution.md
Outdated
|
||
## Placeholder executables | ||
|
||
Installing Node.js will not create "placeholder" executables: commands that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wording is pretty vague. npx
can be considered to be a placeholder executable according to this definition or at least some interpretations of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't see that, especially with the examples. The npx executable runs npx, which is included in our distribution. Npx itself isn't getting downloaded, it's downloading other things. Likewise for npm and Corepack (when run via the corepack
executable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to be clear, that means this policy would not block use from shipping a corepack_yarn
executable, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would block that. The policy says “refers to,” not “shares the exact same name as.” corepack_yarn
refers to Yarn, just as download_yarn
or download_and_install_yarn
refer to Yarn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does npx
fits into this policy then? npx yarn
downloads and executes Yarn, same as corepack_yarn
would.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For npx yarn
the executable is npx
, which doesn’t refer to Yarn. The text says “Installing Node.js will not create ‘placeholder’ executables;” Installing Node doesn’t create an npx yarn
executable, it creates an npx
executable. You may use that executable to run a command to download and install Yarn, and that’s fine, but the executable itself doesn’t refer to Yarn. Installing Node creates a node
executable too and there’s some node --eval
command that can download and install Yarn, but I’m not intending to ban the node
executable either.
I understand you’re looking for loopholes, and perhaps the language can be improved, but the intent of the PR is very clearly laid out in the description. If you want to suggest alternative language that is clearer or stronger, that’s fine, but as I said in the TSC meeting that introduced this PR, the point is less about the specific language then the fact that we reach an agreement about placeholder executables. If we can reach such a consensus, then we can move on to nail down the best possible language to express that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer we explicitly explain the issues we have with alternative npm clients (as they are described in #52107 (comment): Security issues, breaking changes) instead of a vague policy that doesn't address the real issues and might prevent us from doing things in the future
Not at all, one of the reason Corepack came to be is precisely so each projects can follow their own release cycle without affecting the other one.
Corepack already downloads the latest version by default (unless a specific version is requested) – the caveat for Yarn is that it would download the latest Yarn 1.x version. If that were to change, and
I think everyone agrees with that, but unfortunately that ship has long sailed since Node.js default includes npm, and that's unlikely to change anytime soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My read of this PR is that it goes against the TSC vote that decided that Yarn will be included via Corepack (nodejs/TSC#1012 (comment)).
As I wrote above, all it would take for us to change our minds is another PR to update this document. Nothing is ever set in stone, including that 2021 vote.
This was addressed in node/doc/contributing/technical-priorities.md Lines 169 to 172 in 454d080
And your comment proves my point: if you’re going to point to npm as precedent to justify adding Yarn and pnpm, then others will surely point to the inclusion of Yarn and pnpm to justify future additions, whether of package managers or other tools. This isn’t a debate we want to keep having. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a definite -1 on this pull request. If you want to proceed with this pull request, we should have a TSC vote.
That’s fine. Can you please explain your objection? What policy do you think we should have for placeholder executables? Or if you think we should have none, why? |
I think this is a strawman argument. I don't think the ship has sailed in any fashion. There is history regarding npm that cannot be compared to the current situation. Also one "wrong" does not make additional wrongs right. |
My general concerns/current thoughts include:
|
@aduh95 I would appreciate clarification on how this can be practically adopted. If one of those projects has a security issue, how can we not coordinate with them? Please elaborate also within the context of (#52107 (comment)):
|
How would coordinate with them achieve exactly? The project does not define what version users use, whatever is the latest on first use will be used (or the one defined in |
Exactly. How would that work?
I think here we have a fundamental disagreement. Which is why I asked you to elaborate in the context of #52107 (comment). While I agree with you in theory. In practice, I don't think that is how it will be perceived/expected by the users. We will be expected to be responsible for security issues in these projects, no matter whether or not we say that it's the users responsibility. When a user installs node, and starts using |
When the user first use the |
This comment was marked as off-topic.
This comment was marked as off-topic.
I won't comment the proposed doc changes. About the topic itself, I outlined my opinion here: |
I can think of only two ways that we could create a placeholder executable to download versions of Yarn:
Is there a third option, where somehow it can download the latest version’s checksum from somewhere in a secure way? If not, I think the only way that we can ship a placeholder executable with the same security guarantees as we provide for npm is via option 2, which means we’d be on the hook for security releases whenever vulnerabilities are discovered in Yarn. This also presupposes that we have some security around generating the hashes that we embed in our distribution, so that we aren’t inadvertently distributing the hash of a compromised download. |
We can use a Digital Signature Algorithm. |
Okay, but Corepack isn’t doing this currently? How would we ensure that whatever’s signing the release on the server side isn’t corrupted? |
Stupid question. But since we have decided to not remove npm. Can't corepack or whatever solution we decide on simply use npm for the security sensitive stuff, i.e. all that needs to be done is to make npm install the right package manager in the project |
Can we move that conversation to the Corepack repo? It doesn't seem to be related a "placeholder" executable policy. |
It would be good for us to reach a decision on this question both for Corepack and so that any potential alternative that the @nodejs/package-maintenance team designs knows what the potential constraints are. To those TSC members who haven't weighed in yet, is there anything else you need to reach a decision? Any more information, or another discussion? We aren't reaching a consensus on this one, so we'll need to vote; but ideally everyone will be as informed as possible before voting, and we don't vote until everyone is ready. |
We are putting together a session for the collab summit next week. Hopefully it is not too late 🤞, but there are a few things which need to be decided on:
I opened the issue quickly just now, but was thinking I would open a PMWG issue to discuss the agenda in more detail and then update it from there. |
@aduh95 and @anonrig, are there any steps that can be taken to resolve your objections to this PR?
I don’t really see how this PR is in conflict with the vote from 2021. That vote added Corepack but didn’t add any placeholder executables (what enabling Corepack would create) and in the discussion before the vote I see mentions of how the initial Corepack PR did create the placeholder executables but those were removed (changed to created on Regardless, votes aren’t binding forever and three years is a long time. We’re entirely within our rights to reevaluate old votes and old decisions, and we should, as circumstances change. So even if that vote directly addressed this topic, I don’t think “because we voted on this” is a valid objection, unless the vote just happened recently. Do you have a different objection, that we could try to resolve by consensus?
I don’t find this comment very collaborative or in the spirit of consensus seeking and I asked above for clarification and it’s been over a week with no reply. What is your objection that makes you a -1 on this pull request? Per our collaborator guide:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to generally prohibit placeholders. There could be ways to include them under very specific circumstances. Since we likely need a vote to include such placeholder anyway, I am against prohibiting it in general. I would be fine to say any placeholder addition requires a TSC vote and maybe even further requirements similar to the ones @mhdawson mentioned.
@BridgeAR I’d like to try to resolve this objection. As stated in the top post, my goal with this PR is to try to resolve all the outstanding Corepack decisions, so this could be written to prohibit placeholder executables specifically for package managers and stop there. That would leave for another day the possibility of placeholders for other use cases; personally I think they should always be prohibited, but for today we don’t need to go that far until someone actually proposes some other use case in the future. If I added text to that effect, and also that any placeholders (for any purpose?) need a TSC vote to be added, would that satisfy your concern? Something like this?
@MoLow @aduh95 I tried to address your comments as well, please let me know what you think of this. |
Instead of going directly for core pack, we tried to reduce the problem into multiple small problems, but the decision of this pull-request defines the future of core pack which I don't think we should do. This change is hiding the fact that we are actually determining the future of corepack within node.js. I believe that this decision is too big of a change that it is as equal as important as making a decision towards removing core pack. I'd like to have an official voting for this decision rather than approving a pull request and landing it with the current state. We saw how the previous pull request was interpreted by people. The decision to vote core pack is long overdue and further delaying it is causing exhaustion and burning out all stake holders of these discussions. Regarding core pack discussions: If we go ahead and remove |
I think it’s inevitable that there will be votes, either for this PR or the “remove Corepack” one or both, but before this one gets voted on I’d like to try to make it as good as it can be, and to come as close to consensus as I can get.
I proposed this in nodejs/package-maintenance#591 (comment) and openjs-foundation/summit#400 (comment). It will be part of next week’s collab summit version management working session. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 as a generic statement on this.
addressed note
I am still -1 on this change. although it's less vague I don't agree it reflects the current or the desired situation |
What do you feel is the desired situation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still against this. I don't think we should generally prohibit any specific kind of placeholders. I left a suggestion that I believe could be used instead.
Co-authored-by: Ruben Bridgewater <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to not include a new process, especially this document does not feel the right place to document that. Instead, I propose we take advantage of the existing process: semver-major PRs require at least 2 approval from TSC voting members.
without a very strong reason to do so. It therefore requires an affirmative vote | ||
of the Node.js Technical Steering Committee to include any such placeholder | ||
executables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without a very strong reason to do so. It therefore requires an affirmative vote | |
of the Node.js Technical Steering Committee to include any such placeholder | |
executables. | |
without a very strong reason to do so. Including such placeholder executable is therefore considered a semver-major change. |
It doesn’t seem like this will reach consensus. Rather than force a vote on it, I think a more productive use of time will be to engage with the package maintenance team and see if that approach produces a favorable outcome. We can come back to this if needed. |
I don’t think we want our distribution to include or create upon installation “placeholder” executables: a command like
yarn
that downloads and installs Yarn when run. This is for the following reasons:It’s already very easy for users to install CLI tools, whether via
npm install
orcurl
or the command of their choice.Providing placeholders puts us arguably on the hook for any security issues contained within the placeholder. Even if we have some fine print somewhere saying that somehow we’re not responsible for any vulnerabilities within the Yarn software that our
yarn
command downloads and installs, I think many users would understandably argue that that doesn’t absolve us: that we should provide the same security guarantees for Yarn that we do for npm, even if Yarn isn’t actually bundled within our distribution.Any placeholder we provide would arguably need to follow the same rules for breaking changes that we currently follow for npm. So if a
yarn
placeholder currently downloads Yarn 1, it can’t be changed to download Yarn 4 until the next major release of Node. That would mean that Yarn would need to coordinate their major releases with us the way npm does, to minimize lag time; and it would also mean that we can’t ship a placeholder that simply always downloads the latest version. Such “pinned” placeholders, whether pinned to an exact version or the latest version of a semver major line, are much less useful to users and provide worse UX than current methods for installing software.Placeholder executables arguably serve a “political” purpose: they imply a recommendation of the referenced software. Just as bundling npm arguably implies a recommendation of npm, shipping placeholders implies equivalent recommendations for those other tools. Obviously this is desirable for competitors to npm, but I don’t think this is a road we as a project want to go down: it’s much easier to state that we’re agnostic and don’t have recommendations for any tools, rather than having debates about every tool that someone proposes to create a placeholder for.
Landing this PR would mean that we’re deciding not to land #51886 or #51931 in the near term, and that any future efforts at either of the proposals in those PRs would require updating the text added by this PR with a new policy to permit the placeholder executables envisioned by either of those PRs.