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

Yarn v2/v3 zero-install not updating cache files #5946

Closed
agrobbin opened this issue Oct 21, 2022 · 19 comments · Fixed by #5964
Closed

Yarn v2/v3 zero-install not updating cache files #5946

agrobbin opened this issue Oct 21, 2022 · 19 comments · Fixed by #5964
Labels
L: javascript:yarn npm packages via yarn

Comments

@agrobbin
Copy link

agrobbin commented Oct 21, 2022

As first mentioned by @viniciuspalma in #1297 (comment), dependency updates on Yarn v2/v3 are not including updates to cache files, making yarn install --immutable --immutable-cache fail due to outdated cache data.

An example Dependabot run that generated a handful of dependency updates without updating the cache information in one of my repos is 489332542. I thought it might be because I hadn't included vendor: true in my .dependabot.yml configuration file, but adding that failed the Dependabot validation check.

/cc @jurre

@jurre
Copy link
Member

jurre commented Oct 21, 2022

Could you link to a repo where this reproduces?

The new dependency should be included in the cache, which I can reproduce in test repo's and we also have tests for.

There is an upstream issue on yarn regarding the old entry not getting removed from the cache: yarnpkg/berry#4886, is that what you're running into?

@pavera
Copy link
Contributor

pavera commented Oct 21, 2022

We currently run yarn with --mode=update-lockfile, I believe this prevents us from removing items from the cache though I've certainly seen the cache get new items added. Since yarn install will probably attempt to remove the extraneous items from the cache I assume the failure is with --immutable-cache which will fail if it modifies the cache.

@agrobbin
Copy link
Author

The repo in question is yassosio/yassos, a private repo. PR 313 is an example PR that should help debug!

Looking more closely, it might actually be 2 issues, the old cache item not being removed that you mentioned above, as well as .pnp.cjs and .pnp.loader.mjs not being updated.

@jurre
Copy link
Member

jurre commented Oct 21, 2022

The repo in question is yassosio/yassos, a private repo. PR 313 is an example PR that should help debug!

We're actually not able to see any of that 😄

the old cache item not being removed that you mentioned above

Maybe we need to try and manually do this for now? Seems a bit icky 😞 We could drop --mode=update-lockfile for repo's that have a cache folder checked in, but it degrades performance significantly from my testing

as well as .pnp.cjs and .pnp.loader.mjs not being updated.

Ah, I think this should actually be an easy fix, we just need to track those files as things that need to be updated with the PR. We'll need to check if the path for those is configurable and pull it from the config file if so

@agrobbin
Copy link
Author

@jurre thanks for the quick response!

Re: --mode=update-lockfile, I think your suggestion makes a lot of sense, otherwise the caching behavior as it stands does not have much benefit, as the cache would effectively continue to grow and grow, and the recommendation from Yarn itself of running --immutable --immutable-cache in places like CI would not work. I'm not sure what the performance impacts are, but without removing old cache items, that is a significant limitation to Yarn v2/v3 with Dependabot.

Great to hear that the .pnp.cjs and .pnp.loader.mjs updates are an easy fix!

@agrobbin
Copy link
Author

Looking more closely at .pnp.cjs and .pnp.loader.mjs, I think the only one that likely needs to be tracked is .pnp.cjs.

@deivid-rodriguez deivid-rodriguez added the L: javascript:yarn npm packages via yarn label Oct 21, 2022
@wojtekmaj
Copy link

Could you link to a repo where this reproduces?

The new dependency should be included in the cache, which I can reproduce in test repo's and we also have tests for.

There is an upstream issue on yarn regarding the old entry not getting removed from the cache: yarnpkg/berry#4886, is that what you're running into?

Here's a PR that Dependabot has just created in my repo in which .yarn/cache is checked out:

obraz

As you can see, clearly, old core_js is no longer used, as it's completely wiped from yarn.lock. However, the binary from .yarn/cache has not been removed.

@jonaskuske
Copy link

Looking more closely at .pnp.cjs and .pnp.loader.mjs, I think the only one that likely needs to be tracked is .pnp.cjs

And .pnp.data.json, created through pnpEnableInlining: false!
From Yarn 4 it'll always be .pnp.data.json (yarnpkg/berry#4773), but prior to that it's configurable via .yarnrc.yml#pnpDataPath.

@pavera
Copy link
Contributor

pavera commented Oct 25, 2022

I've deployed the fix for this, feel free to re-open if you continue to see issues with zero-install enabled repos.

@agrobbin
Copy link
Author

@pavera just tried it! Things look good, except that the permissions of .pnp.cjs don't seem to be maintained (it changes from 755 to 644).

@gabormagyar
Copy link

gabormagyar commented Oct 25, 2022

@pavera Thanks for looking at this!

There's one more thing missing that I'd like to report. If packages specify build scripts, yarn install will unpack them into the .yarn/unplugged folder. This is reflected in the .pnp.cjs file, which is then committed. More info on this behavior here.

Dependabot seems to undo these changes to the .pnp.cjs file. The next yarn install will generate these diffs in my example:

Screenshot 2022-10-25 at 21 37 49

To reproduce: add (for example) the protobufjs package, and let dependabot run on the repo. (Can be a different package that gets updated.)

@pavera
Copy link
Contributor

pavera commented Oct 26, 2022

We believe this is caused by our policy on post install scripts. We currently disable post install scripts yarn config set enableScripts false. This is something we generally do across all package managers, for security reasons we attempt to run as little 3rd party code as possible. I'll confirm this config does cause the behavior noted and we'll go from there.

@merceyz
Copy link

merceyz commented Oct 27, 2022

@pavera If you use the --mode="skip-build" flag instead of the config you get the behaviour you're after.

skip-build will not run the build scripts at all. Note that this is different from setting enableScripts to false because the later will disable build scripts, and thus affect the content of the artifacts generated on disk, whereas the former will just disable the build step - but not the scripts themselves, which just won't run.
https://yarnpkg.com/cli/install#options-mode%20%230:~:text=skip%2Dbuild,scripts%20at%20all

@pavera pavera reopened this Nov 1, 2022
@pavera
Copy link
Contributor

pavera commented Nov 1, 2022

I've begun work on a larger refactor to address this as well as hopefully regain the performance improvements we lost when we removed --mode=update-lockfile for those who are not using zero-installs the removal of this option massively degrades performance.

Adding to the complexity, --mode is only available in yarn >= 3.

Currently the proposed pseudo code solution looks like:

if yarn >= 2 && yarn < 3
  args = ""
  yarn config set enableScripts false
if yarn >= 3 and zero-install
  args = "--mode=skip-build"
if yarn >= 3 and !zero-install
  args = "--mode=update-lockfile"
  yarn config set enableScripts false

Hopefully this will satisfy performance and behavior requirements for the largest possible group of users :)

@merceyz
Copy link

merceyz commented Nov 1, 2022

Adding to the complexity, --mode is only available in yarn >= 3.
if yarn >= 2 && yarn < 3

^2.4.0 has a --skip-build flag that you can use though only for yarn install (yarnpkg/berry#2064).

if yarn >= 3 and !zero-install

You don't need to set enableScripts when using the --mode flag.

@pavera
Copy link
Contributor

pavera commented Nov 2, 2022

I've now deployed #6017 which I believe should result in correct --mode usage. I'm closing this as fixed, please re-open if there are additional zero install issues.

@pavera pavera closed this as completed Nov 2, 2022
@agrobbin
Copy link
Author

agrobbin commented Nov 4, 2022

@pavera this seems to mostly be fixed, the only exception is the file permission of .pnp.cjs! I'm still seeing it change from 755 to 644.

@osdiab
Copy link

osdiab commented Nov 7, 2022

just received a batch of dependabot updates from 8 hours ago, and they didn't touch my yarn cache file - just updated dependencies in (nested yarn monorepo) package.json files and then fail our CI because we install with yarn install --immutable

Screen Shot 2022-11-08 at 08 52 51

@pavera
Copy link
Contributor

pavera commented Dec 1, 2022

@osdiab There was a subsequent issue #6076 which I'm working to fix right now. Without more info about your specific repo setup I can't really tell if the fix will repair your issue, but if you want to move to that issue to continue the conversation that would be great. I'm going to deploy the fix later today, so hopefully it addresses your issue as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: javascript:yarn npm packages via yarn
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants