Simplify Environment Patching #12562

joefitzgerald · 2016-08-31T06:16:37Z

This PR simplifies the logic used to determine when to patch the environment for users of linux and macOS. Instead of using a whitelist of shells and trying to detect how Atom was launched with environment variable markers, we simply modify atom.sh to set a environment variable that is only set when launched from the shell. If this environment variable is not detected, the environment is patched with the environment from a new, interactive, login shell.

Stop whitelisting shells
Introduce ATOM_SUPPRESS_ENV_PATCHING environment variable, which is only set in atom.sh
Manually test on Ubuntu
Manually test on macOS

Fixes #11910
Fixes #12535

thomasjo · 2016-08-31T07:47:11Z

LGTM™ 🚀

thomasjo · 2016-09-07T15:58:30Z

Let's get this in before rolling the 🚃 🚃 ? /cc @atom/feedback @nathansobo @lee-dohm

nathansobo · 2016-09-08T16:21:52Z

src/update-process-env.js

@@ -36,6 +33,10 @@ function updateProcessEnv (launchEnv) {
    for (let key in envToAssign) {
      if (!ENVIRONMENT_VARIABLES_TO_PRESERVE.has(key)) {
        process.env[key] = envToAssign[key]
+      } else {
+        if (!process.env[key] && envToAssign[key]) {


So we'll always assign "preserved" environment variables if they're not already defined?

Seems like this duplicated assignment could be eliminated by making the conditional:

if (!ENVIRONMENT_VARIABLES_TO_PRESERVE.has(key) || !process.env[key]) { process.env[key] = envToAssign[key] }

This is related to #12562 (diff) but I think this new behavior may be too pessimistic. It would prevent the changing of an existing value in process.env which is not the expected behavior. I will write up a spec to validate this assumption and fix if true.

nathansobo · 2016-09-08T16:23:39Z

If I'm reading this correctly, it seems like the name of the variable ATOM_SUPPRESS_ENV_PATCHING is a bit misleading. We will update the process.env of an existing instance of Atom regardless of this flag. The flag indicates whether we should attempt to invoke the user's shell to retrieve the environment... am I right about that?

nathansobo · 2016-09-08T16:27:02Z

What about an environment variable like ATOM_LAUNCHED_FROM_SHELL. That seems like it would more clearly communicate the intent unless I'm missed something.

nathansobo · 2016-09-08T16:28:17Z

spec/update-process-env-spec.js

+        expect(shouldGetEnvFromShell({SHELL: '/bin/zsh'})).toBe(true)
+        expect(shouldGetEnvFromShell({SHELL: '/usr/local/bin/zsh'})).toBe(true)
+        expect(shouldGetEnvFromShell({SHELL: '/bin/fish'})).toBe(true)
+        expect(shouldGetEnvFromShell({SHELL: '/usr/local/bin/fish'})).toBe(true)
      })

      it('returns false when the shell should not be patched', function () {


Maybe this should be phrased returns false when the environment indicates that Atom was launched from a shell

joefitzgerald · 2016-09-08T16:45:34Z

If I'm reading this correctly, it seems like the name of the variable ATOM_SUPPRESS_ENV_PATCHING is a bit misleading. We will update the process.env of an existing instance of Atom regardless of this flag. The flag indicates whether we should attempt to invoke the user's shell to retrieve the environment... am I right about that?

There are two separate behaviors going on in update-environment:

We are potentially patching the environment by launching a new, interactive, login shell and using that environment - if Atom was not launched from the shell
We are allowing multiple invocations of atom from the shell to update the environment with new environment values; we are also allowing for the scenario where you open atom to a directory, and then subsequently open that same directory via atom ./thedir and allowing the environment to be updated with the environment from the shell

So, there's patching, and there's updating of the environment. They are different. Suppressing the patching should not suppress the updating. Maybe we need new verbs ;)

joefitzgerald · 2016-09-08T16:53:36Z

Also worth mentioning: it should be possible for a user to set ATOM_SUPPRESS_ENV_PATCHING in their environment used by launchd to run apps launched by Dock / Finder / Spotlight to completely suppress environment patching. I believe this will be the minority of users, but I suspect some users will want to do this. It would be nice to provide some other configuration file that could be used to suppress this behavior - possibly residing in ATOM_HOME as a future extension / in a subsequent PR.

- Stop using shell whitelist

- Fix lint warnings

- 🎨 Update spec description to more accurately reflect the intent

- The additional (!process.env[key] && envToAssign[key]) check allows “preserved” variables to be set for the first time if they are currently unset

nathansobo · 2016-09-08T18:07:48Z

src/update-process-env.js

@@ -34,7 +31,7 @@ function updateProcessEnv (launchEnv) {
    }

    for (let key in envToAssign) {
-      if (!ENVIRONMENT_VARIABLES_TO_PRESERVE.has(key)) {
+      if (!ENVIRONMENT_VARIABLES_TO_PRESERVE.has(key) || (!process.env[key] && envToAssign[key])) {


Why would envToAssign[key] be falsy? You're thinking like a blank string environment variable? The key definitely exists because we're looping over keys in the line above.

Suppose you launch atom from Finder. ATOM_SUPPRESS_ENV_PATCHING is not set. It is also a "preserved" environment variable. If you were to subsequently launch Atom via the shell, it would never be set, and would not be preserved in future invocations.

Right, that explains the !process.env[key] part of the expression but not the && envToAssign[key]. It's minor though. I'll just merge anyway.

For posterity: That last part is unnecessary because key is always unique.

nathansobo · 2016-09-08T18:12:08Z

Maybe we need new verbs ;)

We definitely need new verbs. Patching and updating are the same thing. We need a clearer description of what we're suppressing that focuses on how we obtain the thing we're using to update the current environment... DISABLE_SHELLING_OUT_FOR_ENVIRONMENT? I'm not happy with the clarity of the existing naming.

joefitzgerald · 2016-09-08T18:26:49Z

Renamed ATOM_SUPPRESS_ENV_PATCHING to ATOM_DISABLE_SHELLING_OUT_FOR_ENVIRONMENT.

nathansobo · 2016-09-08T19:55:36Z

Thanks @joefitzgerald. This definitely feels like a more robust approach.

joefitzgerald added the enhancement label Aug 31, 2016

joefitzgerald assigned lee-dohm Aug 31, 2016

joefitzgerald force-pushed the jf-patch-linux-environment branch from 932c1dd to 8a8ddaf Compare August 31, 2016 06:21

nathansobo reviewed Sep 8, 2016
View reviewed changes

joefitzgerald added 5 commits September 8, 2016 10:56

Get Environment From Shell On Linux

c570e14

Use ATOM_SUPPRESS_ENV_PATCHING Environment Variable

1027060

- Stop using shell whitelist

Prevent Regression For Common Shells

b321ab6

- Fix lint warnings

Add Spec To Validate Updating An Existing Env Var

3c7a89e

- 🎨 Update spec description to more accurately reflect the intent

Combine Check

36291f6

- The additional (!process.env[key] && envToAssign[key]) check allows “preserved” variables to be set for the first time if they are currently unset

joefitzgerald force-pushed the jf-patch-linux-environment branch from 59e4272 to 36291f6 Compare September 8, 2016 17:18

nathansobo reviewed Sep 8, 2016
View reviewed changes

ATOM_SUPPRESS_ENV_PATCHING > ATOM_DISABLE_SHELLING_OUT_FOR_ENVIRONMENT

d4d0524

nathansobo merged commit 6d76608 into master Sep 8, 2016

nathansobo deleted the jf-patch-linux-environment branch September 8, 2016 19:55

steelbrain mentioned this pull request Sep 14, 2016

Don't update the NODE_PATH when updating the environment on launch #12317

Merged

joefitzgerald mentioned this pull request Oct 1, 2016

Atom Unable to install nfs gocode joefitzgerald/go-plus#428

Closed

joefitzgerald mentioned this pull request Nov 11, 2016

Shelling out to collect environment variables is creating runaway 100% CPU processes on Linux #13084

Closed

6 tasks

gotjoshua mentioned this pull request Dec 14, 2016

environment package VS process.env geksilla/atom-fuzzy-grep#39

Closed

lee-dohm mentioned this pull request Dec 23, 2016

Possible internal python unicode issue #13363

Closed

richarddewit mentioned this pull request Jan 11, 2017

Space menu not appearing/workingin Atom 1.13.0 dvcrn/proton#251

Closed

jpinnix mentioned this pull request Jun 13, 2017

Remove environment package dvcrn/proton#248

Closed

mrattner mentioned this pull request Dec 3, 2018

Error in install-dep when using customMatchers UziTech/atom-jasmine3-test-runner#7

Closed

jeybin mentioned this pull request Mar 3, 2019

Object.provide is deprecated. atom/archive-view#70

Closed

This was referenced Apr 11, 2019

[Atom 1.32.0] Does not inherit environment variables when launched not from the command line #18318

Closed

Calculate env vars when launching Atom from the desktop #19138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify Environment Patching #12562

Simplify Environment Patching #12562

joefitzgerald commented Aug 31, 2016 •

edited

Loading

thomasjo commented Aug 31, 2016

thomasjo commented Sep 7, 2016

nathansobo Sep 8, 2016

nathansobo Sep 8, 2016 •

edited

Loading

joefitzgerald Sep 8, 2016

nathansobo commented Sep 8, 2016

nathansobo commented Sep 8, 2016

nathansobo Sep 8, 2016

joefitzgerald commented Sep 8, 2016

joefitzgerald commented Sep 8, 2016

nathansobo Sep 8, 2016

joefitzgerald Sep 8, 2016

nathansobo Sep 8, 2016

skull-squadron Apr 21, 2017

nathansobo commented Sep 8, 2016

joefitzgerald commented Sep 8, 2016

nathansobo commented Sep 8, 2016

Simplify Environment Patching #12562

Simplify Environment Patching #12562

Conversation

joefitzgerald commented Aug 31, 2016 • edited Loading

thomasjo commented Aug 31, 2016

thomasjo commented Sep 7, 2016

nathansobo Sep 8, 2016

Choose a reason for hiding this comment

nathansobo Sep 8, 2016 • edited Loading

Choose a reason for hiding this comment

joefitzgerald Sep 8, 2016

Choose a reason for hiding this comment

nathansobo commented Sep 8, 2016

nathansobo commented Sep 8, 2016

nathansobo Sep 8, 2016

Choose a reason for hiding this comment

joefitzgerald commented Sep 8, 2016

joefitzgerald commented Sep 8, 2016

nathansobo Sep 8, 2016

Choose a reason for hiding this comment

joefitzgerald Sep 8, 2016

Choose a reason for hiding this comment

nathansobo Sep 8, 2016

Choose a reason for hiding this comment

skull-squadron Apr 21, 2017

Choose a reason for hiding this comment

nathansobo commented Sep 8, 2016

joefitzgerald commented Sep 8, 2016

nathansobo commented Sep 8, 2016

joefitzgerald commented Aug 31, 2016 •

edited

Loading

nathansobo Sep 8, 2016 •

edited

Loading