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

When a package references a tarball, and the tarball's dependencies change, the change isn't reflected in the shrinkwrap. #1342

Closed
iclanton opened this issue Aug 28, 2018 · 10 comments

Comments

@iclanton
Copy link

pnpm version:

2.13.6

Code to reproduce the issue:

Currently putting together repro repo

Expected behavior:

When a package references a tarball, and the tarball's dependencies change, the change is reflected in the shrinkwrap and the correct dependencies are installed.

Actual behavior:

When a package references a tarball, and the tarball's dependencies change, the change isn't reflected in the shrinkwrap and incorrect dependencies are installed.

Additional information:

  • node -v prints: 8.11.4
  • Windows, OS X, or Linux?: Windows 10
@octogonz
Copy link
Sponsor Member

I investigated this. We think that people are manually tampering with shrinkwrap.yaml.

It starts out like this:

  'file:projects/example.tgz':
    dependencies:
      gulp: 3.9.0
    dev: false
    name: '@rush-temp/example'
    resolution:
      integrity: sha512-A2ac6a6cPfvxyknisFEMpjV9vS3VW8u4cx6Abgdp0Kwel6v+NOfAuB7nXXFQJxZuYelJ1aSBWzZzRexh5A4lbg==
      tarball: 'file:projects/example.tgz'
    version: 0.0.0

...and then I find a commit where (for some reason) a person has bumped gulp to 3.9.1, without updating the package.json file that ends up in this tarball. We're still trying to determine why people do this, but it's apparently something people do occasionally.

Then rush install sees that gulp 3.9.0 is missing from shrinkwrap.yaml, and runs pnpm install to fix that, but afterwards the shrinkwrap file is unchanged. The reason is that PNPM (reasonably) assumes that the shrinkwrap is up to date, because the integrity hash seems to match the tarball.

A question

This problem is admittedly pretty Rush-specific. But we're thinking the most robust solution would be to delete the integrity field from the shrinkwrap file before we run pnpm install, to force PNPM to reconsider the tarball contents.

@zkochan a couple questions:

  1. Is it okay to delete the integrity field from shrinkwrap.yaml for certain entries?
  2. Will this force PNPM to read the package.json contents and update the shrinkwrap file?
  3. Will this affect install times significantly? (e.g. if we have 150 such projects in a repo with ~2500 package dependencies)

@octogonz
Copy link
Sponsor Member

@nickpape-msft

@zkochan
Copy link
Member

zkochan commented Aug 30, 2018

...and then I find a commit where (for some reason) a person has bumped gulp to 3.9.1, without updating the package.json file that ends up in this tarball. We're still trying to determine why people do this, but it's apparently something people do occasionally.

I would like to know the reason for this before we make a decision.

Integrities of local tarballs are useful because pnpm uses them to detect changes inside the tarballs. So if the gulp version inside the tarball is bumped, the integrity changes and pnpm unpacks it and reinstalls

@octogonz
Copy link
Sponsor Member

octogonz commented Aug 30, 2018

Following up: I tracked down the person who committed the changes. He's pretty sure that nobody manually modified the shrinkwrap.yaml file. It was part of an incident where a bundle size increased because two different versions of a dependency were getting installed, and during the scuffle they had to roll back their Git branch a couple times, and also resolve merge conflicts with the master branch.

It is conceivable that Git incorrectly merged the shrinkwrap file, however our .gitattributes specifically tries to prevent this:

shrinkwrap.yaml              merge=binary
npm-shrinkwrap.json          merge=binary

@zkochan Is it possible that PNPM itself can fail to update the integrity hash in certain circumstances?

Rush actually regenerates these tarballs for every installation. In the past, we had problems where the integrity hash would be different each time, due to timestamps inside the tarball. We mostly worked around that by using the portable option for tar, however certain people still get diffs occasionally for some reason (maybe NTFS path case differences?). If it won't significantly affect PNPM performance, deleting the integrity hashes from the shrinkwrap.yaml file might fix both issues.

@zkochan
Copy link
Member

zkochan commented Aug 30, 2018

I think it might be an issue with headless installation.
Currently, when the shrinkwrap.yaml satisfies the package.json in the root, a simpler, faster algorithm is used for installation and it uses a frozen shrinkwrap, so it doesn't check whether the tarball matches the integrity checksum.
As a temporary solution, you can set prefer-frozen-shrinkwrap to false.

If my assumption is correct, the fix should be easy: pnpm should never run headless installation if there are local tarball dependencies

@octogonz
Copy link
Sponsor Member

I still have the repro -- lemme try that.

@octogonz
Copy link
Sponsor Member

As a temporary solution, you can set prefer-frozen-shrinkwrap to false.

This fixed the problem! Should we have Rush always include --prefer-frozen-shrinkwrap false when invoking pnpm install? That's a pretty easy change to make.

@zkochan
Copy link
Member

zkochan commented Aug 30, 2018

If Rush always uses local tarballs, then I think you can use --no-prefer-frozen-shrinkwrap because that is what pnpm will use anyway after the fix

@octogonz
Copy link
Sponsor Member

We'll do that, since not everyone will be using the latest PNPM. Thanks again for helping us investigate this!

@octogonz
Copy link
Sponsor Member

octogonz commented Sep 8, 2018

@zkochan We can close this issue now also. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants