From 08dd4ad94579df74871ac62b7738719416a35c30 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Mon, 3 Feb 2020 09:28:20 +0100 Subject: [PATCH 1/9] fix: relativize file links when inflating shrinkwrap --- lib/install/inflate-shrinkwrap.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/install/inflate-shrinkwrap.js b/lib/install/inflate-shrinkwrap.js index 1ec4f9ba6dcfd..2a7a96b00b38b 100644 --- a/lib/install/inflate-shrinkwrap.js +++ b/lib/install/inflate-shrinkwrap.js @@ -89,6 +89,14 @@ function tarballToVersion (name, tb) { return match[2] || match[1] } +function relativizeLink( name , spec, topPath, requested) { + if (!spec.startsWith('file:')) { + return; + } + const relativized = path.relative(requested.fetchSpec, path.resolve(topPath, spec.slice(5))); + return 'file:' + relativized; +} + function inflatableChild (onDiskChild, name, topPath, tree, sw, requested, opts) { validate('OSSOOOO|ZSSOOOO', arguments) const usesIntegrity = ( @@ -101,7 +109,15 @@ function inflatableChild (onDiskChild, name, topPath, tree, sw, requested, opts) sw.resolved = sw.version sw.version = regTarball } - if (sw.requires) Object.keys(sw.requires).map(_ => { sw.requires[_] = tarballToVersion(_, sw.requires[_]) || sw.requires[_] }) + if (sw.requires) { + Object.keys(sw.requires).forEach(_ => { + sw.requires[_] = ( + tarballToVersion(_, sw.requires[_]) || + relativizeLink(_, sw.requires[_], topPath, requested) || + sw.requires[_] + ); + }); + } const modernLink = requested.type === 'directory' && !sw.from if (hasModernMeta(onDiskChild) && childIsEquivalent(sw, requested, onDiskChild)) { // The version on disk matches the shrinkwrap entry. From e453b4182932a37d05b58942b2b55c359c8b58e8 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Mon, 3 Feb 2020 12:06:06 +0100 Subject: [PATCH 2/9] fixup spaces and semicolon formatting --- lib/install/inflate-shrinkwrap.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/install/inflate-shrinkwrap.js b/lib/install/inflate-shrinkwrap.js index 2a7a96b00b38b..a424e63c7c275 100644 --- a/lib/install/inflate-shrinkwrap.js +++ b/lib/install/inflate-shrinkwrap.js @@ -89,12 +89,12 @@ function tarballToVersion (name, tb) { return match[2] || match[1] } -function relativizeLink( name , spec, topPath, requested) { +function relativizeLink (name, spec, topPath, requested) { if (!spec.startsWith('file:')) { - return; + return } - const relativized = path.relative(requested.fetchSpec, path.resolve(topPath, spec.slice(5))); - return 'file:' + relativized; + const relativized = path.relative(requested.fetchSpec, path.resolve(topPath, spec.slice(5))) + return 'file:' + relativized } function inflatableChild (onDiskChild, name, topPath, tree, sw, requested, opts) { @@ -115,8 +115,8 @@ function inflatableChild (onDiskChild, name, topPath, tree, sw, requested, opts) tarballToVersion(_, sw.requires[_]) || relativizeLink(_, sw.requires[_], topPath, requested) || sw.requires[_] - ); - }); + ) + }) } const modernLink = requested.type === 'directory' && !sw.from if (hasModernMeta(onDiskChild) && childIsEquivalent(sw, requested, onDiskChild)) { From 1a893fcdacf6468d4fd7db800599ce11bd29042f Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Tue, 4 Feb 2020 22:58:23 +0100 Subject: [PATCH 3/9] Improve variable names in the code that normalizes the specs --- lib/install/inflate-shrinkwrap.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/install/inflate-shrinkwrap.js b/lib/install/inflate-shrinkwrap.js index a424e63c7c275..32d622c3e789f 100644 --- a/lib/install/inflate-shrinkwrap.js +++ b/lib/install/inflate-shrinkwrap.js @@ -110,12 +110,11 @@ function inflatableChild (onDiskChild, name, topPath, tree, sw, requested, opts) sw.version = regTarball } if (sw.requires) { - Object.keys(sw.requires).forEach(_ => { - sw.requires[_] = ( - tarballToVersion(_, sw.requires[_]) || - relativizeLink(_, sw.requires[_], topPath, requested) || - sw.requires[_] - ) + Object.keys(sw.requires).forEach(name => { + const spec = sw.requires[name] + sw.requires[name] = tarballToVersion(name, spec) || + relativizeLink(name, spec, topPath, requested) || + spec }) } const modernLink = requested.type === 'directory' && !sw.from From 1b597b1caa24e6373fbf613101f67e45c1a2fc66 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Wed, 5 Feb 2020 14:04:32 +0100 Subject: [PATCH 4/9] Fix relativizeLink for requests from tarballs --- lib/install/inflate-shrinkwrap.js | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/install/inflate-shrinkwrap.js b/lib/install/inflate-shrinkwrap.js index 32d622c3e789f..26d5331c31178 100644 --- a/lib/install/inflate-shrinkwrap.js +++ b/lib/install/inflate-shrinkwrap.js @@ -89,11 +89,21 @@ function tarballToVersion (name, tb) { return match[2] || match[1] } -function relativizeLink (name, spec, topPath, requested) { +function relativizeLink (name, spec, topPath, sw, requested) { if (!spec.startsWith('file:')) { return } - const relativized = path.relative(requested.fetchSpec, path.resolve(topPath, spec.slice(5))) + + let requestedPath + if (requested.type === 'directory') { + requestedPath = requested.fetchSpec + } else if (requested.type === 'file') { + requestedPath = path.dirname(requested.fetchSpec) + } else { + requestedPath = sw.resolved + } + + const relativized = path.relative(requestedPath, path.resolve(topPath, spec.slice(5))) return 'file:' + relativized } @@ -113,7 +123,7 @@ function inflatableChild (onDiskChild, name, topPath, tree, sw, requested, opts) Object.keys(sw.requires).forEach(name => { const spec = sw.requires[name] sw.requires[name] = tarballToVersion(name, spec) || - relativizeLink(name, spec, topPath, requested) || + relativizeLink(name, spec, topPath, sw, requested) || spec }) } From 83358bce3b84b9fdb3e058f260bf3fdd5d602bf0 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Fri, 14 Feb 2020 09:51:53 +0100 Subject: [PATCH 5/9] getRequested: look also at optDeps when determining package spec 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. --- lib/install/get-requested.js | 4 +++- test/tap/install-dep-classification.js | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/install/get-requested.js b/lib/install/get-requested.js index ab410ffc9b6e3..b6d3968db6985 100644 --- a/lib/install/get-requested.js +++ b/lib/install/get-requested.js @@ -7,6 +7,8 @@ module.exports = function (child, reqBy) { if (!reqBy) reqBy = child.requiredBy[0] const deps = reqBy.package.dependencies || {} const devDeps = reqBy.package.devDependencies || {} + const optDeps = reqBy.package.optionalDependencies || {} const name = moduleName(child) - return npa.resolve(name, deps[name] || devDeps[name], reqBy.realpath) + const spec = deps[name] || devDeps[name] || optDeps[name] + return npa.resolve(name, spec, reqBy.realpath) } diff --git a/test/tap/install-dep-classification.js b/test/tap/install-dep-classification.js index 257fc99fc123f..1c9995cedc8a0 100644 --- a/test/tap/install-dep-classification.js +++ b/test/tap/install-dep-classification.js @@ -126,7 +126,7 @@ test('optional dependency identification', function (t) { optional: true }, example: { - version: '1.0.0', + version: 'file:../example-1.0.0.tgz', optional: true } } @@ -150,7 +150,7 @@ test('development dependency identification', function (t) { dev: true }, example: { - version: '1.0.0', + version: 'file:../example-1.0.0.tgz', dev: true } } @@ -173,7 +173,7 @@ test('default dependency identification', function (t) { optional: true }, example: { - version: '1.0.0', + version: 'file:../example-1.0.0.tgz', optional: true } } From ce2936f810fa6f8940229e1775551d338cac1017 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Fri, 14 Feb 2020 12:11:34 +0100 Subject: [PATCH 6/9] Simplify relativizeLink now it doesn't need to look at sw.resolved --- lib/install/inflate-shrinkwrap.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/install/inflate-shrinkwrap.js b/lib/install/inflate-shrinkwrap.js index 26d5331c31178..122068c201287 100644 --- a/lib/install/inflate-shrinkwrap.js +++ b/lib/install/inflate-shrinkwrap.js @@ -89,18 +89,14 @@ function tarballToVersion (name, tb) { return match[2] || match[1] } -function relativizeLink (name, spec, topPath, sw, requested) { +function relativizeLink (name, spec, topPath, requested) { if (!spec.startsWith('file:')) { return } - let requestedPath - if (requested.type === 'directory') { - requestedPath = requested.fetchSpec - } else if (requested.type === 'file') { - requestedPath = path.dirname(requested.fetchSpec) - } else { - requestedPath = sw.resolved + let requestedPath = requested.fetchSpec + if (requested.type === 'file') { + requestedPath = path.dirname(requestedPath) } const relativized = path.relative(requestedPath, path.resolve(topPath, spec.slice(5))) @@ -123,7 +119,7 @@ function inflatableChild (onDiskChild, name, topPath, tree, sw, requested, opts) Object.keys(sw.requires).forEach(name => { const spec = sw.requires[name] sw.requires[name] = tarballToVersion(name, spec) || - relativizeLink(name, spec, topPath, sw, requested) || + relativizeLink(name, spec, topPath, requested) || spec }) } From b8a5e052577374ff6cc5cf6e83946048c3a9b9ea Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Mon, 17 Feb 2020 10:11:43 +0100 Subject: [PATCH 7/9] Don't use tree.package._where when resolving shrinkwrap dependency 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. --- lib/install/deps.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/install/deps.js b/lib/install/deps.js index 3d8b333c64441..934ccdb62f85d 100644 --- a/lib/install/deps.js +++ b/lib/install/deps.js @@ -570,7 +570,7 @@ function addDependency (name, versionSpec, tree, log, done) { try { var req = childDependencySpecifier(tree, name, versionSpec) if (tree.swRequires && tree.swRequires[name]) { - var swReq = childDependencySpecifier(tree, name, tree.swRequires[name], tree.package._where) + var swReq = childDependencySpecifier(tree, name, tree.swRequires[name]) } } catch (err) { return done(err) From 3963e5ac53653291777876bc16e73980786eeb0d Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Mon, 17 Feb 2020 10:15:59 +0100 Subject: [PATCH 8/9] doesChildVersionMatch: take fetchSpec into account 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. --- lib/install/deps.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/install/deps.js b/lib/install/deps.js index 934ccdb62f85d..f7dba8f10a3bc 100644 --- a/lib/install/deps.js +++ b/lib/install/deps.js @@ -74,7 +74,10 @@ function doesChildVersionMatch (child, requested, requestor) { var childReq = child.package._requested if (childReq) { if (childReq.rawSpec === requested.rawSpec) return true - if (childReq.type === requested.type && childReq.saveSpec === requested.saveSpec) return true + if (childReq.type === requested.type) { + if (childReq.saveSpec === requested.saveSpec) return true + if (childReq.fetchSpec === requested.fetchSpec) return true + } } // If _requested didn't exist OR if it didn't match then we'll try using // _from. We pass it through npa to normalize the specifier. From e8ab3630d338ee223568869dedbbda05768c1d89 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Mon, 17 Feb 2020 10:54:37 +0100 Subject: [PATCH 9/9] getRequested: don't use reqBy.realpath when resolving dependency paths `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. --- lib/install/deps.js | 1 + lib/install/get-requested.js | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/install/deps.js b/lib/install/deps.js index f7dba8f10a3bc..72c7192963000 100644 --- a/lib/install/deps.js +++ b/lib/install/deps.js @@ -203,6 +203,7 @@ function removeObsoleteDep (child, log) { }) } +exports.packageRelativePath = packageRelativePath function packageRelativePath (tree) { if (!tree) return '' var requested = tree.package._requested || {} diff --git a/lib/install/get-requested.js b/lib/install/get-requested.js index b6d3968db6985..df0eec6f6a631 100644 --- a/lib/install/get-requested.js +++ b/lib/install/get-requested.js @@ -1,7 +1,7 @@ 'use strict' const npa = require('npm-package-arg') const moduleName = require('../utils/module-name.js') - +const packageRelativePath = require('./deps').packageRelativePath module.exports = function (child, reqBy) { if (!child.requiredBy.length) return if (!reqBy) reqBy = child.requiredBy[0] @@ -10,5 +10,6 @@ module.exports = function (child, reqBy) { const optDeps = reqBy.package.optionalDependencies || {} const name = moduleName(child) const spec = deps[name] || devDeps[name] || optDeps[name] - return npa.resolve(name, spec, reqBy.realpath) + const where = packageRelativePath(reqBy) + return npa.resolve(name, spec, where) }