Skip to content
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

Calculate env vars when launching Atom from the desktop #19138

Merged
merged 1 commit into from Apr 11, 2019

Conversation

Projects
None yet
1 participant
@rafeca
Copy link
Contributor

commented Apr 11, 2019

Identify the Bug

Atom has some logic to get the correct env variables when calling directly the Atom binary (the one located in /usr/share/atom/atom).

This logic is bypassed when calling Atom via the bash CLI (the script located in /usr/bin/atom). This is done by checking the ATOM_DISABLE_SHELLING_OUT_FOR_ENVIRONMENT env var which is set by the CLI (there's more info in the PR that introduced this).

This logic assumes that entry points that don't contain the correct env vars (like a desktop launcher) will call the binary directly, while a shell script or terminal (which contain the correct env vars) will call the CLI. After #17508, this is no longer true, and this is the root cause of the issue.

Applicable issue: #18318

Description of the Change

This PR makes Atom keep using the previous logic to calculate env vars instead. To do so, it modifies the linux launcher to define the ATOM_DISABLE_SHELLING_OUT_FOR_ENVIRONMENT env var and changes the bash script to not define it if it's already set.

Alternate Designs

  • Create a different bash script which is only used by desktop launchers. This will require more changes and it does not provide any major benefit.
  • Change the bash script to use bash in interactive mode. This can cause unexpected behaviours in some configurations (depending on the users' .bashrc files).

Possible Drawbacks

None that I can think of.

Verification Process

  • Patched a working copy of Atom in a linux machine and checked that it works correctly.
  • Download an Atom build from this PR and verify it.

Release Notes

Fixed issue inheriting environmental variables when opening Atom from the desktop on Linux.

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Download an Atom build from this PR and verify it.

Uhm, it seems that we don't create artifacts on builds for PRs, so if nobody opposes, I'll do the last bit of the verification process after this PR gets merged into master.

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

CI build passed, I'm gonna merge this one for now and check that the next Linux build on master works fine.

/cc @atom/core

@rafeca rafeca merged commit 3f2a702 into master Apr 11, 2019

2 checks passed

Atom Pull Requests #20190411.1 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rafeca rafeca deleted the maintain-env-vars branch Apr 11, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I've verified that the functionality works correctly on Ubuntu by downloading the .dev binary generated by the azure pipelines on commit 3f2a702

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.