From 65def1caaa1476ff3795470a7ddb713a12376db4 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 29 Jan 2019 16:03:02 +0000 Subject: [PATCH 1/3] Check os and platform even when engines is not present in package.json --- CHANGELOG.md | 4 ++ __tests__/package-compatibility.js | 66 ++++++++++++++++++- .../pkg-tests-specs/sources/basic.js | 64 ++++++++++++++++++ src/cli/commands/install.js | 2 +- src/package-compatibility.js | 31 ++++++++- 5 files changed, 162 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5287a8ae5b..df57eeae7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,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) +- Check `os` and `platform` requirements from `package.json` even when the package does not specify any `engines` requirements + + [#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 index 78eeef570e..c681c398fd 100644 --- 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 index 8ad67bfc99..46de091cee 100644 --- 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 index e72e2b6924..33db9edee5 100644 --- a/src/cli/commands/install.js +++ b/src/cli/commands/install.js @@ -572,7 +572,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 index 5dbe96f7a2..d6eb49799b 100644 --- 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; + function isValid(items: Array, actual: string): boolean { let isNotWhitelist = true; let isBlacklist = false; @@ -127,19 +129,19 @@ export function checkOne(info: Manifest, config: Config, ignoreEngines: boolean) }; const invalidPlatform = - !config.ignorePlatform && Array.isArray(info.os) && info.os.length > 0 && !isValidPlatform(info.os); + shouldCheckPlatform(info, config.ignorePlatform) && info.os != null && !isValidPlatform(info.os); if (invalidPlatform) { pushError(reporter.lang('incompatibleOS', process.platform)); } - const invalidCpu = !config.ignorePlatform && Array.isArray(info.cpu) && info.cpu.length > 0 && !isValidArch(info.cpu); + const invalidCpu = shouldCheckCpu(info, config.ignorePlatform) && info.cpu != null && !isValidArch(info.cpu); if (invalidCpu) { pushError(reporter.lang('incompatibleCPU', process.arch)); } - if (!ignoreEngines && typeof info.engines === 'object') { + if (shouldCheckEngines(info, ignoreEngines)) { for (const entry of entries(info.engines)) { let name = entry[0]; const range = entry[1]; @@ -168,3 +170,26 @@ export function check(infos: Array, config: Config, ignoreEngines: boo checkOne(info, config, ignoreEngines); } } + +function shouldCheckCpu(manifest: PartialManifest, ignorePlatform: boolean): boolean { + return !ignorePlatform && Array.isArray(manifest.cpu) && manifest.cpu.length > 0; +} + +function shouldCheckPlatform(manifest: PartialManifest, ignorePlatform: boolean): boolean { + return !ignorePlatform && Array.isArray(manifest.os) && manifest.os.length > 0; +} + +function shouldCheckEngines(manifest: PartialManifest, ignoreEngines: boolean): boolean { + return !ignoreEngines && typeof manifest.engines === 'object'; +} + +export function shouldCheck( + manifest: PartialManifest, + options: {ignoreEngines: boolean, ignorePlatform: boolean}, +): boolean { + return ( + shouldCheckCpu(manifest, options.ignorePlatform) || + shouldCheckPlatform(manifest, options.ignorePlatform) || + shouldCheckEngines(manifest, options.ignoreEngines) + ); +} From 3bf913c8b772675ed1a9571ba76c29d333caeb0f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 31 Jan 2019 08:56:26 +0000 Subject: [PATCH 2/3] Use %checks to remove unnecessary null check --- src/package-compatibility.js | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/package-compatibility.js b/src/package-compatibility.js index d6eb49799b..7ba68615d3 100644 --- a/src/package-compatibility.js +++ b/src/package-compatibility.js @@ -128,20 +128,17 @@ export function checkOne(info: Manifest, config: Config, ignoreEngines: boolean) } }; - const invalidPlatform = - shouldCheckPlatform(info, config.ignorePlatform) && info.os != null && !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 = shouldCheckCpu(info, config.ignorePlatform) && info.cpu != null && !isValidArch(info.cpu); - - if (invalidCpu) { + if (shouldCheckCpu(cpu, config.ignorePlatform) && !isValidArch(cpu)) { pushError(reporter.lang('incompatibleCPU', process.arch)); } - if (shouldCheckEngines(info, ignoreEngines)) { + if (shouldCheckEngines(engines, ignoreEngines)) { for (const entry of entries(info.engines)) { let name = entry[0]; const range = entry[1]; @@ -171,16 +168,16 @@ export function check(infos: Array, config: Config, ignoreEngines: boo } } -function shouldCheckCpu(manifest: PartialManifest, ignorePlatform: boolean): boolean { - return !ignorePlatform && Array.isArray(manifest.cpu) && manifest.cpu.length > 0; +function shouldCheckCpu(cpu: $PropertyType, ignorePlatform: boolean): boolean %checks { + return !ignorePlatform && Array.isArray(cpu) && cpu.length > 0; } -function shouldCheckPlatform(manifest: PartialManifest, ignorePlatform: boolean): boolean { - return !ignorePlatform && Array.isArray(manifest.os) && manifest.os.length > 0; +function shouldCheckPlatform(os: $PropertyType, ignorePlatform: boolean): boolean %checks { + return !ignorePlatform && Array.isArray(os) && os.length > 0; } -function shouldCheckEngines(manifest: PartialManifest, ignoreEngines: boolean): boolean { - return !ignoreEngines && typeof manifest.engines === 'object'; +function shouldCheckEngines(engines: $PropertyType, ignoreEngines: boolean): boolean %checks { + return !ignoreEngines && typeof engines === 'object'; } export function shouldCheck( @@ -188,8 +185,8 @@ export function shouldCheck( options: {ignoreEngines: boolean, ignorePlatform: boolean}, ): boolean { return ( - shouldCheckCpu(manifest, options.ignorePlatform) || - shouldCheckPlatform(manifest, options.ignorePlatform) || - shouldCheckEngines(manifest, options.ignoreEngines) + shouldCheckCpu(manifest.cpu, options.ignorePlatform) || + shouldCheckPlatform(manifest.os, options.ignorePlatform) || + shouldCheckEngines(manifest.engines, options.ignoreEngines) ); } From 2a56795b9be2b0d51735ce27ac36cc75ac6adf1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Fri, 1 Feb 2019 10:57:37 +0000 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df57eeae7c..0039b891dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,7 @@ 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) -- Check `os` and `platform` requirements from `package.json` even when the package does not specify any `engines` requirements +- 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)