javascript: Manage npm version with corepack #18748
Conversation
Requires explicit setting of 8.5.5 to be backwards compatible
| @@ -120,6 +120,22 @@ async def download_known_version( | |||
| download_file = DownloadFile(url, FileDigest(known_version.sha256, known_version.filesize)) | |||
| return await Get(DownloadedExternalTool, ExternalToolRequest(download_file, exe)) | |||
|
|
|||
| package_managers = DictOption[str]( | |||
| default={"npm": "8.5.5"}, | |||
There was a problem hiding this comment.
I would have preferred to set this to the empty dictionary, and let Corepacks own "knownGoodReleases.json" be in effect for npm by default.
Unfortunately that version (7.20.0) is not the same as the one that has been in use by pants so far (8.5.5), and downgrading a major version as a default might break stuff.
There was a problem hiding this comment.
Maybe we can drop this default once corepack updates their default to 8.5.5 or later.
| directory_digest = await Get(Digest, CreateDigest([Directory("._corepack")])) | ||
| binary_digest = binaries.digest if binaries.digest else EMPTY_DIGEST | ||
| input_digest = await Get(Digest, MergeDigests((directory_digest, binary_digest))) |
There was a problem hiding this comment.
There is an (unlikely) situation here that could break things. If a user uses a bootstrapped nodejs that doesnt ship with corepack (no LTS versions of this dist exists https://nodejs.dev/en/about/releases/, corepack is part of v14 dist), the "corepack enable" invocation on line 324 will surely not behave nicely.
I'm reluctant to implement support for versions <14 as that is past maintenance, but maybe better error messaging is warranted than a filenotfound error from the engine?
| binary_digest_with_shims = await add_corepack_shims_to_digest( | ||
| binaries, binary_shims, corepack_env_vars | ||
| ) |
There was a problem hiding this comment.
Corepack shims has to be part of the same digest as the downloaded node binaries, otherwise the symlinks are incredibly incorrect when passed as separate immutable digests.
| pkg_manager_env = { | ||
| "__PACKAGE_MANAGER_VERSIONS": ",".join(versions) | ||
| } # Invalidates cached process in event of update |
There was a problem hiding this comment.
Is this abuse of append_only_caches? I noticed processes were not re-run after I changed the npm version (which triggers a rerun of prepare_default_package_managers). It is kinda bad if a user is attempting an upgrade and tests/builds still pass.
There was a problem hiding this comment.
I think I've convinced myself that yes, this is very bad (and I think its the reason CI is failing). Because corepack mutates a goodKnownReleases.json file in this cache, the choice of version becomes "erratic". It doesnt matter if the consuming process is force to re-read, this is still (seemingly) racy.
tobni
force-pushed
the
add/manage-npm-version-with-corepack
branch
from
d8065d6 to
866248f
Compare
Corepack turned out to be stateful w.r.t version tracking
cognifloyd
added
category:new feature
backend: JavaScript
Enables users to configure a wanted version of a package manager blessed by Corepack, either by:
packageManagerentry in theirpackage.json, the feature of Corepack.This is a prerequisite for pnpm and yarn support.
Fixes #18525.