From 6b38049b8d10deb9abf66fbdb19d2280e04075f2 Mon Sep 17 00:00:00 2001 From: Joe Fitzgerald Date: Thu, 10 Mar 2016 15:16:41 -0700 Subject: [PATCH] Remove Project API, Work With process.env Directly - Convert environment.coffee and environment-spec.coffee to JavaScript - Pass the process's environment across the wire when launching atom multiple times from the command line --- spec/atom-environment-spec.coffee | 13 -- spec/environment-spec.coffee | 27 ---- spec/environment-spec.js | 161 +++++++++++++++++++++++ spec/project-spec.coffee | 68 ---------- src/atom-environment.coffee | 7 +- src/browser/atom-application.coffee | 29 ++-- src/browser/main.coffee | 1 + src/environment.coffee | 41 ------ src/environment.js | 94 +++++++++++++ src/initialize-application-window.coffee | 3 +- src/project.coffee | 20 --- 11 files changed, 275 insertions(+), 189 deletions(-) delete mode 100644 spec/environment-spec.coffee create mode 100644 spec/environment-spec.js delete mode 100644 src/environment.coffee create mode 100644 src/environment.js diff --git a/spec/atom-environment-spec.coffee b/spec/atom-environment-spec.coffee index 6271ea5d4..3283b63d6 100644 --- a/spec/atom-environment-spec.coffee +++ b/spec/atom-environment-spec.coffee @@ -362,16 +362,3 @@ describe "AtomEnvironment", -> version = '1.7.0-dev-5340c91' expect(atom.getReleaseChannel()).toBe 'dev' - - describe "environment patching", -> - it "patches process.env on startup", -> - configDirPath = temp.mkdirSync() - fakeDocument = { - addEventListener: -> - removeEventListener: -> - head: document.createElement('head') - body: document.createElement('body') - } - atomEnvironment = new AtomEnvironment({applicationDelegate: atom.applicationDelegate, window, document: fakeDocument}) - - expect(process.env).toEqual atomEnvironment.project.getEnv() diff --git a/spec/environment-spec.coffee b/spec/environment-spec.coffee deleted file mode 100644 index 9eac5b071..000000000 --- a/spec/environment-spec.coffee +++ /dev/null @@ -1,27 +0,0 @@ -child_process = require('child_process') -{getShellEnv} = require("../src/environment") - -fdescribe "Environment handling", -> - describe "when things are configured properly", -> - beforeEach -> - spyOn(child_process, "spawnSync").andReturn - stdout: """ - FOO=BAR - TERM=xterm-something - PATH=/usr/bin:/bin:/usr/sbin:/sbin:/some/crazy/path/entry/that/should/not/exist - """ - - it "returns an object containing the information from the user's shell environment", -> - env = getShellEnv() - - expect(env.FOO).toEqual "BAR" - expect(env.TERM).toEqual "xterm-something" - expect(env.PATH).toEqual "/usr/bin:/bin:/usr/sbin:/sbin:/some/crazy/path/entry/that/should/not/exist" - - describe "when an error occurs", -> - beforeEach -> - spyOn(child_process, "spawnSync").andReturn - error: new Error("testing when an error occurs") - - it "returns undefined", -> - expect(getShellEnv()).toBeUndefined() diff --git a/spec/environment-spec.js b/spec/environment-spec.js new file mode 100644 index 000000000..ae36d7d2c --- /dev/null +++ b/spec/environment-spec.js @@ -0,0 +1,161 @@ +'use babel' +/* eslint-env jasmine */ + +import child_process from 'child_process' +import environment from '../src/environment' +import os from 'os' +import _ from 'underscore-plus' + +fdescribe('Environment handling', () => { + let originalEnv + let options + + beforeEach(() => { + originalEnv = process.env + delete process._originalEnv + options = { + platform: process.platform, + env: _.clone(process.env) + } + }) + + afterEach(() => { + process.env = originalEnv + delete process._originalEnv + }) + + describe('on OSX, when PWD is not set', () => { + beforeEach(() => { + options.platform = 'darwin' + }) + + describe('needsPatching', () => { + it('returns true if PWD is unset', () => { + delete options.env.PWD + expect(environment.needsPatching(options)).toBe(true) + options.env.PWD = undefined + expect(environment.needsPatching(options)).toBe(true) + options.env.PWD = null + expect(environment.needsPatching(options)).toBe(true) + options.env.PWD = false + expect(environment.needsPatching(options)).toBe(true) + }) + + it('returns false if PWD is set', () => { + options.env.PWD = 'xterm' + expect(environment.needsPatching(options)).toBe(false) + }) + }) + + describe('normalize', () => { + it('changes process.env if PWD is unset', () => { + if (process.platform === 'win32') { + return + } + delete options.env.PWD + environment.normalize(options) + expect(process._originalEnv).toBeDefined() + expect(process._originalEnv).toBeTruthy() + expect(process.env).toBeDefined() + expect(process.env).toBeTruthy() + expect(process.env.PWD).toBeDefined() + expect(process.env.PWD).toBeTruthy() + expect(process.env.PATH).toBeDefined() + expect(process.env.PATH).toBeTruthy() + expect(process.env.ATOM_HOME).toBeDefined() + expect(process.env.ATOM_HOME).toBeTruthy() + }) + }) + }) + + describe('on a platform other than OSX', () => { + beforeEach(() => { + options.platform = 'penguin' + }) + + describe('needsPatching', () => { + it('returns false if PWD is set or unset', () => { + delete options.env.PWD + expect(environment.needsPatching(options)).toBe(false) + options.env.PWD = undefined + expect(environment.needsPatching(options)).toBe(false) + options.env.PWD = null + expect(environment.needsPatching(options)).toBe(false) + options.env.PWD = false + expect(environment.needsPatching(options)).toBe(false) + options.env.PWD = '/' + expect(environment.needsPatching(options)).toBe(false) + }) + + it('returns false for linux', () => { + options.platform = 'linux' + options.PWD = '/' + expect(environment.needsPatching(options)).toBe(false) + }) + + it('returns false for windows', () => { + options.platform = 'win32' + options.PWD = 'c:\\' + expect(environment.needsPatching(options)).toBe(false) + }) + }) + + describe('normalize', () => { + it('does not change the environment', () => { + if (process.platform === 'win32') { + return + } + delete options.env.PWD + environment.normalize(options) + expect(process._originalEnv).toBeUndefined() + expect(process.env).toBeDefined() + expect(process.env).toBeTruthy() + expect(process.env.PATH).toBeDefined() + expect(process.env.PATH).toBeTruthy() + expect(process.env.PWD).toBeUndefined() + expect(process.env.PATH).toBe(originalEnv.PATH) + expect(process.env.ATOM_HOME).toBeDefined() + expect(process.env.ATOM_HOME).toBeTruthy() + }) + }) + }) + + describe('getFromShell', () => { + describe('when things are configured properly', () => { + beforeEach(() => { + spyOn(child_process, 'spawnSync').andReturn({ + stdout: 'FOO=BAR' + os.EOL + 'TERM=xterm-something' + os.EOL + + 'PATH=/usr/bin:/bin:/usr/sbin:/sbin:/crazy/path' + }) + }) + + it('returns an object containing the information from the user\'s shell environment', () => { + let env = environment.getFromShell() + expect(env.FOO).toEqual('BAR') + expect(env.TERM).toEqual('xterm-something') + expect(env.PATH).toEqual('/usr/bin:/bin:/usr/sbin:/sbin:/crazy/path') + }) + }) + + describe('when an error occurs launching the shell', () => { + beforeEach(() => { + spyOn(child_process, 'spawnSync').andReturn({ + error: new Error('testing when an error occurs') + }) + }) + + it('returns undefined', () => { + expect(environment.getFromShell()).toBeUndefined() + }) + + it('leaves the environment as-is when normalize() is called', () => { + options.platform = 'darwin' + delete options.env.PWD + expect(environment.needsPatching(options)).toBe(true) + environment.normalize(options) + expect(process.env).toBeDefined() + expect(process._originalEnv).toBeUndefined() + }) + }) + }) +}) diff --git a/spec/project-spec.coffee b/spec/project-spec.coffee index f5fcad8e3..499efd017 100644 --- a/spec/project-spec.coffee +++ b/spec/project-spec.coffee @@ -8,8 +8,6 @@ BufferedProcess = require '../src/buffered-process' {Directory} = require 'pathwatcher' GitRepository = require '../src/git-repository' -environment = require '../src/environment' - describe "Project", -> beforeEach -> atom.project.setPaths([atom.project.getDirectories()[0]?.resolve('dir')]) @@ -539,69 +537,3 @@ describe "Project", -> randomPath = path.join("some", "random", "path") expect(atom.project.contains(randomPath)).toBe false - - describe ".getEnv", -> - [originalTerm] = [] - - beforeEach -> - originalTerm = process.env.TERM - - afterEach -> - process.env.TERM = originalTerm - delete atom.project.env - - it "returns a copy of the environment", -> - env = atom.project.getEnv() - - env.PROJECT_GET_ENV_TESTING = "foo" - expect(process.env.PROJECT_GET_ENV_TESTING).not.toEqual "foo" - expect(atom.project.getEnv().PROJECT_GET_ENV_TESTING).not.toEqual "foo" - - describe "on platforms other than OS X", -> - beforeEach -> - spyOn(process, "platform").andReturn("foo") - - describe "when TERM is not set", -> - beforeEach -> - delete process.env.TERM - - it "returns the PATH unchanged", -> - expect(atom.project.getEnv().PATH).toEqual process.env.PATH - - describe "when TERM is set", -> - beforeEach -> - process.env.TERM = "foo" - - it "returns the PATH unchanged", -> - expect(atom.project.getEnv().PATH).toEqual process.env.PATH - - describe "on OS X", -> - beforeEach -> - spyOn(process, "platform").andReturn("darwin") - - describe "when TERM is not set", -> - beforeEach -> - delete process.env.TERM - - it "replaces the environment with the one obtained from the shell", -> - spyOn(environment, "getShellEnv").andReturn - FOO: "BAR" - TERM: "xterm-something" - PATH: "/usr/bin:/bin:/usr/sbin:/sbin:/some/crazy/path/entry/that/should/not/exist" - - expect(atom.project.getEnv().TERM).toEqual "xterm-something" - expect(atom.project.getEnv().PATH).toEqual "/usr/bin:/bin:/usr/sbin:/sbin:/some/crazy/path/entry/that/should/not/exist" - expect(atom.project.getEnv().FOO).toEqual "BAR" - - it "does the best it can when there is an error retrieving the shell environment", -> - spyOn(environment, "getShellEnv").andReturn(undefined) - - expect(atom.project.getEnv().PATH).not.toBeUndefined() - expect(atom.project.getEnv().PATH).toEqual process.env.PATH - - describe "when TERM is set", -> - beforeEach -> - process.env.TERM = "foo" - - it "returns the PATH unchanged", -> - expect(atom.project.getEnv().PATH).toEqual process.env.PATH diff --git a/src/atom-environment.coffee b/src/atom-environment.coffee index e43051571..97ec3c5d4 100644 --- a/src/atom-environment.coffee +++ b/src/atom-environment.coffee @@ -4,6 +4,7 @@ path = require 'path' _ = require 'underscore-plus' {deprecate} = require 'grim' +environment = require('./environment') {CompositeDisposable, Disposable, Emitter} = require 'event-kit' fs = require 'fs-plus' {mapSourcePosition} = require 'source-map-support' @@ -127,6 +128,7 @@ class AtomEnvironment extends Model # Call .loadOrCreate instead constructor: (params={}) -> + environment.normalize(params) {@blobStore, @applicationDelegate, @window, @document, configDirPath, @enablePersistence, onlyLoadBaseStyleSheets} = params @unloaded = false @@ -230,11 +232,6 @@ class AtomEnvironment extends Model checkPortableHomeWritable() - # Patch the `process.env` on startup to fix the problem first documented - # in #4126. Retain the original in case someone needs it. - process._originalEnv = process.env - process.env = @project.getEnv() - attachSaveStateListeners: -> saveState = => @saveState({isUnloading: false}) unless @unloaded debouncedSaveState = _.debounce(saveState, @saveStateDebounceInterval) diff --git a/src/browser/atom-application.coffee b/src/browser/atom-application.coffee index 230e1bb9f..4767c9065 100644 --- a/src/browser/atom-application.coffee +++ b/src/browser/atom-application.coffee @@ -85,16 +85,16 @@ class AtomApplication else @loadState(options) or @openPath(options) - openWithOptions: ({initialPaths, pathsToOpen, executedFrom, urlsToOpen, test, pidToKillWhenClosed, devMode, safeMode, newWindow, logFile, profileStartup, timeout, clearWindowState, addToLastWindow}) -> + openWithOptions: ({initialPaths, pathsToOpen, executedFrom, urlsToOpen, test, pidToKillWhenClosed, devMode, safeMode, newWindow, logFile, profileStartup, timeout, clearWindowState, addToLastWindow, env}) -> if test - @runTests({headless: true, devMode, @resourcePath, executedFrom, pathsToOpen, logFile, timeout}) + @runTests({headless: true, devMode, @resourcePath, executedFrom, pathsToOpen, logFile, timeout, env}) else if pathsToOpen.length > 0 - @openPaths({initialPaths, pathsToOpen, executedFrom, pidToKillWhenClosed, newWindow, devMode, safeMode, profileStartup, clearWindowState, addToLastWindow}) + @openPaths({initialPaths, pathsToOpen, executedFrom, pidToKillWhenClosed, newWindow, devMode, safeMode, profileStartup, clearWindowState, addToLastWindow, env}) else if urlsToOpen.length > 0 - @openUrl({urlToOpen, devMode, safeMode}) for urlToOpen in urlsToOpen + @openUrl({urlToOpen, devMode, safeMode, env}) for urlToOpen in urlsToOpen else # Always open a editor window if this is the first instance of Atom. - @openPath({initialPaths, pidToKillWhenClosed, newWindow, devMode, safeMode, profileStartup, clearWindowState, addToLastWindow}) + @openPath({initialPaths, pidToKillWhenClosed, newWindow, devMode, safeMode, profileStartup, clearWindowState, addToLastWindow, env}) # Public: Removes the {AtomWindow} from the global window list. removeWindow: (window) -> @@ -134,7 +134,8 @@ class AtomApplication @deleteSocketFile() server = net.createServer (connection) => connection.on 'data', (data) => - @openWithOptions(JSON.parse(data)) + options = JSON.parse(data) + @openWithOptions(options) server.listen @socketPath server.on 'error', (error) -> console.error 'Application server failed', error @@ -418,8 +419,8 @@ class AtomApplication # :profileStartup - Boolean to control creating a profile of the startup time. # :window - {AtomWindow} to open file paths in. # :addToLastWindow - Boolean of whether this should be opened in last focused window. - openPath: ({initialPaths, pathToOpen, pidToKillWhenClosed, newWindow, devMode, safeMode, profileStartup, window, clearWindowState, addToLastWindow} = {}) -> - @openPaths({initialPaths, pathsToOpen: [pathToOpen], pidToKillWhenClosed, newWindow, devMode, safeMode, profileStartup, window, clearWindowState, addToLastWindow}) + openPath: ({initialPaths, pathToOpen, pidToKillWhenClosed, newWindow, devMode, safeMode, profileStartup, window, clearWindowState, addToLastWindow, env} = {}) -> + @openPaths({initialPaths, pathsToOpen: [pathToOpen], pidToKillWhenClosed, newWindow, devMode, safeMode, profileStartup, window, clearWindowState, addToLastWindow, env}) # Public: Opens multiple paths, in existing windows if possible. # @@ -432,7 +433,7 @@ class AtomApplication # :windowDimensions - Object with height and width keys. # :window - {AtomWindow} to open file paths in. # :addToLastWindow - Boolean of whether this should be opened in last focused window. - openPaths: ({initialPaths, pathsToOpen, executedFrom, pidToKillWhenClosed, newWindow, devMode, safeMode, windowDimensions, profileStartup, window, clearWindowState, addToLastWindow}={}) -> + openPaths: ({initialPaths, pathsToOpen, executedFrom, pidToKillWhenClosed, newWindow, devMode, safeMode, windowDimensions, profileStartup, window, clearWindowState, addToLastWindow, env}={}) -> devMode = Boolean(devMode) safeMode = Boolean(safeMode) clearWindowState = Boolean(clearWindowState) @@ -469,7 +470,7 @@ class AtomApplication windowInitializationScript ?= require.resolve('../initialize-application-window') resourcePath ?= @resourcePath windowDimensions ?= @getDimensionsForNewWindow() - openedWindow = new AtomWindow({initialPaths, locationsToOpen, windowInitializationScript, resourcePath, devMode, safeMode, windowDimensions, profileStartup, clearWindowState}) + openedWindow = new AtomWindow({initialPaths, locationsToOpen, windowInitializationScript, resourcePath, devMode, safeMode, windowDimensions, profileStartup, clearWindowState, env}) if pidToKillWhenClosed? @pidsToOpenWindows[pidToKillWhenClosed] = openedWindow @@ -532,7 +533,7 @@ class AtomApplication # :urlToOpen - The atom:// url to open. # :devMode - Boolean to control the opened window's dev mode. # :safeMode - Boolean to control the opened window's safe mode. - openUrl: ({urlToOpen, devMode, safeMode}) -> + openUrl: ({urlToOpen, devMode, safeMode, env}) -> unless @packages? PackageManager = require '../package-manager' @packages = new PackageManager @@ -547,7 +548,7 @@ class AtomApplication packagePath = @packages.resolvePackagePath(packageName) windowInitializationScript = path.resolve(packagePath, pack.urlMain) windowDimensions = @getDimensionsForNewWindow() - new AtomWindow({windowInitializationScript, @resourcePath, devMode, safeMode, urlToOpen, windowDimensions}) + new AtomWindow({windowInitializationScript, @resourcePath, devMode, safeMode, urlToOpen, windowDimensions, env}) else console.log "Package '#{pack.name}' does not have a url main: #{urlToOpen}" else @@ -562,7 +563,7 @@ class AtomApplication # :specPath - The directory to load specs from. # :safeMode - A Boolean that, if true, won't run specs from ~/.atom/packages # and ~/.atom/dev/packages, defaults to false. - runTests: ({headless, resourcePath, executedFrom, pathsToOpen, logFile, safeMode, timeout}) -> + runTests: ({headless, resourcePath, executedFrom, pathsToOpen, logFile, safeMode, timeout, env}) -> if resourcePath isnt @resourcePath and not fs.existsSync(resourcePath) resourcePath = @resourcePath @@ -592,7 +593,7 @@ class AtomApplication devMode = true isSpec = true safeMode ?= false - new AtomWindow({windowInitializationScript, resourcePath, headless, isSpec, devMode, testRunnerPath, legacyTestRunnerPath, testPaths, logFile, safeMode}) + new AtomWindow({windowInitializationScript, resourcePath, headless, isSpec, devMode, testRunnerPath, legacyTestRunnerPath, testPaths, logFile, safeMode, env}) resolveTestRunnerPath: (testPath) -> FindParentDir ?= require 'find-parent-dir' diff --git a/src/browser/main.coffee b/src/browser/main.coffee index b4df62bd6..6bf8817f9 100644 --- a/src/browser/main.coffee +++ b/src/browser/main.coffee @@ -13,6 +13,7 @@ console.log = require 'nslog' start = -> args = parseCommandLine() + args.env = process.env setupAtomHome(args) setupCompileCache() return if handleStartupEventWithSquirrel() diff --git a/src/environment.coffee b/src/environment.coffee deleted file mode 100644 index 56de0c194..000000000 --- a/src/environment.coffee +++ /dev/null @@ -1,41 +0,0 @@ -child_process = require('child_process') -os = require('os') - -# Gets a dump of the user's configured shell environment. -# -# Returns the output of the `env` command or `undefined` if there was an error. -getRawShellEnv = -> - shell = process.env.SHELL ? "/bin/bash" - - # The `-ilc` set of options was tested to work with the OS X v10.11 - # default-installed versions of bash, zsh, sh, and ksh. It *does not* - # work with csh or tcsh. Given that bash and zsh should cover the - # vast majority of users and it gracefully falls back to prior behavior, - # this should be safe. - results = child_process.spawnSync(shell, ["-ilc", "env"], encoding: "utf8") - return if results.error? - return unless results.stdout and results.stdout.length > 0 - - results.stdout - -module.exports = - # Gets the user's configured shell environment. - # - # Returns a copy of the user's shell enviroment. - getShellEnv: -> - shellEnvText = getRawShellEnv() - return unless shellEnvText? - - env = {} - - for line in shellEnvText.split(os.EOL) - if line.includes("=") - components = line.split("=") - if components.length is 2 - env[components[0]] = components[1] - else - k = components.shift() - v = components.join("=") - env[k] = v - - env diff --git a/src/environment.js b/src/environment.js new file mode 100644 index 000000000..00c112bda --- /dev/null +++ b/src/environment.js @@ -0,0 +1,94 @@ +'use babel' + +import {spawnSync} from 'child_process' +import os from 'os' + +// Gets a dump of the user's configured shell environment. +// +// Returns the output of the `env` command or `undefined` if there was an error. +function getRawShellEnv () { + let shell = getUserShell() + + // The `-ilc` set of options was tested to work with the OS X v10.11 + // default-installed versions of bash, zsh, sh, and ksh. It *does not* + // work with csh or tcsh. + let results = spawnSync(shell, ['-ilc', 'env'], {encoding: 'utf8'}) + if (results.error || !results.stdout || results.stdout.length <= 0) { + return + } + + return results.stdout +} + +function getUserShell () { + if (process.env.SHELL) { + return process.env.SHELL + } + + return '/bin/bash' +} + +// Gets the user's configured shell environment. +// +// Returns a copy of the user's shell enviroment. +function getFromShell () { + let shellEnvText = getRawShellEnv() + if (!shellEnvText) { + return + } + + let env = {} + + for (let line of shellEnvText.split(os.EOL)) { + if (line.includes('=')) { + let components = line.split('=') + if (components.length === 2) { + env[components[0]] = components[1] + } else { + let k = components.shift() + let v = components.join('=') + env[k] = v + } + } + } + + return env +} + +function needsPatching (options = { platform: process.platform, env: process.env }) { + if (options.platform === 'darwin' && !options.env.PWD) { + let shell = getUserShell() + if (shell.endsWith('csh') || shell.endsWith('tcsh')) { + return false + } + return true + } + + return false +} + +function normalize (options = {}) { + if (options && options.env) { + process.env = options.env + } + + if (!options.env) { + options.env = process.env + } + + if (!options.platform) { + options.platform = process.platform + } + + if (needsPatching(options)) { + // Patch the `process.env` on startup to fix the problem first documented + // in #4126. Retain the original in case someone needs it. + let shellEnv = getFromShell() + if (shellEnv && shellEnv.PATH) { + process._originalEnv = process.env + process.env = shellEnv + } + } +} + +export default { getFromShell, needsPatching, normalize } diff --git a/src/initialize-application-window.coffee b/src/initialize-application-window.coffee index cea4e1c3c..10f321ded 100644 --- a/src/initialize-application-window.coffee +++ b/src/initialize-application-window.coffee @@ -4,7 +4,7 @@ module.exports = ({blobStore}) -> require './window' {getWindowLoadSettings} = require './window-load-settings-helpers' - {resourcePath, isSpec, devMode} = getWindowLoadSettings() + {resourcePath, isSpec, devMode, env} = getWindowLoadSettings() # Add application-specific exports to module search path. exportsPath = path.join(resourcePath, 'exports') @@ -21,6 +21,7 @@ module.exports = ({blobStore}) -> applicationDelegate: new ApplicationDelegate, configDirPath: process.env.ATOM_HOME enablePersistence: true + env: env }) atom.startEditorWindow().then -> diff --git a/src/project.coffee b/src/project.coffee index 2a21aa2d8..340075a1b 100644 --- a/src/project.coffee +++ b/src/project.coffee @@ -274,26 +274,6 @@ class Project extends Model contains: (pathToCheck) -> @rootDirectories.some (dir) -> dir.contains(pathToCheck) - ### - Section: Environment - ### - - # Public: Retrieves a normalized copy of the environment. - # - # On OS X, the `PATH` can be different depending on whether Atom is launched - # from the Dock, Finder, Spotlight or the terminal. This detects how Atom was - # started and corrects the `PATH` environment variable before returning a copy - # of the environment. - getEnv: -> - unless @env? - @env = _.clone(process.env) - if process.platform is "darwin" and not process.env.TERM? - {getShellEnv} = require("../src/environment") - shellEnv = getShellEnv() - @env = shellEnv if shellEnv? - - _.clone(@env) - ### Section: Private ###