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

Bugfix 3752 - Artifacts disappearing when missing integrity file #4185

Merged

Conversation

Projects
None yet
4 participants
@jamsinclair
Copy link
Contributor

commented Aug 16, 2017

Summary

I encountered personally similar problems as described in #3752 after interrupting a yarn install.

After some debugging I isolated this to being that the integrity file is removed during the linking step (with good reasons).

However, if this file is never rewritten (e.g. user/system interrupted yarn) the next time yarn is invoked it no longer has context of package artifacts, as they are retrieved from the integrity file.

I played around with some solutions and would like to put forward an idea of forcing package installations if the integrity file is missing.

Test plan

I've added two test cases for add and install scripts that replicate the reproduction steps listed in #3752 (comment).

Without changes to the src code you can see these test cases fail (running Node v8.2.1):
screen shot 2017-08-17 at 00 09 06
screen shot 2017-08-17 at 00 11 24

With the src codes changes you can see on CI that all tests pass ✌️

Concerns

I'm not sure but, are there any scenarios where setting force in the PackageInstallScripts would cause the lockfile to be rewritten? Thinking of whether this change would affect flags like --frozen-lockfile 🤔

@arcanis arcanis self-requested a review Aug 16, 2017

@arcanis arcanis self-assigned this Aug 16, 2017

@arcanis

This comment has been minimized.

Copy link
Member

commented Aug 16, 2017

Great work @jamsinclair, thanks! Have you checked if it works fine with the --ignore-scripts flag?

@@ -34,6 +34,10 @@ export default class PackageInstallScripts {
force: boolean;
artifacts: InstallArtifacts;

setForce(force: boolean) {

This comment has been minimized.

Copy link
@BYK

BYK Aug 16, 2017

Member

Do we really need these setters and getters?

This comment has been minimized.

Copy link
@jamsinclair

jamsinclair Aug 16, 2017

Author Contributor

Good question.

The force property was formerly only set on construction of the PackageInstallScripts class:
https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/install.js#L178

When the install bailout method runs, the PackageInstallScripts instance has already been created. Adding the setter, allowing overrides, seemed like a simpler solution rather than reinstantiating. Happy to hear another approach.

Edit: Oh, is it all good to override properties directly, this.scripts.force = true? In that case could definitely remove setter 😂

This comment has been minimized.

Copy link
@arcanis

arcanis Aug 17, 2017

Member

I think a setter is fine, it's how we already do it (setArtifacts), and I have the feeling it makes the code a bit safer (because JS doesn't (yet) have private variables, setters are one of the only ways to clearly state that a variable might be updated from the outside).

@jamsinclair

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2017

@arcanis, yup just tested changes now with --ignore-scripts 👍

Looks right. The package install scripts do not seem to take place and reports a warning:

warning Ignored scripts due to flag.

@arcanis arcanis merged commit 6bfaac3 into yarnpkg:master Aug 17, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arcanis

This comment has been minimized.

Copy link
Member

commented Aug 17, 2017

Thanks @jamsinclair! A simple solution to an important issue 👌

@anhkind

This comment has been minimized.

Copy link

commented Aug 21, 2017

Will this fix be included in 0.27.x and 0.28.x versions soon enough?

@arcanis

This comment has been minimized.

Copy link
Member

commented Aug 21, 2017

It's already available through the nightly builds. Otherwise, it will be in the next tagged release, which should be the 1.0.0 and should come in the next few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.