Fix: Reimplements part of the integrity check #4122

arcanis · 2017-08-08T18:13:40Z

Summary

The integrity file doesn't currently support workspaces, which leads
to #3863 (removing a workspace node_modules folder and running yarn
doesn't do anything, because Yarn incorrectly assumes that everything
is already done).

This PR rewrites a good chunk of the integrity logic to support and
fix this behavior.

Test plan

Tests are passing, I still have to write an automated test before
merging this PR.

arcanis · 2017-08-09T13:31:08Z

The tests are failing consistently on CI, but not in local oõ

**Summary** The integrity file doesn't currently support workspaces, which leads to yarnpkg#3863 (removing a workspace node_modules folder and running yarn doesn't do anything, because Yarn incorrectly assumes that everything is already done). This PR rewrites a good chunk of the integrity logic to support and fix this behavior. **Test plan** Tests are passing, I still have to write an automated test before merging this PR.

BYK

Looks pretty great (and seems to be more efficient than what we had).

I was able to reproduce the failure on my local so let's fix that and I'm good.

BYK · 2017-08-09T15:58:06Z

src/integrity-checker.js

+    } else if (this.config.workspaceRootFolder) {
+      return this.config.workspaceRootFolder;
+    } else {
+      return path.join(this.config.lockfileFolder, 'node_modules');


I think we should use this.config.registries[name].folder instead of 'node_modules' here.

I'd like to avoid using this.config.registry, since its semantic isn't clear (cf #4109). However I can put node_modules in the constants.js file, if you prefer.

Until we actually act on #4109 we should keep things the same way so the clean up itself is easier too.

BYK · 2017-08-09T15:58:33Z

src/integrity-checker.js

    } else {
-      locationFolder = await this._getModuleLocation(usedRegistries);
+      return path.join(this.config.lockfileFolder, 'node_modules');


Same as above: let's use already defined constants instead of 'node_modules'

Also, shouldn't we include this.config.workspaceRootFolder in this chain too?

No, workspaceRootFolder should only be used for resolving relative paths. In workspaces it will be the package root folder, and we don't want to store the .yarn-integrity file there.

BYK · 2017-08-09T16:00:13Z

src/integrity-checker.js

@@ -118,23 +109,59 @@ export default class InstallationIntegrityChecker {
  }

  /**
-   * returns a list of files recursively in a directory sorted
+   * Get the list of the directories that contain our modules (there might be multiple such folders b/c of workspaces).


s/might/may

BYK · 2017-08-09T16:00:54Z

src/integrity-checker.js

+    if (this.config.modulesFolder) {
+      locations.push(this.config.modulesFolder);
+    } else {
+      locations.push(path.join(this.config.lockfileFolder, 'node_modules'));


No 'node_modules' please.

BYK · 2017-08-09T16:02:40Z

src/integrity-checker.js

+    const files = [];
+
+    const recurse = async dir => {
+      if (!await fs.exists(dir)) {


Why do we need this check?

The dir might not exists at the root. I'll move it inside the l160 loop, since it only matter there.

BYK · 2017-08-09T16:12:46Z

src/integrity-checker.js

+
+    // If we run "yarn" after "yarn --check-files", we shouldn't fail the less strict validation
+    if (actual.flags.indexOf('checkFiles') === -1) {
+      relevantExpectedFlags = relevantExpectedFlags.filter(flag => flag !== 'checkFiles');


Feels like 'checkFiles' and possible other flags should be in constants.

BYK · 2017-08-09T16:13:16Z

src/integrity-checker.js

-        const actualFiles = await this._getFilesDeep(locationFolder);
-        if (actualFiles.length > 0) {
-          return 'FILES_MISSING';
+      // Early bailout if we expect more files than what we have


BYK · 2017-08-09T16:16:30Z

src/integrity-checker.js

-          }
+
+        // If we've reached the end of the actual array, the file is missing
+        if (v === actual.files.length) {


Another optimization: whenever actual.files.length - v < expected.files.length - u we're done.

BYK · 2017-08-09T16:18:52Z

src/integrity-checker.js

-      workspaceLayout,
-    );
+    let integrityMatches = this._compareIntegrityFiles(actual, expected, flags.checkFiles, workspaceLayout);
+    console.log(actual, expected, integrityMatches);


Don't forget to remove this guy :)

Yeah I'm debugging why it breaks

BYK · 2017-08-09T16:20:04Z

src/integrity-checker.js

+          this.config.lockfileFolder,
+          await fs.exists(path.join(this.config.lockfileFolder, modulesFolder)),
+        );
+        if (!await fs.exists(path.join(this.config.lockfileFolder, modulesFolder))) {


This needs comment since I don't follow the logic. This should have been caught by the checks above already, right?

Nope

the files entry contains the list of files, but is only there if we use the --check-files option. Otherwise it does no check at all. We still want to check if at the very least the directories exist.

the modulesFolders entry is a static list of directory that isn't baked by the filesystem. They are not the actual physical folders, they are the theoretical folders. So when we compare them in _compareIntegrityFiles, we don't actually check they exist. That's done here.

We still want to check if at the very least the directories exist.

See, we need to add a comment about this :D

the modulesFolders entry is a static list of directory that isn't baked by the filesystem. They are not the actual physical folders, they are the theoretical folders. So when we compare them in _compareIntegrityFiles, we don't actually check they exist. That's done here.

Wow. Again, needs comment so any future person would know this when they look at the code.

…patterns

BYK · 2017-08-10T10:07:36Z

src/integrity-checker.js

@@ -185,7 +185,21 @@ export default class InstallationIntegrityChecker {
      artifacts,
    };

-    result.topLevelPatterns = patterns.sort(sortAlpha);
+    result.topLevelPatterns = patterns.sort(sortAlpha).filter(p => {


I'd do filter first and sort second to reduce the runtime of sort.

BYK · 2017-08-10T10:10:10Z

src/integrity-checker.js

@@ -185,7 +185,21 @@ export default class InstallationIntegrityChecker {
      artifacts,
    };

-    result.topLevelPatterns = patterns.sort(sortAlpha);
+    result.topLevelPatterns = patterns.sort(sortAlpha).filter(p => {
+      return !workspaceLayout || !workspaceLayout.getManifestByPattern(p);


Since workspaceLayout won't ever change during the lifetime of this function, you can do the following and speed up the cases where workspaces are not used:

const topLevelPatterns = workspaceLayout ? patterns.filter(p => !workspaceLayout.getManifestByPattern(p)) : patterns; result.topLevelPatterns = topLevelPatterns.sort(sortAlpha);

BYK · 2017-08-10T10:10:51Z

src/integrity-checker.js

+    if (workspaceLayout) {
+      for (const name of Object.keys(workspaceLayout.workspaces)) {
+        if (workspaceLayout.workspaces[name].loc) {
+          result.topLevelPatterns = result.topLevelPatterns.concat(


This breaks sorting, right? Is that intentional?

It's still sorted, just not sorted alphabetically. As long as it has a deterministic order, it's should be fine.

Object.keys(workspaceLayout.workspaces) doesn't have a deterministic order AFAIK so you may wanna sort the keys first. Also we should add a comment about this into the code.

Yeah I guess. It's actually a bit more complex: the sorting order is de facto implementation-defined, so it will stay the same for any given engine, which is all we need. I thought about sorting the array but it wouldn't change anything ... except be a bit safer in some weird edge cases.

Still you're right, I'll add it, just in case. Better be safe :)

I don't think assuming engine internals will stay the same across runs is safe enough, yeah. Thanks! :)

BYK · 2017-08-10T10:12:45Z

src/integrity-checker.js

-        // Skip over files that have been added
-        while (v < actual.files.length && actual.files[v] !== expected.files[u]) {
+        // Number of iterations after which there won't be enough entries remaining for the arrays to match
+        const max = actual.files.length - expected.files.length;


Move this out of the loop? :)

I did this originally but I find it more readable for this variable and its comment to be close to the place we're using it. Since this is something that could/should be optimized by a decent compiler I think it's fine.

Well, you can move it to the initialization part of the for loop if you wanna keep it cozy? :D

BYK · 2017-08-10T10:14:45Z

src/integrity-checker.js

          v += 1;
        }

        // If we've reached the end of the actual array, the file is missing
-        if (v === actual.files.length) {
+        if (v === max) {


Don't think this logic is correct. max is just a "small" difference where v is the absolute index which has to go past max. If you kept a separate advance variable and calculated v as u + advance and did if (advance === max) then the logic would be correct.

BYK

Rejecting because of non-deterministic Object.keys() comment. Also I'd love for you to address my other comments, especially around not using the string 'node_modules' but other optimizations and clarifications as well.

BYK · 2017-08-10T13:34:52Z

src/integrity-checker.js

+    if (workspaceLayout) {
+      for (const name of Object.keys(workspaceLayout.workspaces)) {
+        if (workspaceLayout.workspaces[name].loc) {
+          result.topLevelPatterns = result.topLevelPatterns.concat(


Object.keys(workspaceLayout.workspaces) doesn't have a deterministic order AFAIK so you may wanna sort the keys first. Also we should add a comment about this into the code.

BYK · 2017-08-10T13:38:39Z

src/integrity-checker.js

+
+      // Since we know the "files" array is sorted (alphabetically), we can optimize the thing
+      // Instead of storing the files in a Set, we can just iterate both arrays at once. O(n)!
+      for (let u = 0, v = 0; u < expected.files.length; ++u) {


I honestly think the following is easier to follow:

const maxDiff = actual.files.length - expected.files.length; if (maxDiff < 0) { return 'FILES_MISSING'; } const advance = 0; for (const u = 0; u < expected.files.length; ++u) { while (advance < maxDiff && actual.files[u + advance] !== expected.files[u]) { advance += 1; } if (advance === maxDiff) { return 'FILES_MISSING'; } }

Hm, I think your code won't early exit when the check cannot succeed anymore. It will always iterate on all of actual (minus advance).

It will always iterate on all of actual (minus advance).

Not really. It keeps incrementing advance as long as there are non-matching so if the non-matching files are at the start of the array, as soon as we exhaust them (maxDiff iterations) we stop.

…heck

arcanis · 2017-08-14T13:20:09Z

@BYK Code is now using a constant. Good to merge? :)

BYK

Nice! Let's start enjoying the faster and more comprehensive integrity checks!

BYK · 2017-08-14T13:23:58Z

src/integrity-checker.js

+      for (const workspaceName of Object.keys(workspaceLayout.workspaces)) {
+        const loc = workspaceLayout.workspaces[workspaceName].loc;
+
+        if (loc) {


I think after my fixes, we don't need the emptiness check here anymore.

Referring to these changes:

https://github.com/yarnpkg/yarn/pull/4129/files#diff-ffb8cf10e4b2d16cc7df603574698af6R287

https://github.com/yarnpkg/yarn/pull/4131/files#diff-1ff3de58ebf278d554d14bcbe160d981R12

BYK · 2017-08-14T13:31:40Z

src/integrity-checker.js

+    }
+
+    for (const modulesFolder of this._getModulesFolders({workspaceLayout})) {
+      if (await fs.exists(modulesFolder)) {


I'm still not entirely getting how we may have missing folders in workspaces. Can you provide more details about what this is protecting us from?

The _getModulesFolders function only returns the theoretical folders, it does not check that those folders actually exist on the disk. So for example, if we have a workspace named foobar, the function will return foobar/node_modules without checking that this folder actually exists.

Bnaya · 2017-08-14T13:54:29Z

Thanks @arcanis @BYK :D

arcanis force-pushed the check-workspace-files branch from b101665 to 2f479d9 Compare August 8, 2017 18:15

arcanis changed the title ~~Reimplements part of the integrity check~~ Fix: Reimplements part of the integrity check Aug 8, 2017

arcanis requested a review from BYK August 8, 2017 18:15

arcanis force-pushed the check-workspace-files branch 2 times, most recently from 4978a20 to 1eabd01 Compare August 9, 2017 12:18

arcanis force-pushed the check-workspace-files branch 2 times, most recently from 3fc70b1 to d3185c4 Compare August 9, 2017 14:06

arcanis force-pushed the check-workspace-files branch from d3185c4 to 0765d26 Compare August 9, 2017 14:23

BYK requested changes Aug 9, 2017

View reviewed changes

BYK self-assigned this Aug 9, 2017

Fixes a bug by making sure the topLevel entry includes the workspace …

9deaad1

…patterns

BYK requested changes Aug 10, 2017

View reviewed changes

Fixes the calculation of the "max" variable

d3ea78c

BYK requested changes Aug 10, 2017

View reviewed changes

Maël Nison added 3 commits August 10, 2017 17:14

Crafts the pattern manually, since they're not available under yarn c…

537d4a9

…heck

Flow

6321f95

Uses a constant instead of a literal

6f08fa8

BYK approved these changes Aug 14, 2017

View reviewed changes

BYK merged commit 50508b2 into yarnpkg:master Aug 14, 2017

BYK mentioned this pull request Aug 16, 2017

"yarn check --integrity" fails with npm as a dependency #3679

Closed

Fix: Reimplements part of the integrity check #4122

Fix: Reimplements part of the integrity check #4122

Conversation

arcanis commented Aug 8, 2017

arcanis commented Aug 9, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented Aug 14, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bnaya commented Aug 14, 2017

arcanis Aug 9, 2017 •

edited

Loading

arcanis Aug 9, 2017 •

edited

Loading

BYK Aug 10, 2017 •

edited

Loading

arcanis Aug 10, 2017 •

edited

Loading