Check os and platform even when engines is not present in package.json (

#6976) * Check os and platform even when engines is not present in package.json * Use %checks to remove unnecessary null check * Update CHANGELOG.md
yarnpkg · Feb 1, 2019 · 65e198d · 65e198d
1 parent af91e7c
commit 65e198d
Show file tree

Hide file tree

Showing 5 changed files with 163 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -46,6 +46,10 @@ Please add one entry in this file for each change in Yarn's behavior. Use the sa
 
   [#6942](https://github.com/yarnpkg/yarn/pull/6942) - [**John-David Dalton**](https://twitter.com/jdalton)
 
+- Fixes a bug where `os` and `platform` requirements weren't properly checked when `engines` was missing
+
+  [#6976](https://github.com/yarnpkg/yarn/pull/6976) - [**Micha Reiser**](https://github.com/MichaReiser)  
+
 ## 1.13.0
 
 - Implements a new `package.json` field: `peerDependenciesMeta`

diff --git a/__tests__/package-compatibility.js b/__tests__/package-compatibility.js
@@ -1,6 +1,6 @@
 /* @flow */
 
-import {testEngine} from '../src/package-compatibility.js';
+import {testEngine, shouldCheck} from '../src/package-compatibility.js';
 
 test('node semver semantics', () => {
   expect(testEngine('node', '^5.0.0', {node: '5.1.0'}, true)).toEqual(true);
@@ -19,3 +19,67 @@ test('node semver semantics', () => {
 test('ignore semver prerelease semantics for yarn', () => {
   expect(testEngine('yarn', '^1.3.0', {yarn: '1.4.1-20180208.2355'}, true)).toEqual(true);
 });
+
+test('shouldCheck returns true if ignorePlatform is false and the manifest specifies an os or cpu requirement', () => {
+  expect(
+    shouldCheck(
+      {
+        os: ['darwin'],
+      },
+      {ignorePlatform: false, ignoreEngines: false},
+    ),
+  ).toBe(true);
+
+  expect(
+    shouldCheck(
+      {
+        cpu: ['i32'],
+      },
+      {ignorePlatform: false, ignoreEngines: false},
+    ),
+  ).toBe(true);
+
+  expect(shouldCheck({}, {ignorePlatform: false, ignoreEngines: false})).toBe(false);
+
+  expect(
+    shouldCheck(
+      {
+        os: [],
+        cpu: [],
+      },
+      {ignorePlatform: false, ignoreEngines: false},
+    ),
+  ).toBe(false);
+
+  expect(
+    shouldCheck(
+      {
+        cpu: ['i32'],
+        os: ['darwin'],
+      },
+      {ignorePlatform: true, ignoreEngines: false},
+    ),
+  ).toBe(false);
+});
+
+test('shouldCheck returns true if ignoreEngines is false and the manifest specifies engines', () => {
+  expect(
+    shouldCheck(
+      {
+        engines: {node: '>= 10'},
+      },
+      {ignorePlatform: false, ignoreEngines: false},
+    ),
+  ).toBe(true);
+
+  expect(shouldCheck({}, {ignorePlatform: false, ignoreEngines: false})).toBe(false);
+
+  expect(
+    shouldCheck(
+      {
+        engines: {node: '>= 10'},
+      },
+      {ignorePlatform: false, ignoreEngines: true},
+    ),
+  ).toBe(false);
+});
diff --git a/packages/pkg-tests/pkg-tests-specs/sources/basic.js b/packages/pkg-tests/pkg-tests-specs/sources/basic.js
@@ -380,5 +380,69 @@ module.exports = (makeTemporaryEnv: PackageDriver) => {
         },
       ),
     );
+
+    test(
+      `it should fail if the environment does not satisfy the os platform`,
+      makeTemporaryEnv(
+        {
+          os: ['unicorn'],
+        },
+        async ({path, run, source}) => {
+          await expect(run(`install`)).rejects.toThrow(/The platform "\w+" is incompatible with this module\./);
+        },
+      ),
+    );
+
+    test(
+      `it should fail if the environment does not satisfy the cpu architecture`,
+      makeTemporaryEnv(
+        {
+          cpu: ['unicorn'],
+        },
+        async ({path, run, source}) => {
+          await expect(run(`install`)).rejects.toThrow(/The CPU architecture "\w+" is incompatible with this module\./);
+        },
+      ),
+    );
+
+    test(
+      `it should fail if the environment does not satisfy the engine requirements`,
+      makeTemporaryEnv(
+        {
+          engines: {
+            node: "0.18.1"
+          }
+        },
+        async ({path, run, source}) => {
+          await expect(run(`install`)).rejects.toThrow(/The engine "node" is incompatible with this module\. Expected version "0.18.1"./);
+        },
+      ),
+    );
+
+    test(
+      `it should not fail if the environment does not satisfy the os and cpu architecture but ignore platform is true`,
+      makeTemporaryEnv(
+        {
+          os: ['unicorn'],
+        },
+        async ({path, run, source}) => {
+          await run(`install`, '--ignore-platform');
+        },
+      ),
+    );
+
+    test(
+      `it should not fail if the environment does not satisfy the engine requirements but ignore engines is true`,
+      makeTemporaryEnv(
+        {
+          engines: {
+            node: "0.18.1"
+          }
+        },
+        async ({path, run, source}) => {
+          await run(`install`, '--ignore-engines');
+        },
+      ),
+    );
   });
 };
diff --git a/src/cli/commands/install.js b/src/cli/commands/install.js
@@ -571,7 +571,7 @@ export class Install {
       this.scripts.setArtifacts(artifacts);
     }
 
-    if (!this.flags.ignoreEngines && typeof manifest.engines === 'object') {
+    if (compatibility.shouldCheck(manifest, this.flags)) {
       steps.push(async (curr: number, total: number) => {
         this.reporter.step(curr, total, this.reporter.lang('checkingManifest'), emoji.get('mag'));
         await this.checkCompatibility();

diff --git a/src/package-compatibility.js b/src/package-compatibility.js
@@ -15,6 +15,8 @@ const VERSIONS = Object.assign({}, process.versions, {
   yarn: yarnVersion,
 });
 
+type PartialManifest = $Shape<Manifest>;
+
 function isValid(items: Array<string>, actual: string): boolean {
   let isNotWhitelist = true;
   let isBlacklist = false;
@@ -126,20 +128,17 @@ export function checkOne(info: Manifest, config: Config, ignoreEngines: boolean)
     }
   };
 
-  const invalidPlatform =
-    !config.ignorePlatform && Array.isArray(info.os) && info.os.length > 0 && !isValidPlatform(info.os);
+  const {os, cpu, engines} = info;
 
-  if (invalidPlatform) {
+  if (shouldCheckPlatform(os, config.ignorePlatform) && !isValidPlatform(os)) {
     pushError(reporter.lang('incompatibleOS', process.platform));
   }
 
-  const invalidCpu = !config.ignorePlatform && Array.isArray(info.cpu) && info.cpu.length > 0 && !isValidArch(info.cpu);
-
-  if (invalidCpu) {
+  if (shouldCheckCpu(cpu, config.ignorePlatform) && !isValidArch(cpu)) {
     pushError(reporter.lang('incompatibleCPU', process.arch));
   }
 
-  if (!ignoreEngines && typeof info.engines === 'object') {
+  if (shouldCheckEngines(engines, ignoreEngines)) {
     for (const entry of entries(info.engines)) {
       let name = entry[0];
       const range = entry[1];
@@ -168,3 +167,26 @@ export function check(infos: Array<Manifest>, config: Config, ignoreEngines: boo
     checkOne(info, config, ignoreEngines);
   }
 }
+
+function shouldCheckCpu(cpu: $PropertyType<Manifest, 'cpu'>, ignorePlatform: boolean): boolean %checks {
+  return !ignorePlatform && Array.isArray(cpu) && cpu.length > 0;
+}
+
+function shouldCheckPlatform(os: $PropertyType<Manifest, 'os'>, ignorePlatform: boolean): boolean %checks {
+  return !ignorePlatform && Array.isArray(os) && os.length > 0;
+}
+
+function shouldCheckEngines(engines: $PropertyType<Manifest, 'engines'>, ignoreEngines: boolean): boolean %checks {
+  return !ignoreEngines && typeof engines === 'object';
+}
+
+export function shouldCheck(
+  manifest: PartialManifest,
+  options: {ignoreEngines: boolean, ignorePlatform: boolean},
+): boolean {
+  return (
+    shouldCheckCpu(manifest.cpu, options.ignorePlatform) ||
+    shouldCheckPlatform(manifest.os, options.ignorePlatform) ||
+    shouldCheckEngines(manifest.engines, options.ignoreEngines)
+  );
+}