Refactored integrity generation and check #2818

bestander · 2017-03-01T22:18:29Z

Summary

I plan to improve integrity check code so that it could also verify node_modules structure.
This PR moves most of the integrity checking and saving code into a separate class, no actual changes to the logic.

Test plan

All tests should pass (integrity checks are covered with integration tests)

Pending things to do:

a bit of cleanup

…d check commands

bestander · 2017-03-02T00:32:57Z

src/cli/commands/check.js

  const install = new Install(flags, config, reporter, lockfile);

  // get patterns that are installed when running `yarn install`
+  // TODO measure slowdown here


This is a reminder for myself, I noticed check got slower in the last month.
Probably one of the following 2 commands to blame

arcanis · 2017-03-02T01:28:58Z

src/integrity-checker.js

+    // if we already have an integrity hash in one of these folders then use it's location otherwise use the
+    // first folder
+    const possibles = possibleFolders.map((folder): string => path.join(folder, constants.INTEGRITY_FILENAME));
+    let loc = possibles[0];


Shouldn't it check to see if a directory exists before fallbacking to the first entry? For example, let's say Yarn supports Bower and that there's a bower_modules directory but no node_modules one. Shouldn't Yarn use the bower_directory rather than the non-existing node_modules? (on a similar note but possibly out of scope for this PR, shouldn't the integrity file be different for each registry rather than global to all of them?)

Check out the next few lines.
possibles[0] is taken by default but then it checks fs.exists(possibleLoc) and picks the first one that exists.

on a similar note but possibly out of scope for this PR, shouldn't the integrity file be different for each registry rather than global to all of them?

We use one type of registry per project, unlikely there will be a bower_modules and node_modules in one folder, so it should be fine to have only one integrity there.

It checks to see if the path(folder, constants.INTEGRITY_FILENAME) file exists, but not the folder itself. So for example, if there's a bower_modules without integrity file, the returned path will still be node_modules/....

Still thinking it through.

I don't think we should choose location for .yarn-integrity based on a folder.

What if a project has checked-in bower_modules folder managed by bower, then Yarn might pick it up as the location but user might want to install node_modules instead.

However, we should pick the location based on what type of registry are there in the project.
Right now we support only node_modules and yarn/npm registry, so it would be a more relevant when we support another one.

arcanis · 2017-03-02T01:43:25Z

src/cli/commands/check.js

-    }
+  const match = await integrityChecker.check(patterns, lockfile, lockSource, flags);
+  for (const pattern of match.missingPatterns) {
+    reportError('lockfileNotContainPatter', pattern);


It seems there's a typo in the name of the error (but this typo also affects the lang file so it actually works)

arcanis · 2017-03-02T01:44:07Z

src/cli/commands/check.js

-    }
-  } else {
+  if (match.integrityFileMissing) {
    reportError('noIntegirtyHashFile');


Yeah, let's fix those here.
In the future it may be problematic when there are translations in the wild

bestander · 2017-03-07T01:07:51Z

@arcanis, I refactored integrity hash location a bit.
The idea is to pass the subset of used registries on save (and pick first if none have an integrity file).
As commented above I think we should not rely on a folder location

arcanis · 2017-03-07T01:46:27Z

src/integrity-checker.js

+    let registries = registryNames;
+    if (usedRegistries && usedRegistries.size > 0) {
+      registries = usedRegistries;
+    }


Even if it's currently the case here, you might want to put a comment to explain that passing falsy values for usedRegistries is only acceptable when one only needs to read the file but not write to it - otherwise multiple integrity files might get generated!

Removed my previous comment.
Actually it is fine to pass falsy usedRegistries when generating the integrity file, e.g. running yarn install for empty package.json.
Also Yarn won't generate multiple integrity files because this method checks whether one exists already and reuses that location

arcanis · 2017-03-07T01:47:36Z

src/integrity-checker.js

+    return {
+      locationPath: loc || possibles[0],
+      exists: !!loc,
+    };


Not fond of the location getter that also check if a file exists, but it's a private method, so no big deal.

arcanis · 2017-03-07T01:49:48Z

src/integrity-checker.js

        loc = possibleLoc;
        break;
      }
    }


There's still the behaviour where the first entry will always be returned if there is no integrity file, even if a more "suitable" already directory exists, right? Do you want to keep it that way?

This script will return the first folder that has .yarn-integrity file.
If none found it will pick the first from the list of available.

Right now it is hard to decide whether existence of bower_modules is the right signal for Yarn to save .yarn-integrity there.
Let's get back to this subject when Yarn has more than one folder it can serve, currently it is node_modules only

bestander and others added 5 commits March 1, 2017 12:49

proper integrity check: first commit

1cfaa7d

moved the rest code to integrity-check and integrated with install an…

8e984fb

…d check commands

more fixes

f6f0050

removed todo

d7e4ec5

removed hashes from integrity error message

64eb3fa

bestander changed the title ~~[WIP] Refactored integrity generation and check~~ Refactored integrity generation and check Mar 2, 2017

bestander commented Mar 2, 2017

View reviewed changes

arcanis reviewed Mar 2, 2017

View reviewed changes

bestander added 4 commits March 3, 2017 15:09

typos fixes

54d6c89

replaced todo with a proper comment

4c22ee5

added a test for bailout

b399747

changed integrity hash location logic a bit

32cc176

arcanis reviewed Mar 7, 2017

View reviewed changes

arcanis approved these changes Mar 7, 2017

View reviewed changes

bestander merged commit ddff4c5 into yarnpkg:master Mar 7, 2017

Refactored integrity generation and check #2818

Refactored integrity generation and check #2818

Conversation

bestander commented Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanis Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestander Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestander commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bestander commented Mar 1, 2017 •

edited

Loading

arcanis Mar 2, 2017 •

edited

Loading

bestander Mar 7, 2017 •

edited

Loading