Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Make updateProcessEnv async and add core:loaded-shell-environment activation hook #13200

Merged
merged 2 commits into from
Nov 11, 2016

Conversation

as-cii
Copy link
Contributor

@as-cii as-cii commented Nov 11, 2016

Fixes #9406.
Fixes #13084.

When Atom is launched from the Dock, we invoke the user shell to retrieve environment variables that are assigned by their profile settings. However, we were doing so synchronously during startup, thus causing a major slowdown when opening Atom from the Dock and sometimes hanging the startup process indefinitely.

With this pull request, we are changing updateProcessEnv into an asynchronous method. Packages that require shell environment variables to be assigned prior to activation can use the new core:loaded-shell-environment activation hook by putting the following snippet in package.json:

{"activationHooks": ["core:loaded-shell-environment"]}

This will defer packages' activation until process.env has been updated with the user environment variables.

/cc: @atom/maintainers

Antonio Scandurra added 2 commits November 11, 2016 18:39
Signed-off-by: Nathan Sobo <nathan@github.com>
Signed-off-by: Nathan Sobo <nathan@github.com>
if (error) {
if (error.handle) {
error.handle()
}
console.log('warning: ' + env.SHELL + '-ilc "command env" failed with signal (' + error.signal + ')')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible that the exit code is non-zero but there is no error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately execFile doesn't expose a status property. However, in my testing it seems to be reporting an error when commands exit with non-zero codes (e.g. for a bogus option like ls -j).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, got it. Makes sense...

@as-cii as-cii merged commit c4fb0d9 into master Nov 11, 2016
@as-cii as-cii deleted the as-ns-update-env-async branch November 11, 2016 18:44
as-cii pushed a commit that referenced this pull request Nov 11, 2016
Make `updateProcessEnv` async and add `core:loaded-shell-environment` activation hook
as-cii pushed a commit that referenced this pull request Nov 11, 2016
Make `updateProcessEnv` async and add `core:loaded-shell-environment` activation hook
facebook-github-bot pushed a commit to facebookarchive/nuclide that referenced this pull request Nov 17, 2016
Summary:
Default mercurial install location is: `/usr/local/bin`.
With atom/atom#13200 - Atom may have unprepared environment when activating packages, which in case of some packages,
trigger `hg` calls.

Reviewed By: vjeux

Differential Revision: D4193211

fbshipit-source-id: ffb6be9fe9b46dc7adf834f258053165db12b36a
@ivankravets
Copy link

Packages that require shell environment variables to be assigned prior to activation can use the new

@as-cii, Could you call this hook when package has been installed via package manager? I've tried to use this hook for @platformio IDE and new release broke installation process. Users need to manually restart Atom and only after it package will be activated.

Thanks!

@joefitzgerald
Copy link
Contributor

@ivankravets you point out a key bug in the current implementation: if a user disables a package dependent on this hook for activation, or if a user installs a new package dependent on this hook, or if a user updates a package dependent on this hook, it will fail to activate.

@ivankravets
Copy link

I made hot fix for PlatformIO IDE that adds this hook ONLY when Atom is >=1.12.2 and ony after install/upgrade procedure. Default IDE's manifest doesn't contain activation hooks. Nevertheless, IDE will add them after first installation.

See https://github.com/platformio/platformio-atom-ide/blob/develop/lib/maintenance.js#L78:L94

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants