From b7a48967fad6f4bb3abcd60390b28050953ac270 Mon Sep 17 00:00:00 2001 From: Joe Fitzgerald Date: Thu, 11 Aug 2016 12:45:02 -0600 Subject: [PATCH 1/4] Fix Regression In #12317 For zsh In Atom 1.7 and 1.8, the environment would be patched for users of the zsh shell on OS X. A whitelist of shells was established in #12317, which is extended here. --- spec/update-process-env-spec.js | 38 +++++++++++++++++++++++- src/initialize-application-window.coffee | 2 +- src/update-process-env.js | 29 ++++++++++++++++-- 3 files changed, 64 insertions(+), 5 deletions(-) diff --git a/spec/update-process-env-spec.js b/spec/update-process-env-spec.js index 3501bfebf..41c80c842 100644 --- a/spec/update-process-env-spec.js +++ b/spec/update-process-env-spec.js @@ -2,7 +2,7 @@ /* eslint-env jasmine */ import child_process from 'child_process' -import updateProcessEnv from '../src/update-process-env' +import {updateProcessEnv, shellShouldBePatched} from '../src/update-process-env' import dedent from 'dedent' describe('updateProcessEnv(launchEnv)', function () { @@ -84,5 +84,41 @@ describe('updateProcessEnv(launchEnv)', function () { expect(process.env).toEqual({FOO: 'bar'}) }) }) + + describe('shells on osx', function () { + it('shellShouldBePatched() returns the shell when the shell should be patched', function () { + process.platform = 'darwin' + let shellsToTest = new Set([ + '/bin/sh', + '/usr/local/bin/sh', + '/bin/bash', + '/usr/local/bin/bash', + '/bin/zsh', + '/usr/local/bin/zsh', + '/bin/fish', + '/usr/local/bin/fish' + ]) + for (let shell of shellsToTest) { + process.env.SHELL = shell + expect(shellShouldBePatched()).toBe(shell) + } + }) + + it('shellShouldBePatched() returns false when the shell should not be patched', function () { + process.platform = 'darwin' + let shellsToTest = new Set([ + '/bin/unsupported', + '/bin/shh', + '/bin/tcsh', + '/usr/csh' + ]) + for (let shell of shellsToTest) { + process.env.SHELL = shell + let result = shellShouldBePatched() + console.log(result) + expect(result).toBe(false) + } + }) + }) }) }) diff --git a/src/initialize-application-window.coffee b/src/initialize-application-window.coffee index f85207153..9787977e2 100644 --- a/src/initialize-application-window.coffee +++ b/src/initialize-application-window.coffee @@ -1,6 +1,6 @@ # Like sands through the hourglass, so are the days of our lives. module.exports = ({blobStore}) -> - updateProcessEnv = require('./update-process-env') + {updateProcessEnv} = require('./update-process-env') path = require 'path' require './window' {getWindowLoadSettings} = require './window-load-settings-helpers' diff --git a/src/update-process-env.js b/src/update-process-env.js index 63dc601bb..dd9ec6299 100644 --- a/src/update-process-env.js +++ b/src/update-process-env.js @@ -8,7 +8,14 @@ const ENVIRONMENT_VARIABLES_TO_PRESERVE = new Set([ 'ATOM_HOME' ]) -export default function updateProcessEnv (launchEnv) { +const OSX_SHELLS_TO_PATCH = new Set([ + '/sh', + '/bash', + '/zsh', + '/fish' +]) + +function updateProcessEnv (launchEnv) { let envToAssign if (launchEnv && launchEnv.PWD) { envToAssign = launchEnv @@ -33,9 +40,23 @@ export default function updateProcessEnv (launchEnv) { } } -function getEnvFromShell () { +function shellShouldBePatched () { let shell = process.env.SHELL - if (shell && (shell.endsWith('/bash') || shell.endsWith('/sh'))) { + if (!shell) { + return false + } + for (let s of OSX_SHELLS_TO_PATCH) { + if (shell.endsWith(s)) { + return shell + } + } + + return false +} + +function getEnvFromShell () { + let shell = shellShouldBePatched() + if (shell) { let {stdout} = spawnSync(shell, ['-ilc', 'command env'], {encoding: 'utf8'}) if (stdout) { let result = {} @@ -51,3 +72,5 @@ function getEnvFromShell () { } } } + +export default { updateProcessEnv, shellShouldBePatched } From 9ec63a8ffc8672bc21c524fee81f1771deba94c0 Mon Sep 17 00:00:00 2001 From: Joe Fitzgerald Date: Thu, 11 Aug 2016 16:45:19 -0600 Subject: [PATCH 2/4] :art: Cleanup * shellShouldBePatched > envShouldBePatched --- spec/update-process-env-spec.js | 50 ++++++++++++++------------------- src/update-process-env.js | 41 ++++++++++++++------------- 2 files changed, 42 insertions(+), 49 deletions(-) diff --git a/spec/update-process-env-spec.js b/spec/update-process-env-spec.js index 41c80c842..403e8fa7d 100644 --- a/spec/update-process-env-spec.js +++ b/spec/update-process-env-spec.js @@ -2,7 +2,7 @@ /* eslint-env jasmine */ import child_process from 'child_process' -import {updateProcessEnv, shellShouldBePatched} from '../src/update-process-env' +import {updateProcessEnv, envShouldBePatched} from '../src/update-process-env' import dedent from 'dedent' describe('updateProcessEnv(launchEnv)', function () { @@ -86,38 +86,30 @@ describe('updateProcessEnv(launchEnv)', function () { }) describe('shells on osx', function () { - it('shellShouldBePatched() returns the shell when the shell should be patched', function () { + it('envShouldBePatched() returns the shell when the shell should be patched', function () { process.platform = 'darwin' - let shellsToTest = new Set([ - '/bin/sh', - '/usr/local/bin/sh', - '/bin/bash', - '/usr/local/bin/bash', - '/bin/zsh', - '/usr/local/bin/zsh', - '/bin/fish', - '/usr/local/bin/fish' - ]) - for (let shell of shellsToTest) { - process.env.SHELL = shell - expect(shellShouldBePatched()).toBe(shell) - } + expect(envShouldBePatched('/bin/sh')).toBe(true) + expect(envShouldBePatched('/usr/local/bin/sh')).toBe(true) + expect(envShouldBePatched('/bin/bash')).toBe(true) + expect(envShouldBePatched('/usr/local/bin/bash')).toBe(true) + expect(envShouldBePatched('/bin/zsh')).toBe(true) + expect(envShouldBePatched('/usr/local/bin/zsh')).toBe(true) + expect(envShouldBePatched('/bin/fish')).toBe(true) + expect(envShouldBePatched('/usr/local/bin/fish')).toBe(true) }) - it('shellShouldBePatched() returns false when the shell should not be patched', function () { + it('envShouldBePatched() returns false when the shell should not be patched', function () { process.platform = 'darwin' - let shellsToTest = new Set([ - '/bin/unsupported', - '/bin/shh', - '/bin/tcsh', - '/usr/csh' - ]) - for (let shell of shellsToTest) { - process.env.SHELL = shell - let result = shellShouldBePatched() - console.log(result) - expect(result).toBe(false) - } + expect(envShouldBePatched('/bin/unsupported')).toBe(false) + expect(envShouldBePatched('/bin/shh')).toBe(false) + expect(envShouldBePatched('/bin/tcsh')).toBe(false) + expect(envShouldBePatched('/usr/csh')).toBe(false) + }) + + it('envShouldBePatched() returns false when the shell is undefined or empty', function () { + process.platform = 'darwin' + expect(envShouldBePatched(undefined)).toBe(false) + expect(envShouldBePatched('')).toBe(false) }) }) }) diff --git a/src/update-process-env.js b/src/update-process-env.js index dd9ec6299..565d566d9 100644 --- a/src/update-process-env.js +++ b/src/update-process-env.js @@ -8,7 +8,7 @@ const ENVIRONMENT_VARIABLES_TO_PRESERVE = new Set([ 'ATOM_HOME' ]) -const OSX_SHELLS_TO_PATCH = new Set([ +const OSX_SHELLS = new Set([ '/sh', '/bash', '/zsh', @@ -40,14 +40,13 @@ function updateProcessEnv (launchEnv) { } } -function shellShouldBePatched () { - let shell = process.env.SHELL - if (!shell) { +function envShouldBePatched (shell) { + if (!shell || shell.trim() === '') { return false } - for (let s of OSX_SHELLS_TO_PATCH) { + for (let s of OSX_SHELLS) { if (shell.endsWith(s)) { - return shell + return true } } @@ -55,22 +54,24 @@ function shellShouldBePatched () { } function getEnvFromShell () { - let shell = shellShouldBePatched() - if (shell) { - let {stdout} = spawnSync(shell, ['-ilc', 'command env'], {encoding: 'utf8'}) - if (stdout) { - let result = {} - for (let line of stdout.split('\n')) { - if (line.includes('=')) { - let components = line.split('=') - let key = components.shift() - let value = components.join('=') - result[key] = value - } + let shell = process.env.SHELL + if (!envShouldBePatched(shell)) { + return + } + + let {stdout} = spawnSync(shell, ['-ilc', 'command env'], {encoding: 'utf8'}) + if (stdout) { + let result = {} + for (let line of stdout.split('\n')) { + if (line.includes('=')) { + let components = line.split('=') + let key = components.shift() + let value = components.join('=') + result[key] = value } - return result } + return result } } -export default { updateProcessEnv, shellShouldBePatched } +export default { updateProcessEnv, envShouldBePatched } From 485cb71be7d264876c53190436339f7d73e8c993 Mon Sep 17 00:00:00 2001 From: Joe Fitzgerald Date: Thu, 11 Aug 2016 17:07:21 -0600 Subject: [PATCH 3/4] :art: envShouldBePatched > shouldGetEnvFromShell --- spec/update-process-env-spec.js | 36 ++++++++++++++++----------------- src/update-process-env.js | 6 +++--- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/spec/update-process-env-spec.js b/spec/update-process-env-spec.js index 403e8fa7d..d19ece978 100644 --- a/spec/update-process-env-spec.js +++ b/spec/update-process-env-spec.js @@ -2,7 +2,7 @@ /* eslint-env jasmine */ import child_process from 'child_process' -import {updateProcessEnv, envShouldBePatched} from '../src/update-process-env' +import {updateProcessEnv, shouldGetEnvFromShell} from '../src/update-process-env' import dedent from 'dedent' describe('updateProcessEnv(launchEnv)', function () { @@ -86,30 +86,30 @@ describe('updateProcessEnv(launchEnv)', function () { }) describe('shells on osx', function () { - it('envShouldBePatched() returns the shell when the shell should be patched', function () { + it('shouldGetEnvFromShell() returns the shell when the shell should be patched', function () { process.platform = 'darwin' - expect(envShouldBePatched('/bin/sh')).toBe(true) - expect(envShouldBePatched('/usr/local/bin/sh')).toBe(true) - expect(envShouldBePatched('/bin/bash')).toBe(true) - expect(envShouldBePatched('/usr/local/bin/bash')).toBe(true) - expect(envShouldBePatched('/bin/zsh')).toBe(true) - expect(envShouldBePatched('/usr/local/bin/zsh')).toBe(true) - expect(envShouldBePatched('/bin/fish')).toBe(true) - expect(envShouldBePatched('/usr/local/bin/fish')).toBe(true) + expect(shouldGetEnvFromShell('/bin/sh')).toBe(true) + expect(shouldGetEnvFromShell('/usr/local/bin/sh')).toBe(true) + expect(shouldGetEnvFromShell('/bin/bash')).toBe(true) + expect(shouldGetEnvFromShell('/usr/local/bin/bash')).toBe(true) + expect(shouldGetEnvFromShell('/bin/zsh')).toBe(true) + expect(shouldGetEnvFromShell('/usr/local/bin/zsh')).toBe(true) + expect(shouldGetEnvFromShell('/bin/fish')).toBe(true) + expect(shouldGetEnvFromShell('/usr/local/bin/fish')).toBe(true) }) - it('envShouldBePatched() returns false when the shell should not be patched', function () { + it('shouldGetEnvFromShell() returns false when the shell should not be patched', function () { process.platform = 'darwin' - expect(envShouldBePatched('/bin/unsupported')).toBe(false) - expect(envShouldBePatched('/bin/shh')).toBe(false) - expect(envShouldBePatched('/bin/tcsh')).toBe(false) - expect(envShouldBePatched('/usr/csh')).toBe(false) + expect(shouldGetEnvFromShell('/bin/unsupported')).toBe(false) + expect(shouldGetEnvFromShell('/bin/shh')).toBe(false) + expect(shouldGetEnvFromShell('/bin/tcsh')).toBe(false) + expect(shouldGetEnvFromShell('/usr/csh')).toBe(false) }) - it('envShouldBePatched() returns false when the shell is undefined or empty', function () { + it('shouldGetEnvFromShell() returns false when the shell is undefined or empty', function () { process.platform = 'darwin' - expect(envShouldBePatched(undefined)).toBe(false) - expect(envShouldBePatched('')).toBe(false) + expect(shouldGetEnvFromShell(undefined)).toBe(false) + expect(shouldGetEnvFromShell('')).toBe(false) }) }) }) diff --git a/src/update-process-env.js b/src/update-process-env.js index 565d566d9..2bbbbfe7e 100644 --- a/src/update-process-env.js +++ b/src/update-process-env.js @@ -40,7 +40,7 @@ function updateProcessEnv (launchEnv) { } } -function envShouldBePatched (shell) { +function shouldGetEnvFromShell (shell) { if (!shell || shell.trim() === '') { return false } @@ -55,7 +55,7 @@ function envShouldBePatched (shell) { function getEnvFromShell () { let shell = process.env.SHELL - if (!envShouldBePatched(shell)) { + if (!shouldGetEnvFromShell(shell)) { return } @@ -74,4 +74,4 @@ function getEnvFromShell () { } } -export default { updateProcessEnv, envShouldBePatched } +export default { updateProcessEnv, shouldGetEnvFromShell } From f529ebb237cc12966898f28620b021c434100d38 Mon Sep 17 00:00:00 2001 From: Joe Fitzgerald Date: Thu, 11 Aug 2016 17:08:28 -0600 Subject: [PATCH 4/4] :art: Update Describe / It Descriptions --- spec/update-process-env-spec.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/update-process-env-spec.js b/spec/update-process-env-spec.js index d19ece978..39376b4c2 100644 --- a/spec/update-process-env-spec.js +++ b/spec/update-process-env-spec.js @@ -85,8 +85,8 @@ describe('updateProcessEnv(launchEnv)', function () { }) }) - describe('shells on osx', function () { - it('shouldGetEnvFromShell() returns the shell when the shell should be patched', function () { + describe('shouldGetEnvFromShell()', function () { + it('returns the shell when the shell should be patched', function () { process.platform = 'darwin' expect(shouldGetEnvFromShell('/bin/sh')).toBe(true) expect(shouldGetEnvFromShell('/usr/local/bin/sh')).toBe(true) @@ -98,7 +98,7 @@ describe('updateProcessEnv(launchEnv)', function () { expect(shouldGetEnvFromShell('/usr/local/bin/fish')).toBe(true) }) - it('shouldGetEnvFromShell() returns false when the shell should not be patched', function () { + it('returns false when the shell should not be patched', function () { process.platform = 'darwin' expect(shouldGetEnvFromShell('/bin/unsupported')).toBe(false) expect(shouldGetEnvFromShell('/bin/shh')).toBe(false) @@ -106,7 +106,7 @@ describe('updateProcessEnv(launchEnv)', function () { expect(shouldGetEnvFromShell('/usr/csh')).toBe(false) }) - it('shouldGetEnvFromShell() returns false when the shell is undefined or empty', function () { + it('returns false when the shell is undefined or empty', function () { process.platform = 'darwin' expect(shouldGetEnvFromShell(undefined)).toBe(false) expect(shouldGetEnvFromShell('')).toBe(false)