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

chore: Do not allow changes to package json during CI yarn install #12125

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

spalladino
Copy link
Contributor

@spalladino spalladino commented Feb 19, 2025

Running yarn install will format the package.json for all packages in the workspace. This will cause cache_content_hash to fail on CI due to changed files, which (before #12124) caused the build to terminate abruptly (see here for an example).

This PR adds all package.json files to yarn's immutablePatterns, so if it detects that the install process changed them it, it will fail with The checksum for **/package.json has been modified by this install, which is explicitly forbidden. and a nice exit code, which should be visible on the CI run. This only applies to CI runs.

Running `yarn install` will format the package.json for all packages in
the workspace. This will cause `cache_content_hash` to fail on CI due to
changed files, which causes the build to terminate abruptly (see
[here](https://github.com/AztecProtocol/aztec-packages/actions/runs/13422411604/job/37497960586?pr=12096#step:7:109) for an example).

This PR adds all package.json files to yarn's `immutablePatterns`, so if
it detects that the install process changed them it, it will fail with
`The checksum for **/package.json has been modified by this install,
which is explicitly forbidden.` and a nice exit code, which should be
visible on the CI run. This only applies to CI runs.
# If in CI mode, retry as bcrypto can sometimes fail mysteriously.
# We set immutable since we don't expect the yarn.lock to change. Note that we have also added all package.json
# files to yarn immutablePatterns, so if they are also changed, this step will fail.
denoise "retry yarn install --immutable"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is redundant as yarn install reacts to CI being set

Copy link
Collaborator

Choose a reason for hiding this comment

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

(I don't hate it tho)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree it's redundant, but I'd rather be explicit

@ludamad ludamad enabled auto-merge (squash) February 19, 2025 22:53
@ludamad ludamad merged commit cca368a into master Feb 19, 2025
11 checks passed
@ludamad ludamad deleted the palla/immutable-yarn-install branch February 19, 2025 23:11
spalladino added a commit that referenced this pull request Mar 6, 2025
We introduced two issues in #12125 when adding the immutable patterns to
yarn install:

- If `immutablePatterns` is set, yarn sets the `immutable` flag even
  outside CI (contradicting its documentation). This is fixed by
explicitly setting `--no-immutable` on the non-CI branch of install.

- The glob `**/package.json` inadvertently matched package.json files
  within `node_modules`, so installing them created the new files, which
means they were flagged as changed. This is fixed by having a less eager
glob.

Fixes #12538
spalladino added a commit that referenced this pull request Mar 6, 2025
We introduced two issues in #12125 when adding the immutable patterns to
yarn install:

- If `immutablePatterns` is set, yarn sets the `immutable` flag even
outside CI (contradicting its documentation). This is fixed by
explicitly setting `--no-immutable` on the non-CI branch of install.

- The glob `**/package.json` inadvertently matched package.json files
within `node_modules`, so installing them created the new files, which
means they were flagged as changed. This is fixed by having a less eager
glob.

Fixes #12538
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