Check engine and platform when building ideal tree

lib/arborist/build-ideal-tree.js

@@ -18,6 +18,7 @@ const Link = require('../link.js')
 const addRmPkgDeps = require('../add-rm-pkg-deps.js')
 const gatherDepSet = require('../gather-dep-set.js')
 const optionalSet = require('../optional-set.js')
+const {checkEngine, checkPlatform} = require('npm-install-checks')
 
 // enum of return values for canPlaceDep.
 // No, this is a conflict, you may not put that package here
@@ -87,6 +88,9 @@ const _followSymlinkPath = Symbol('followSymlinkPath')
 const _getRelpathSpec = Symbol('getRelpathSpec')
 const _retrieveSpecName = Symbol('retrieveSpecName')
 const _strictPeerDeps = Symbol('strictPeerDeps')
+const _checkEngineAndPlatform = Symbol('checkEngineAndPlatform')
+const _checkEngine = Symbol('checkEngine')
+const _checkPlatform = Symbol('checkPlatform')
 
 // used for the ERESOLVE error to show the last peer conflict encountered
 const _peerConflict = Symbol('peerConflict')
@@ -190,6 +194,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
       await this[_buildDeps]()
       await this[_fixDepFlags]()
       await this[_pruneFailedOptional]()
+      await this[_checkEngineAndPlatform]()
     } finally {
       process.emit('timeEnd', 'idealTree')
       this.finishTracker('idealTree')
@@ -198,10 +203,46 @@ module.exports = cls => class IdealTreeBuilder extends cls {
     return this.idealTree
   }
 
+  [_checkEngineAndPlatform] () {
+    // engine/platform checks throw, so start the promise chain off first
+    return Promise.resolve()
+      .then(() => {
+        for (const node of this.idealTree.inventory.values()) {
+          if (!node.optional) {
+            this[_checkEngine](node)
+            this[_checkPlatform](node)
+          }
+        }
+      })
+  }
+
+  [_checkPlatform] (node) {
+    checkPlatform(node.package, this[_force])
+  }
+
+  [_checkEngine] (node) {
+    const { engineStrict, npmVersion, nodeVersion } = this.options
+    const c = () => checkEngine(node.package, npmVersion, nodeVersion, this[_force])
+
+    if (engineStrict)
+    c()
+    else {
+      try {
+        c()
+      } catch (er) {
+        this.log.warn(er.code, er.message, {
+          package: er.pkgid,
+          required: er.required,
+          current: er.current,
+        })
+      }
+    }
+  }
+
   [_parseSettings] (options) {
     const update = options.update === true ? { all: true }
       : Array.isArray(options.update) ? { names: options.update }
-      : options.update || {}
+        : options.update || {}
 
     if (update.all || !Array.isArray(update.names))
       update.names = []

lib/arborist/reify.js

@@ -3,7 +3,6 @@
 const npa = require('npm-package-arg')
 const pacote = require('pacote')
 const rpj = require('read-package-json-fast')
-const {checkEngine, checkPlatform} = require('npm-install-checks')
 const { orderDeps, updateDepSpec } = require('../dep-spec.js')
 const AuditReport = require('../audit-report.js')
 
@@ -42,13 +41,10 @@ const _diffTrees = Symbol.for('diffTrees')
 const _createSparseTree = Symbol.for('createSparseTree')
 const _loadShrinkwrapsAndUpdateTrees = Symbol.for('loadShrinkwrapsAndUpdateTrees')
 const _reifyNode = Symbol.for('reifyNode')
-const _checkEngineAndPlatform = Symbol('checkEngineAndPlatform')
 const _extractOrLink = Symbol('extractOrLink')
 const _symlink = Symbol('symlink')
 const _warnDeprecated = Symbol('warnDeprecated')
-const _recheckEngineAndPlatform = Symbol('recheckEngineAndPlatform')
-const _checkEngine = Symbol('checkEngine')
-const _checkPlatform = Symbol('checkPlatform')
+const _loadAncientPackageDetails = Symbol('loadAncientPackageDetails')
 const _loadBundlesAndUpdateTrees = Symbol.for('loadBundlesAndUpdateTrees')
 const _submitQuickAudit = Symbol('submitQuickAudit')
 const _awaitQuickAudit = Symbol('awaitQuickAudit')
@@ -380,10 +376,10 @@ module.exports = cls => class Reifier extends cls {
     process.emit('time', `reifyNode:${node.location}`)
     this.addTracker('reify', node.name, node.location)
 
-    const p = this[_checkEngineAndPlatform](node)
+    const p = Promise.resolve()
       .then(() => this[_extractOrLink](node))
       .then(() => this[_warnDeprecated](node))
-      .then(() => this[_recheckEngineAndPlatform](node))
+      .then(() => this[_loadAncientPackageDetails](node))
 
     return this[_handleOptionalFailure](node, p)
       .then(() => {
@@ -437,10 +433,9 @@ module.exports = cls => class Reifier extends cls {
       this.log.warn('deprecated', `${_id}: ${deprecated}`)
   }
 
-  [_recheckEngineAndPlatform] (node) {
-    // If we're loading from a v1 lockfile, then need to do this again later
-    // after reading from the disk.  Also grab the bin, because old lockfiles
-    // did not track that useful bit of info.
+  [_loadAncientPackageDetails] (node) {
+    // If we're loading from a v1 lockfile, load details from the package.json
+    // that weren't recorded in the old format.
     const {meta} = this.idealTree
     const ancient = meta.ancientLockfile
     const old = meta.loadedFromDisk && !(meta.originalLockfileVersion >= 2)
@@ -454,42 +449,10 @@ module.exports = cls => class Reifier extends cls {
         node.package.cpu = pkg.cpu
         node.package.engines = pkg.engines
         meta.add(node)
-        return this[_checkEngineAndPlatform](node)
       })
     }
   }
 
-  [_checkEngineAndPlatform] (node) {
-    // engine/platform checks throw, so start the promise chain off first
-    return Promise.resolve()
-      .then(() => this[_checkEngine](node))
-      .then(() => this[_checkPlatform](node))
-  }
-
-
-  [_checkPlatform] (node) {
-    checkPlatform(node.package, this[_force])
-  }
-
-  [_checkEngine] (node) {
-    const { engineStrict, npmVersion, nodeVersion } = this.options
-    const c = () => checkEngine(node.package, npmVersion, nodeVersion, this[_force])
-
-    if (engineStrict)
-      c()
-    else {
-      try {
-        c()
-      } catch (er) {
-        this.log.warn(er.code, er.message, {
-          package: er.pkgid,
-          required: er.required,
-          current: er.current,
-        })
-      }
-    }
-  }
-
   // if the node is optional, then the failure of the promise is nonfatal
   // just add it and its optional set to the trash list.
   [_handleOptionalFailure] (node, p) {

test/arborist/build-ideal-tree.js

@@ -101,7 +101,55 @@ const printIdeal = (path, opt) => buildIdeal(path, opt).then(printTree)
 
 const OPT = { cache, registry }
 const buildIdeal = (path, opt) =>
-  new Arborist({...OPT, path, ...(opt || {})}).buildIdealTree(opt)
+  new Arborist({ ...OPT, path, ...(opt || {}) }).buildIdealTree(opt)
+
+t.test('fail on mismatched engine when engineStrict is set', async t => {
+  const path = resolve(fixtures, 'engine-specification')
+
+  t.rejects(buildIdeal(path, {
+    ...OPT,
+    nodeVersion: '12.18.4',
+    engineStrict: true,
+  }).then(() => { throw new Error('failed to fail') }), { code: 'EBADENGINE' })
+})
+
+t.test('ignore mismatched engine for optional dependencies', async t => {
+  const path = resolve(fixtures, 'optional-engine-specification')
+  await buildIdeal(path, {
+    ...OPT,
+    nodeVersion: '12.18.4',
+    engineStrict: true,
+  })
+})
+
+t.test('warn on mismatched engine when engineStrict is false', t => {
+  const path = resolve(fixtures, 'engine-specification')
+  const check = warningTracker()
+  return buildIdeal(path, {
+    ...OPT,
+    nodeVersion: '12.18.4',
+    engineStrict: false,
+  }).then(() => t.match(check(), [
+    ['warn', 'EBADENGINE'],
+  ]))
+})
+
+t.test('fail on mismatched platform', async t => {
+  const path = resolve(fixtures, 'platform-specification')
+  t.rejects(buildIdeal(path, {
+    ...OPT,
+    nodeVersion: '4.0.0'
+  }).then(() => { throw new Error('failed to fail') }), { code: 'EBADPLATFORM' })
+})
+
+t.test('ignore mismatched platform for optional dependencies', async t => {
+  const path = resolve(fixtures, 'optional-platform-specification')
+  await buildIdeal(path, {
+    ...OPT,
+    nodeVersion: '12.18.4',
+    engineStrict: true,
+  })
+})
 
 t.test('no options', t => {
   const arb = new Arborist()

test/arborist/reify.js

@@ -516,29 +516,6 @@ t.test('link metadep', t => {
     t.resolveMatchSnapshot(printReified(fixture(t, c)))))
 })
 
-t.test('fail on mismatched engine when engineStrict is set', t =>
-  t.rejects(printReified(fixture(t, 'tap-and-flow'), {
-    nodeVersion: '1.2.3',
-    engineStrict: true,
-  }).then(() => { throw new Error('failed to fail') }), { code: 'EBADENGINE' }))
-
-t.test('warn on mismatched engine when engineStrict is false', t => {
-  const path = fixture(t, 'tap-and-flow')
-  const a = newArb({
-    path,
-    engineStrict: false,
-    nodeVersion: '1.2.3',
-    // just to add coverage for the no-op function for bundleBinLinks
-    binLinks: false,
-  })
-  const check = warningTracker()
-  const binPath = `${path}/node_modules/tap/node_modules/.bin`
-  return a.reify().then(() => t.match(check(), [
-    ['warn', 'EBADENGINE'],
-  ]))
-  .then(() => t.throws(() => fs.readdir(binPath)))
-})
-
 t.test('warn on reifying deprecated dependency', t => {
   const a = newArb({
     path: fixture(t, 'deprecated-dep'),

test/fixtures/engine-specification/package.json

@@ -0,0 +1,14 @@
+{
+  "name": "engine-platform-test",
+  "version": "1.0.0",
+  "description": "",
+  "main": "index.js",
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1"
+  },
+  "author": "",
+  "license": "ISC",
+  "dependencies": {
+    "engine-specifying-test-package": "^1.0.0"
+  }
+}

test/fixtures/optional-engine-specification/package.json

@@ -0,0 +1,14 @@
+{
+  "name": "engine-platform-test",
+  "version": "1.0.0",
+  "description": "",
+  "main": "index.js",
+  "scripts": {
+    "test": "echo \"Error: no test specified\" && exit 1"
+  },
+  "author": "",
+  "license": "ISC",
+  "optionalDependencies": {
+    "engine-specifying-test-package": "^1.0.0"
+  }
+}