From 3474142ece1b598b3eb7a921a379eda275db73d4 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 25 Aug 2017 13:01:14 +0100 Subject: [PATCH 1/5] Fix: Add `$NODE` when running scripts and make `yarn env` reflect these **Summary** Fixes #2609. This patch adds a `$NODE` environment variable when running scripts through `yarn run` which is set to `process.execPath` unless it is formerly set to something else. `$npm_node_execpath` is now just an alias to `$NODE`. The patch also updates `yarn env` to show these extra environment variables. **Test plan** Updates existing `yarn env` tests. --- __tests__/commands/run.js | 37 +++++++++++++++++------ src/cli/commands/run.js | 4 +-- src/util/execute-lifecycle-script.js | 45 ++++++++++++++++------------ 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/__tests__/commands/run.js b/__tests__/commands/run.js index 2b85c4063d..284087ece4 100644 --- a/__tests__/commands/run.js +++ b/__tests__/commands/run.js @@ -1,6 +1,14 @@ /* @flow */ -jest.mock('../../src/util/execute-lifecycle-script'); +jest.mock('../../src/util/execute-lifecycle-script', () => { + return { + // $FlowFixMe + ...require.requireActual('../../src/util/execute-lifecycle-script'), + execCommand: jest.fn(), + }; +}); + +import path from 'path'; import {run as buildRun} from './_helpers.js'; import {BufferReporter} from '../../src/reporters/index.js'; @@ -10,9 +18,7 @@ import * as reporters from '../../src/reporters/index.js'; jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000; -const execCommand: $FlowFixMe = require('../../src/util/execute-lifecycle-script').execCommand; - -const path = require('path'); +const {execCommand}: $FlowFixMe = require('../../src/util/execute-lifecycle-script'); beforeEach(() => execCommand.mockClear()); @@ -77,11 +83,24 @@ test('properly handle bin scripts', (): Promise => { test('properly handle env command', (): Promise => { return runRun(['env'], {}, 'no-args', (config, reporter): ?Promise => { - const rprtr = new reporters.BufferReporter({stdout: null, stdin: null}); - - rprtr.info(`${JSON.stringify(process.env, null, 2)}`); - - expect(reporter.getBuffer()).toEqual(rprtr.getBuffer()); + // $FlowFixMe + const result = JSON.parse(reporter.getBuffer()[0].data); + result.PATH = result.PATH.split(path.delimiter); + + const env = {}; + for (const key of Object.keys(process.env)) { + // Filter out yarn-added `npm_` variables since we run tests through yarn already + if (!key.startsWith('npm_')) { + env[key] = process.env[key]; + } + } + + env.PATH = env.PATH ? expect.arrayContaining(env.PATH.split(path.delimiter)) : []; + + expect(result).toMatchObject(env); + expect(result).toHaveProperty('npm_lifecycle_event'); + expect(result).toHaveProperty('npm_execpath'); + expect(result).toHaveProperty('npm_node_execpath'); }); }); diff --git a/src/cli/commands/run.js b/src/cli/commands/run.js index f1d84818ab..a26a663d71 100644 --- a/src/cli/commands/run.js +++ b/src/cli/commands/run.js @@ -2,7 +2,7 @@ import type {Reporter} from '../../reporters/index.js'; import type Config from '../../config.js'; -import {execCommand} from '../../util/execute-lifecycle-script.js'; +import {execCommand, makeEnv} from '../../util/execute-lifecycle-script.js'; import {MessageError} from '../../errors.js'; import {registries} from '../../resolvers/index.js'; import * as fs from '../../util/fs.js'; @@ -87,7 +87,7 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg await execCommand(stage, config, cmdWithArgs, config.cwd); } } else if (action === 'env') { - reporter.info(`${JSON.stringify(process.env, null, 2)}`); + reporter.log(JSON.stringify(await makeEnv('env', config.cwd, config), null, 2), {force: true}); } else { let suggestion; diff --git a/src/util/execute-lifecycle-script.js b/src/util/execute-lifecycle-script.js index 0a3ec1ac86..56486e47d0 100644 --- a/src/util/execute-lifecycle-script.js +++ b/src/util/execute-lifecycle-script.js @@ -33,7 +33,10 @@ export async function makeEnv( ): { [key: string]: string, } { - const env = Object.assign({}, process.env); + const env = { + NODE: process.execPath, + ...process.env, + }; // Merge in the `env` object specified in .yarnrc const customEnv = config.getOption('env'); @@ -41,8 +44,12 @@ export async function makeEnv( Object.assign(env, customEnv); } + if (!env.NODE) { + env.NODE = process.execPath; + } + env.npm_lifecycle_event = stage; - env.npm_node_execpath = env.NODE || process.execPath; + env.npm_node_execpath = env.NODE; env.npm_execpath = env.npm_execpath || process.mainModule.filename; // Set the env to production for npm compat if production mode. @@ -117,21 +124,6 @@ export async function makeEnv( env[envKey] = val; } - return env; -} - -export async function executeLifecycleScript( - stage: string, - config: Config, - cwd: string, - cmd: string, - spinner?: ReporterSpinner, -): LifecycleReturn { - // if we don't have a spinner then pipe everything to the terminal - const stdio = spinner ? undefined : 'inherit'; - - const env = await makeEnv(stage, cwd, config); - // split up the path const pathParts = (env[constants.ENV_PATH_KEY] || '').split(path.delimiter); @@ -156,8 +148,6 @@ export async function executeLifecycleScript( pathParts.unshift(path.join(cwd, binFolder)); } - await checkForGypIfNeeded(config, cmd, pathParts); - if (config.scriptsPrependNodePath) { pathParts.unshift(path.join(path.dirname(process.execPath))); } @@ -165,6 +155,23 @@ export async function executeLifecycleScript( // join path back together env[constants.ENV_PATH_KEY] = pathParts.join(path.delimiter); + return env; +} + +export async function executeLifecycleScript( + stage: string, + config: Config, + cwd: string, + cmd: string, + spinner?: ReporterSpinner, +): LifecycleReturn { + // if we don't have a spinner then pipe everything to the terminal + const stdio = spinner ? undefined : 'inherit'; + + const env = await makeEnv(stage, cwd, config); + + await checkForGypIfNeeded(config, cmd, env[constants.ENV_PATH_KEY].split(path.delimiter)); + // get shell if (process.platform === 'win32') { // handle windows run scripts starting with a relative path From e1b5b0356990f143c29a0f003dd6f6fa79d2c74e Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 25 Aug 2017 15:43:35 +0100 Subject: [PATCH 2/5] Comment about NODE, remove obsolete lines --- src/util/execute-lifecycle-script.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/util/execute-lifecycle-script.js b/src/util/execute-lifecycle-script.js index 56486e47d0..c71a501379 100644 --- a/src/util/execute-lifecycle-script.js +++ b/src/util/execute-lifecycle-script.js @@ -35,6 +35,9 @@ export async function makeEnv( } { const env = { NODE: process.execPath, + // This lets `process.env.NODE` to override our `process.execPath`. + // This is a bit confusing but it is how `npm` was designed so we + // try to be compatible with that. ...process.env, }; @@ -44,10 +47,6 @@ export async function makeEnv( Object.assign(env, customEnv); } - if (!env.NODE) { - env.NODE = process.execPath; - } - env.npm_lifecycle_event = stage; env.npm_node_execpath = env.NODE; env.npm_execpath = env.npm_execpath || process.mainModule.filename; From bbd462057bae872f9df9654e5360b114f3dcd544 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 25 Aug 2017 15:45:04 +0100 Subject: [PATCH 3/5] Protect against empty path in tests --- __tests__/commands/run.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/__tests__/commands/run.js b/__tests__/commands/run.js index 284087ece4..8a3b7ea6b8 100644 --- a/__tests__/commands/run.js +++ b/__tests__/commands/run.js @@ -85,7 +85,7 @@ test('properly handle env command', (): Promise => { return runRun(['env'], {}, 'no-args', (config, reporter): ?Promise => { // $FlowFixMe const result = JSON.parse(reporter.getBuffer()[0].data); - result.PATH = result.PATH.split(path.delimiter); + result.PATH = result.PATH ? result.PATH.split(path.delimiter) : []; const env = {}; for (const key of Object.keys(process.env)) { From 47e64b4ff9e0a69aacfd4896df1004705655a288 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 25 Aug 2017 15:48:46 +0100 Subject: [PATCH 4/5] Protect against empty path in real life too --- src/util/execute-lifecycle-script.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/execute-lifecycle-script.js b/src/util/execute-lifecycle-script.js index c71a501379..fe7ac805c6 100644 --- a/src/util/execute-lifecycle-script.js +++ b/src/util/execute-lifecycle-script.js @@ -124,7 +124,8 @@ export async function makeEnv( } // split up the path - const pathParts = (env[constants.ENV_PATH_KEY] || '').split(path.delimiter); + const envPath = env[constants.ENV_PATH_KEY]; + const pathParts = envPath ? envPath.split(path.delimiter) : []; // Include node-gyp version that was bundled with the current Node.js version, // if available. From 679ee99b2d3af9fa65029dadd17ac781e44215d5 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 25 Aug 2017 21:20:53 +0100 Subject: [PATCH 5/5] Fix tests for case-insensitive Windows env --- __tests__/commands/run.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/__tests__/commands/run.js b/__tests__/commands/run.js index 8a3b7ea6b8..386c7e3bd7 100644 --- a/__tests__/commands/run.js +++ b/__tests__/commands/run.js @@ -85,17 +85,27 @@ test('properly handle env command', (): Promise => { return runRun(['env'], {}, 'no-args', (config, reporter): ?Promise => { // $FlowFixMe const result = JSON.parse(reporter.getBuffer()[0].data); - result.PATH = result.PATH ? result.PATH.split(path.delimiter) : []; const env = {}; + let pathVarName = 'PATH'; for (const key of Object.keys(process.env)) { // Filter out yarn-added `npm_` variables since we run tests through yarn already - if (!key.startsWith('npm_')) { - env[key] = process.env[key]; + if (key.startsWith('npm_')) { + continue; } + // We need this below for Windows which has case-insensitive env vars + // If we used `process.env` directly, node takes care of this for us, + // but since we use a subset of it, we need to get the "real" path key + // name for Jest's case-sensitive object comparison below. + if (key.toUpperCase() === 'PATH') { + pathVarName = key; + } + env[key] = process.env[key]; } - env.PATH = env.PATH ? expect.arrayContaining(env.PATH.split(path.delimiter)) : []; + result[pathVarName] = result[pathVarName] ? result[pathVarName].split(path.delimiter) : []; + // $FlowFixMe + env[pathVarName] = env[pathVarName] ? expect.arrayContaining(env[pathVarName].split(path.delimiter)) : []; expect(result).toMatchObject(env); expect(result).toHaveProperty('npm_lifecycle_event');