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

Separate read and write operations on lastKnownGood.json #446

Merged
merged 2 commits into from
Apr 13, 2024

Conversation

das7pad
Copy link
Contributor

@das7pad das7pad commented Apr 1, 2024

Description

Closes #183
Closes #422

This PR is separating read and write operations on lastKnownGood.json. It will also skip overwriting lastKnownGood.json with the same content.

Review

This PR is best reviewed with white space changes ignored due to indentation changes from removing try/catch/finally blocks.

I refactored the read and write operations into self contained functions. The re-use of a r+ file handle does not work anymore when separating the two kinds of operations.

The content of lastKnownGood.json is validated/sanitized when reading. Any consumers can operate on the Record<string, string> object directly. This greatly simplified the logic in activatePackageManager and getLastKnownGoodFromFileContent.

I could not get the "per-project" install to work fully offline/read-only in a straightforward way (just running corepack install). Running corepack install only caches the requested binary from package.json -> packageManager. When invoking the package-manager (yarn) via corepack yarn, corepack tries to discover the latest version to prepare a fallbackLocator in Engine.executePackageManagerRequest first, but without a lastKnownGood.json (corepack install does not prepare it), it attempts a network request. The "code fix" here is to defer the fallbackLocator work until after discovering any package.json file. This is a major undertaking and something for another day. I left a note for this in a comment in the "per-project" test, next to the workaround: running corepack yarn --version once populates the lastKnownGood.json cache.

tests/main.test.ts Outdated Show resolved Hide resolved
das7pad and others added 2 commits April 13, 2024 13:22
Also skip overwriting lastKnownGood.json with same content.

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>
@aduh95 aduh95 force-pushed the separate-r-from-w branch from b069c20 to 759dc4d Compare April 13, 2024 11:26
@aduh95
Copy link
Contributor

aduh95 commented Apr 13, 2024

corepack tries to discover the latest version to prepare a fallbackLocator in Engine.executePackageManagerRequest first, but without a lastKnownGood.json (corepack install does not prepare it), it attempts a network request

I wonder if https://github.com/nodejs/corepack/pull/432/files#diff-ac2e60980d41d74acab8d4ec9d41b6bd52b08f056b3ea65f89952de64eca3601 did not fix that already.

@aduh95 aduh95 enabled auto-merge (squash) April 13, 2024 11:29
@aduh95 aduh95 merged commit c449adc into nodejs:main Apr 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants