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

fix: relativize file links when inflating shrinkwrap #758

Closed
wants to merge 9 commits into from

Conversation

jsnajdr
Copy link
Contributor

@jsnajdr jsnajdr commented Feb 3, 2020

Fixes #750.

When reading a package tree from disk and package.json files, file:* dependencies have paths relative to the current package directory. But when reading from shrinkwap file, the shrinkwrap file stores paths relative to the top package's path.

Both data are used to populate a tree.package.dependencies field. For the shrinkwrap, it happens here. But such dependencies have wrong path. They need to be relativized after reading from the shrinkwrap.

The bug manifests when computeMetadata goes through tree.package.dependencies and tries to find a matching package in the tree that satisfies each dependency. But because the file: paths are wrong, the matching package is not found, although it is present in the tree. The package's requiredBy array remains empty and it's removed by pruneIdealTree. The result is a broken node_modules tree with missing links.

@jsnajdr jsnajdr requested a review from a team as a code owner February 3, 2020 10:11
@jsnajdr
Copy link
Contributor Author

jsnajdr commented Feb 4, 2020

There are two things that are not addressed yet:

  • the default dependency identification / locks dependencies as optional unit test is failing. For some reason, the npm install doesn't hoist packages (a and b) to the top level of the tree and puts them into node_modules/example/node_modules/{a,b}.
  • the current code relativizes only specs that have a file: prefix. But there are other ways how to specify a file path, I assume? npa.resolve applies multiple regexp tests that lead to calling fromFile: isFilespec, isFilename, hasSlashes, ...

@jsnajdr
Copy link
Contributor Author

jsnajdr commented Feb 5, 2020

But there are other ways how to specify a file path, I assume?

I realized that this is concern only for package.json files, which support multiple ways (many of them look like legacy) to specify file links. But when writing a shrinkwrap file, all links are consistently prefixed with file:

https://github.com/npm/cli/blob/latest/lib/shrinkwrap.js#L180

@jsnajdr
Copy link
Contributor Author

jsnajdr commented Feb 5, 2020

The default dependency identification unit test is failing because it uses a different flavor of local relative file links: when one tarball requires another:

a-1.0.0.tar.gz: package/package.json

{
  "name": "a",
  "version": "1.0.0",
  "dependencies": {
    "b": "./b-1.0.0.tar.gz"
  }
}

The a tarball requires another one (a sibling) through a relative link.

Then, in a project directory, I can require a:

project/package.json

{
  "name": "project",
  "dependencies": {
    "a": "../a-1.0.0.tar.gz"
  }
}

The generated lockfile includes requires object with file: links, but my relativizeLink code doesn't work. The requested.fetchSpec of the requesting package (a) is not a directory, but a version number from the tarball's package.json, i.e., 1.0.0. I can't use it as a path. Instead, I need to use the sw.resolved field from the lockfile.

I'll need to spend some more time to make this right.

@mikemimik mikemimik added Bug thing that needs fixing Community Enhancement new feature or improvement pr: needs tests requires tests before merging semver:patch semver patch level for changes labels Feb 13, 2020
@mikemimik
Copy link
Contributor

mikemimik commented Feb 13, 2020

Hey @jsnajdr thanks so much for contributing a fix for this!

Seems like you've identified some tests that need to be cleaned up. I might suggest, if this gets a little too hairy because of the legacy changes and domino effect that could happen, we're in the process of working on npm@7 and npm/arborist is the new module to handle what I think you've found a bug in. @isaacs is this a test case that arborist handles in the way that @jscissr has outlined, or does this change also need to be applied forward.

Let us know if you need any help!

If the shrinkwrap code calls `getRequested` on an optional dependency,
the `spec` passed to `npa.resolve` is null. `npa.resolve` then thinks
it's a request `fromRegistry`, with spec defaulting to `latest`. And
in case the real spec is a tarball, returns nonsensical result where
`isRegistry` is true, `fetchSpec` is `1.0.0` instead of `file:...` and
the record written to the shrinkwrap is wrong. It contains a `resolved`
field, which should be used only for packages downloaded from the registry.
@jsnajdr
Copy link
Contributor Author

jsnajdr commented Feb 14, 2020

Seems like you've identified some tests that need to be cleaned up.

I think it's more like I discovered another bug when optional dependencies are involved. Look at the lockfile that the install-dep-classification test produces:

{
  "name": "optional",
  "version": "1.0.0",
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
    "a": {
      "version": "file:../a-1.0.0.tgz",
      "integrity": "sha512-O3nEu8XmPycocmXUJx3y506FnbeKpisziyH+w/MRhlGfnGuAdWSRnOG3/432psA6sjfoM+jgvVinAcr35GOuJA==",
      "optional": true,
      "requires": {
        "b": "file:../b-1.0.0.tgz"
      }
    },
    "b": {
      "version": "file:../b-1.0.0.tgz",
      "integrity": "sha512-F2izzZBMo5LsY6zcTDGGuF4RPcnDfOApjp/MhUYkDqgX8AYv4IpMR8dr7h0bMPDwlxuns8uK8rXzjj1PDJ5C6Q==",
      "optional": true
    },
    "example": {
      "version": "1.0.0",
      "resolved": "/Users/jsnajdr/src/npm-play/fixture/testdir/example-1.0.0.tgz",
      "integrity": "sha512-bnYxD9IzKuWKTbGN2HPJkACGncH3AgjalENYojftgP+YLi8z6S5OFAtQAII/XGcrCqOVMjVuw3D0ez+YFr/wXw==",
      "optional": true,
      "requires": {
        "a": "file:../a-1.0.0.tgz",
        "b": "file:../b-1.0.0.tgz"
      }
    }
  }
}

All three dependencies are tarballs, but example is different: version is 1.0.0 instead of file:..., and there is resolved field that points to local tarball with absolute path instead of a registry URL.

That's because example is an optional dep and these are mistreated by install/get-requested. See the fix and commit message in 83358bc for more details.

Now the tests are still failing because a repeated npm install fails to dedupe the dependencies. I'll need one more deep debugging session to address that.

@mikemimik @isaacs let me know if it's likely that this patch will be eventually merged into the 6.x line and if it's worth spending more time on.

The path in `sw.requires` is always relative to the requesting package
location, and the default from `packageRelativePath(tree)` is always correct.

For `type=file` dependencies, `tree.package._where` contains the `realpath`
of the requesting tarball, which is the location where it was unpacked in
`node_modules`. That's completely useless for resolving dependency paths.
We're interested in the source location of the packed tarball. That's the path
against which the dependency spec should be resolved.
When comparing specs like `file:../b-1.0.0.tgz`, the `rawSpec` and `saveSpec`
contain paths that are relative against different roots. We need to compare
the `fetchSpec`, too, which is absolute.
`realpath` contains the location of the unpacked tarball, and that's useless when
resolving relative dependency paths in `package.json`. For that resolution, the location
of the packed tarball is important.
@jsnajdr
Copy link
Contributor Author

jsnajdr commented Feb 17, 2020

After a few more fixes for dependencies of type file (i.e., tarballs), all tests are finally passing.

When calling npa.resolve(name, spec, where), the where parameter should never be the realpath of a requesting package. When a tarball is requested from another tarball as package.json dependency, the file: path is relative to the requesting tarball location. But realpath is the path in node_modules where the tarball was unpacked to! Never relevant for the resolution of relative file: links. packageRelativePath should be always giving the right answer here.

Another bug that needed fixing is the fact that rawSpec and saveSpec of different requests can be different, and yet point to the same tarball. One package can request it as file:./a-1.0.0.tgz, another as file:../a-1.0.0.tgz. We need to compare the fetchSpec absolute paths, too.

I think this also uncovers a preexisting bug in doesChildVersionMatch where two different tarballs with the same name, but in different directories, can be requested with the same relative spec, but resolve to different tarballs. Consider a tree like this:

{
  package: {
    name: 'app'
  },
  children: [
    {
      package: {
        name: 'a',
        rawSpec: 'file:../a-1.0.0.tgz',
        fetchSpec: '/a-1.0.0.tgz',
      }
    },
    {
      package: {
        name: 'nested',
        rawSpec: 'file:./nested'
      },
      children: [
        {
          package: {
            name: 'a',
            rawSpec: 'file:../a-1.0.0.tgz',
            fetchSpec: '/app/a-1.0.0.tgz'
          }
        }
      ]
    }
  ]
}

and on-disk directory structure like this:

/a-1.0.0.tgz
/app/
  package.json
  a-1.0.0.tgz
  nested/
    package.json

Then childReq.rawSpec === requested.rawSpec and childReq.saveSpec === requested.saveSpec will report a match that isn't true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Enhancement new feature or improvement pr: needs tests requires tests before merging semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In a monorepo with file links, npm install with lockfile fails to create some symlinks
3 participants