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

feat(run): Add engines check before executing scripts. #7021

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@alloy
Copy link

alloy commented Feb 10, 2019

Closes #7013

Summary

A more elaborate description is available in #7013. In short, ensuring scripts are ran in the expected environment can deter hard to diagnose bugs.

Test plan

Given a node engine requirement that doesn’t match the current environment:

$ cat package.json | grep -i1 '"node"'
  "engines": {
    "node": "^10.13.0"
  },

$ nvm use 8
Now using node v8.12.0 (npm v6.4.1)

A script invocation will fail as follows:

$ node ~/Code/JavaScript/yarn/lib/cli/index.js run relay
yarn run v1.15.0-0
error @artsy/reaction@12.1.10: The engine "node" is incompatible with this module. Expected version "^10.13.0". Got "8.12.0"
error Commands cannot run with an incompatible environment.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

If need be, this can be overridden with the --ignore-engines flag:

$ node ~/Code/JavaScript/yarn/lib/cli/index.js run --ignore-engines relay
yarn run v1.15.0-0
$ relay-compiler --src ./src --schema data/schema.graphql --language typescript --artifactDirectory ./src/__generated__ --exclude '**/node_modules/**,**/__mocks__/**,**/__generated__/**'
✨  Done in 2.22s.

Or, of course, making the environment match the requirements:

$ nvm use 10.13
Now using node v10.13.0 (npm v6.4.1)

$ node ~/Code/JavaScript/yarn/lib/cli/index.js run relay
yarn run v1.15.0-0
$ relay-compiler --src ./src --schema data/schema.graphql --language typescript --artifactDirectory ./src/__generated__ --exclude '**/node_modules/**,**/__mocks__/**,**/__generated__/**'
✨  Done in 1.91s.
@@ -229,11 +229,12 @@ const messages = {
nodeGypAutoInstallFailed:
'Failed to auto-install node-gyp. Please run "yarn global add node-gyp" manually. Error: $0',

foundIncompatible: 'Found incompatible module',
foundIncompatible: 'Found incompatible module.',

This comment has been minimized.

@alloy

alloy Feb 10, 2019

Author

Looks like this was the odd one out, so added some punctuation 🤓

@buildsize

This comment has been minimized.

Copy link

buildsize bot commented Feb 10, 2019

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.11 MB 1.11 MB 793 bytes (0%)
yarn-[version].js 4.47 MB 4.47 MB 832 bytes (0%)
yarn-legacy-[version].js 4.66 MB 4.66 MB 830 bytes (0%)
yarn-v[version].tar.gz 1.12 MB 1.12 MB 140 bytes (0%)
yarn_[version]all.deb 815.64 KB 815.73 KB 92 bytes (0%)
@alloy

This comment has been minimized.

Copy link
Author

alloy commented Feb 10, 2019

I'm unsure why Flow fails. My changes are in that file, but otherwise they seem unrelated, to my naive Flow eyes.

try {
await checkCompatibility({_reference: {}, ...pkg}, config, ignoreEngines);
} catch (err) {
throw err instanceof MessageError ? new MessageError(reporter.lang('cannotRunWithIncompatibleEnv')) : err;

This comment has been minimized.

@alloy

alloy Feb 10, 2019

Author

This refines a, in this context, vague error.

@rally25rs

This comment has been minimized.

Copy link
Contributor

rally25rs commented Feb 10, 2019

I put these same comments in Discord chat, but I'll paste them here too for tracking with this PR:

wow, I don't know how it came up with that error message for that line, but...
it's specifically mad about:

await checkCompatibility({_reference: {}, ...pkg}, config, ignoreEngines);

you are assigning _reference: {} but that isn't valid.

Function signature is: checkCompatability(Manifest, ...
and a Manifest's _reference is of type PackageReference

{} isn't a valid PackageReference because that type has several required fields

  requests: Array<PackageRequest>;
  lockfile: Lockfile;
  config: Config;

  isPlugnplay: boolean;
  level: number;
  name: string;
  version: string;
  uid: string;
  ignore: boolean;
  incompatible: boolean;
  fresh: boolean;
  dependencies: Array<string>;
  patterns: Array<string>;
  permissions: {[key: string]: boolean};
  remote: PackageRemote;
  registry: RegistryNames;
  locations: Array<string>;
  resolver: PackageResolver;
@alloy

This comment has been minimized.

Copy link
Author

alloy commented Feb 11, 2019

wow, I don't know how it came up with that error message for that line, but...

Wow, indeed! I don’t even understand how you were able know what it was really complaining about 😅

it's specifically mad about:

await checkCompatibility({_reference: {}, ...pkg}, config, ignoreEngines);

you are assigning _reference: {} but that isn't valid.

I see. At first I just passed pkg, but then at runtime it was complaining about missing _reference. Then I saw that the install command does this so I cargo-culted that.

Based on that, clearly the reference isn’t actually required, so I’m going to remove the invariant call.

@alloy alloy force-pushed the alloy:check-compatibility-before-run branch from 4eb6391 to 135863e Feb 11, 2019

@rally25rs

This comment has been minimized.

Copy link
Contributor

rally25rs commented Feb 11, 2019

package-compatability.js

export function checkOne(info: Manifest, config: Config, ignoreEngines: boolean) {
...
  const pushError = msg => {
    const ref = info._reference;
    invariant(ref, 'expected package reference');

    if (ref.optional) {

if you remove the invariant then the if could exception if ref is undefined, hence the invariant.

You'll probably have to change theif to

if (ref && ref.optional) {

for Flow to let it pass. I think that makes more sense than the invariant anyway, since Manifest._reference is nullable

  _reference?: ?PackageReference,

The code should work whether or not _reference is undefined. 👍

@alloy

This comment has been minimized.

Copy link
Author

alloy commented Feb 11, 2019

@rally25rs Yup, I added that guard, but I squashed my commits so maybe it's not clear that I pushed new changes. Sorry about that.

@alloy

This comment has been minimized.

Copy link
Author

alloy commented Feb 15, 2019

@rally25rs bump, can I get another review? 🙏

@@ -744,7 +744,7 @@ export class Install {

async checkCompatibility(): Promise<void> {
const {manifest} = await this.fetchRequestFromCwd();
await compatibility.checkOne({_reference: {}, ...manifest}, this.config, this.flags.ignoreEngines);
await compatibility.checkOne(manifest, this.config, this.flags.ignoreEngines);

This comment has been minimized.

@rally25rs

rally25rs Feb 18, 2019

Contributor

what does this change do? 🤔

This comment has been minimized.

@alloy

alloy Feb 18, 2019

Author

This was needed to avoid hitting that same invariant that I did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment