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: bump Known Good Release when downloading new version #364

Merged
merged 4 commits into from
Jan 30, 2024
Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 28, 2024

When Corepack downloads a new version, it checks if it corresponds to same major as the global cached one, and if it does, it uses the more recent one the next time the global version is used. That should give users a more smoother experience as they won't need to call corepack install -g as often – with this PR, it should be needed only when wanting to use a different major, assuming you work on projects that keep their "packageManager" field up-to-date.

sources/Engine.ts Outdated Show resolved Hide resolved
@aduh95 aduh95 changed the title feat: bump Last Known Good when downloading new version feat: bump Known Good Release when downloading new version Jan 28, 2024
@@ -193,6 +196,29 @@ export async function installVersion(installTarget: string, locator: Locator, {s
}
}

if (process.env.COREPACK_DEFAULT_TO_LATEST !== `0`) {
let lastKnownGoodFile: FileHandle;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Not a fan of working with file handles; it makes the code harder to follow for something that's relatively trivial perf-wise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were reading and parsing the same JSON twice, so at least that should improve the perf ever so slightly. But I think it's the only way to avoid race conditions in case other processes are touching the same file.

@aduh95 aduh95 enabled auto-merge (squash) January 30, 2024 22:38
@aduh95 aduh95 merged commit a56c13b into main Jan 30, 2024
10 checks passed
@aduh95 aduh95 deleted the bump-lkg branch January 30, 2024 22:51
maxrake added a commit to phylum-dev/phylum-ci that referenced this pull request Feb 22, 2024
The lockfile generation tool `yarn` fails when used in the `phylum-ci`
Docker image as a non-root user. An example of the failure can be seen
from the output of the "smoke test," which is using
`scripts/docker_tests.sh` to ensure basic functionality:

```
yarn --version
Internal Error: EACCES: permission denied, open '/usr/local/corepack/lastKnownGood.json'
Error: EACCES: permission denied, open '/usr/local/corepack/lastKnownGood.json'
```

The same behavior happens for `pnpm`. These are the tools installed by
`corepack`, which changed recently to "Bump Known Good Release when
downloading new version" (nodejs/corepack#364).
Part of that change was to make use of the `COREPACK_DEFAULT_TO_LATEST`
environment variable to *not* update the last known good version, but
setting that to `0` does not appear to prevent *all* writes (or creating
a file handle with write permission) to the `lastKnownGood.json` file.

This fix simply modifies the file permissions for `lastKnownGood.json`
so that non-root users can read and write to it. This approach may seem
specific to a file that may change name or location in the future, but
the alternative method of adding `${COREPACK_HOME}` to the list of
directories that get updated with a `chmod -vR 777` was deemed to be too
blunt and therefore less desirable.
maxrake added a commit to phylum-dev/phylum-ci that referenced this pull request Feb 22, 2024
The lockfile generation tool `yarn` fails when used in the `phylum-ci`
Docker image as a non-root user. An example of the failure can be seen
from the output of the "smoke test," which is using
`scripts/docker_tests.sh` to ensure basic functionality:

```
yarn --version
Internal Error: EACCES: permission denied, open '/usr/local/corepack/lastKnownGood.json'
Error: EACCES: permission denied, open '/usr/local/corepack/lastKnownGood.json'
```

The same behavior happens for `pnpm`. These are the tools installed by
`corepack`, which changed recently to "Bump Known Good Release when
downloading new version" (nodejs/corepack#364).
Part of that change was to make use of the `COREPACK_DEFAULT_TO_LATEST`
environment variable to *not* update the last known good version, but
setting that to `0` does not appear to prevent *all* writes (or creating
a file handle with write permission) to the `lastKnownGood.json` file.

This fix simply modifies the file permissions for `lastKnownGood.json`
so that non-root users can read and write to it. This approach may seem
specific to a file that may change name or location in the future, but
the alternative method of adding `${COREPACK_HOME}` to the list of
directories that get updated with a `chmod -vR 777` was deemed to be too
blunt and therefore less desirable.

---------

Co-authored-by: Kyle Willmon <[email protected]>
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.

2 participants