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

feat: add proposal for binary management #594

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wesleytodd
Copy link
Member

Moved from #593. It should have been a PR but I just was doing that in the collab summit before the session and I didn't have the PMWG repo closed down at the time. Sorry about that.

@wesleytodd
Copy link
Member Author

Not sure if we need to copy this over to here: #593 (comment)

Closing that in favor of the PR version.

Comment on lines +142 to +150
packageManager: {
name: 'npm', // names which we would specify and map to individual known projects
version: '', // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity
resolved: {
version: '1.0.0',
uri: 'https://registry.url/path/to/tarball.tgz',
// this would be the output of ssri
digest: '...'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
packageManager: {
name: 'npm', // names which we would specify and map to individual known projects
version: '', // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity
resolved: {
version: '1.0.0',
uri: 'https://registry.url/path/to/tarball.tgz',
// this would be the output of ssri
digest: '...'
"packageManager": {
"name": "npm", // names which we would specify and map to individual known projects
"version": "", // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity
"resolved": {
"version": "1.0.0",
"uri": "https://registry.url/path/to/tarball.tgz",
// this would be the output of ssri
"digest": "sha1.c85a4305534f76d461407b59277b954bac97b5c4"

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the output from ssri can be more than just this string right? I didn't want to try to make it look like we were documenting the behavior of that package with this line, but if that is the output I am good with this change as a whole.

Copy link
Member

@styfle styfle Apr 4, 2024

Choose a reason for hiding this comment

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

digest was missing an example. I figured you had uri with a value and it could be anything so either add examples to all or none.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah, I see the dissonance in the example. I guess probably what we should do is take the output from ssri via a package-lock.json as an example.

name: 'npm', // names which we would specify and map to individual known projects
version: '', // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity
resolved: {
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid defining a custom addition to the devEngines spec? It sounds like what you’re creating here is a lockfile; but there are already lockfiles that accompany package.json files. I’m not sure we need a standardized lockfile, but if we do, that seems like it should be something proposed on its own?

Copy link
Member Author

@wesleytodd wesleytodd Apr 4, 2024

Choose a reason for hiding this comment

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

This is what I propose we make on the devEngines spec. The existing field already had changes requested in that issue which were not addressed, this attempts to address them. Specifically the name is changed to not suggest that the spec requires implementors to implement "download" functionality.

Copy link
Member

Choose a reason for hiding this comment

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

I'm unaware of requests for changes on that spec that haven't been addressed; but if there are, let's deal with notes over there rather than defining a competing alternative.

@GeoffreyBooth
Copy link
Member

This is a huge document that feels like something that would come out of a monthslong discovery process. Can we maybe mark this as draft for now and start with a new document that just defines the goals we want to address? Let’s get that written down and hopefully find consensus there, before moving on to various approaches and their pros and cons, etc.

One model we could follow is https://github.com/nodejs/loaders/tree/main/doc, where separate files cover prior art, external resources, etc. This PR could be split up into multiple files by topic and then some of them can land without waiting for the goals discussion, such as a doc listing prior art.

@wesleytodd wesleytodd marked this pull request as draft April 4, 2024 15:41
@wesleytodd
Copy link
Member Author

This is a huge document that feels like something that would come out of a monthslong discovery process. Can we maybe mark this as draft for now and start with a new document that just defines the goals we want to address?

Yeah I agree if is too long/much. And I did mark it as draft after reading this comment the first time yesterday. That said, I am not clear on what we get out of multiple documents. I agree this is not complete, but I think part of the problem that was burning folks out in the previous parts of this conversation was too many separate pr's/issues. As I can totally see it is too long, I am concerned about sharding the conversation again. Would it make you more comfortable if I deleted some parts for now (like some of the solutioning I did) and then we can focus for now on the prior art and goals sections but keep it in one doc?

@wesleytodd
Copy link
Member Author

One other thing to note which I was just reminded of by a comment in one of this repos issues, the intent is that with this proposal the PMWG takes over the scope of the old version management effort here: https://github.com/nodejs/version-management

@GeoffreyBooth
Copy link
Member

That said, I am not clear on what we get out of multiple documents.

It means that simple parts can just land quickly, like the list of prior art. It's also more useful to have separate files rather than one big one. If you look in the loaders repo for example, we have several design proposals in separate files. That's much easier to work with than everything in one file.

You don't need to do one PR per file. Maybe start with one PR for all the history stuff, prior art and links/resources, any other related topics that can be one file per topic. That should be easy to land without much controversy, and we can tackle goals in its own PR.

@wesleytodd
Copy link
Member Author

Ok, I can work later this week on breaking it up. That said, I would be happy to have comments on what is here before then because largely I have struggled to carve out the time to complete this, so when I get back to it if I can address many things at once it would be more efficient. Also, I don't need to be the person to do this work so if anyone wants to help move this along faster than I am able that would be awesome as well.

@GeoffreyBooth
Copy link
Member

I'm happy to help with the documentation work. My availability is also scattered so it'll be in dribs and drabs over the next few days/weeks.

I think the most important thing though is to start collecting a list of goals that either have consensus or majority support. Maybe we could start with the consensus ones and separately add ones that require voting as a later PR. Is the best way to do this via a PR or maybe a GitHub discussion?


This has led to a the Node.js project focusing on the well understood (but also highly complicated) aspect of simply supporting one locked version of the toolchain across the many
supported architectures. This is in fact a required area of investment for the project as no matter the local dev toolchain a team or developer chooses, when in prod it is *always*
necessary to support a secure and stable single version workflow.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessarily true. I've seen many production deployments use the latest Node 20.x, for example, bumping the minor automatically with each prod deployment. I think all AWS Lambda deployments function like this; I don't think you can choose the minor or patch version for a Lambda deployment. @styfle what is Vercel like in this regard?

Copy link
Member

@styfle styfle Apr 9, 2024

Choose a reason for hiding this comment

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

Correct, Vercel and AWS only support major versions of Node.js. Minor and patch are automatically upgraded.

Copy link
Member

Choose a reason for hiding this comment

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

That said, it could be desirable to select an exact version. I remember a couple times where upgrading Node.js minor broke deployments. In particular when the “type” filed was added to package.json and tooling didn’t know about it yet.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, of course it could be desirable. My point was just that we shouldn't assume that everyone is version pinning.

I think perhaps a better way to put it is that version pinning through lockfiles for code that becomes part of a deployment as a build artifact has become common, but version pinning for the runtime and other development tools has not; or more precisely, that version pinning for development dependencies is often at the semver major level only (Node 20.x). This reflects the importance of getting the latest security updates for a runtime at the expense of stability. That's the tradeoff, and I think the original npm install took the same approach (pinning only to a minor) but people moved away from it because too many dependencies weren't careful enough to not ship breaking changes in patch releases, but Node is generally careful enough and people aren't tied very closely to it where such pinning is often necessary to avoid breakage; and the cost of missing the latest security updates is high. Package managers are maybe somewhere in between; they really shouldn't need pinning at a level beyond major version; pnpm 8 is an unusual case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am a bit confused about what we are saying is not true. I said "we need to support" not "everyone must do it this way". My point was just that the debians and docker containers and all that are still required no matter if we ship a version manager to help with local and ci dev.

Copy link
Member

@GeoffreyBooth GeoffreyBooth Apr 9, 2024

Choose a reason for hiding this comment

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

I am a bit confused about what we are saying is not true.

This part:

in prod it is always necessary to support a secure and stable single version workflow.

sounds to me like you’re saying that the runtime, if not more, needs to be version-pinned in prod. Maybe you meant “pinned at the semver major level”? I read it as intending to mean pinned exactly.

## The Problem

A realistic and productive Node.js development environment cannot be setup with *only* tools shipped within the current executables. A version manager for both the `node`
executable and a package manager is required.
Copy link
Member

Choose a reason for hiding this comment

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

A version manager is certainly not a requirement, either for runtime or for package manager. It's much more common for runtime, but a common workflow is to develop locally using latest Node and test for a specific locked version using Docker or a nonprod hosted environment. That's how I've seen many development teams operate over many years.

In the last decade at work I've seen maybe three dozen or so Node app development teams and none of them have used a package manager version manager. All of those teams have used either npm or a handful used Yarn 1. From what I can tell, package manager version managers are really only popular for pnpm because of its bug in pnpm 8 where the lockfile changes often.

Also this section purports to define the problem but really it's describing a solution: one or more version managers. What problems are those tools solving?


@TODO list out more about the current version managers and their approaches.

## A Proposal
Copy link
Member

Choose a reason for hiding this comment

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

This proposal should become its own file, but I would suggest setting it aside until we agree on goals because it might change to address them.

name: 'npm', // names which we would specify and map to individual known projects
version: '', // this would be semantically left open from the spec, but should be considered the equivalent of npm package specifiers
// see: https://github.com/zkat/ssri/blob/latest/README.md#--ssriparsesri-opts%E2%80%94integrity
resolved: {
Copy link
Member

Choose a reason for hiding this comment

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

I'm unaware of requests for changes on that spec that haven't been addressed; but if there are, let's deal with notes over there rather than defining a competing alternative.

tool authors into one single direction. If they wanted to limit that `version` cannot be a git repository, that would be acceptable even though `npm` defines that as valid for a
package specifier.

## Technical Approachs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Technical Approachs
## Technical Approaches

@coreybutler
Copy link
Member

There is an inaccuracy in this under the technical details, or at least something that needs clarification. Most existing version managers do NOT manipulate the PATH for version switching. There are really only two strategies employed: shims and symlinks. The PATH is only used to identify the version manager binary, just like any other application.

nvm-sh points to a shim, meaning a single executable is found in the PATH. The shim is responsible for executing the node binary. nvm-windows points the PATH to a "directory", which is a symlink. The symlink target is modified when the version changes, so the native node executable is always called directly.

We tried PATH manipulation in an early version of NVM for Windows. It was a catastrophic failure. Windows can limit the size of the PATH, so changing it whenever a new version needs to be changed can truncate the PATH. If truncation occurred, at best user's other programs stopped working. At worst, the entire OS crashed.

@wesleytodd
Copy link
Member Author

Most existing version managers do NOT manipulate the PATH for version switching.

The most popular version manager nvm uses PATH manipulation. Maybe we have unclear language here?

nvm-sh points to a shim

I have it under very good authority that this is not the case, maybe you can explain a bit better what you mean here? do you mean the windows version and not what I linked above?

We tried PATH manipulation in an early version of NVM for Windows. It was a catastrophic failure.

I would love to hear more about this. Maybe you could join up on one of our meetings to share your experience with this?

@coreybutler
Copy link
Member

The most popular version manager nvm uses PATH manipulation.

I took a quick look at nvm's source and PATH is exported once in the use command, so it does appear to change the path whenever the version changes. I don't know if that's new or not (I'm sure Jordan could confirm).

It's worth noting that nvm-sh is the most popular from a Github star perspective, but don't make the mistake of assuming it represents a majority of users or VMs. From the limited data we have, none of the individual VMs have a majority share of the overall user base.

Unless I'm mistaken, other VMs (like volta and nodist) use a shim as I described. Tools like n use the symlink approach.

There's not too much more to say about switching paths on Windows. Some versions of Windows limit the character count of the PATH. Truncate it and the system becomes volatile. The volatility can be unpredictable. If you append the node path to the end, it may be truncated and node won't be recognized. Prefix it and you risk cutting off system resource routes Windows needs to operate. Newer versions don't have this limitation, but there are a lot of people using older versions of Windows. There is also more than one PATH on Windows (system and user). They require different levels of permissions to modify. Throw in GPO's and many organizations explicitly block system/user env variable modifications. It's a can of worms.

@wesleytodd
Copy link
Member Author

(I'm sure Jordan could confirm).

That is who messaged me about your comment :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants