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

Use fix-path in kernel.coffee #34

Closed
wants to merge 1 commit into from
Closed

Conversation

rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented May 25, 2015

Though I have an objection to forcing a new PATH on users who set an explicit one in launching Atom directly, this does make it easier for new users.

Addresses #28.

Couple notes:

  • this will overwrite any PATH the user set before launching Atom at the command line PATH=~/myvirtualenv. That makes virtualenvs and conda environments a no go.
  • fix-path forces some node modules on the front unconditionally (on OSX and Linux) if shell-path doesn't return a path back

However, I like the sh -c echo $PATH trick here. Wondering if there's a way to find out if Atom was launched via launchctl...

@rgbkrk
Copy link
Member Author

rgbkrk commented May 25, 2015

With the realization of the PATH being overwritten for virtualenvs (or anything else that relies on PATH), I'm against adding this directly. With some launchctl checking we could roll with this.

@rgbkrk
Copy link
Member Author

rgbkrk commented May 25, 2015

Well, there's a way but it's hacky.

screenshot 2015-05-25 15 45 11

cp.exec('launchctl list | grep ' + process.pid, function(err,stdout,stderr){console.log(stdout);})

@willwhitney
Copy link
Collaborator

I'm not comfortable shipping a solution this hacky — it feels like a bug waiting to happen. Thanks for spending the time figuring this out!

I'll write this up as a bug against Atom. Doing this properly once seems like a clear win for the platform.

@rgbkrk rgbkrk deleted the fixpath branch May 26, 2015 03:44
@rgbkrk
Copy link
Member Author

rgbkrk commented May 26, 2015

Definitely appropriate. I'll bet someone has posted the bug - there's got to be a way for Atom to detect being launched by launchctl and performing its own tomfoolery to get the PATH. There are lots of packages that need this (go-plus, atom-script, travis, terminal, the list goes on).

@willwhitney
Copy link
Collaborator

Couldn't find the issue already opened, so I submitted atom/atom#6956.

@rgbkrk
Copy link
Member Author

rgbkrk commented May 26, 2015

Went ahead and finished up the hacky solution earlier, figured I should post it anyway:

if process.platform == "darwin"

  childProcess = require('child_process')
  shell = process.env.SHELL || '/bin/sh'

  childProcess.exec("launchctl list", launchCtlPathFix)

  launchCtlPathFix = (error, stdout) ->
    if error and error.code != 0
      # Launchctl errored
      console.log("Launchctl errored, not patching path:")
      console.log(stderr)
      console.error(error)
      return

    if stdout
      # process.pid was in the list, make sure that it was a reasonable match
      output = stdout.trim()
      lines = output.split('\n')

      # Verify that at least one of the lines matches the exact pattern of
      # process.pid\t[0\-]\t.*\.Atom
      launchRegex = ///^#{process.pid}\t[0\-]\t.*\.Atom///
      matches = (line for line in lines when launchRegex.exec(line))

      if matches.length > 0
        # Now we think we can fix the path since launcctl was clearly running
        console.error("Atom was launched from Launchctl, attempting to shim PATH")
        childProcess.execFile(shell, ['-c', 'echo $PATH'], patchPath)
      else
        console.log("Your Atom process wasn't in the launchctl list. Using your $PATH as is.")

  patchPath = (error, stdout, stderr) ->
    if (error)
      console.error("Unable to get $PATH")
      console.error("stderr: #{stderr}")
      return

    console.log("Old $PATH: #{process.env.PATH}")
    process.env.PATH = stdout.trim()
    console.log("New $PATH: #{process.env.PATH}")

Should probably accept a callback on finish, emit an event, or use promises, but hey it's just some throwaway code.

@rgbkrk
Copy link
Member Author

rgbkrk commented May 26, 2015

More launchctl tomfoolery:

$ launchctl getenv PATH
/usr/bin:/bin:/usr/sbin:/sbin
$ launchctl setenv PATH $PATH
$ launchctl getenv PATH
./bin:/usr/local/bin:/usr/local/sbin:/Users/kyle6475/.dotfiles/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/opt/X11/bin:/usr/local/opt/go/libexec/bin:/Users/kyle6475/.rbenv/bin:/Users/kyle6475/.rbenv/shims:/Users/kyle6475/go/bin

@rgbkrk
Copy link
Member Author

rgbkrk commented May 26, 2015

Now for my new init script.

try
  childProcess = require('child_process')

  atomProcs = childProcess.execSync("launchctl list | grep #{process.pid} | grep Atom").toString().split()
  launchRegex = ///^#{process.pid}\t[0\-]\t.*\.Atom///
  matches = (line for line in atomProcs when launchRegex.exec(line))

  if matches.length > 0
    console.log("Fixing paths for Atom")
    process.env.PATH = childProcess.execFileSync('/bin/bash', ['-c', 'echo $PATH']).toString().trim()
    process.env.GOPATH = childProcess.execFileSync('/bin/bash', ['-c', 'echo $GOPATH']).toString().trim()

catch exception
  console.error("Exception during PATH fixes: #{exception}")

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

Successfully merging this pull request may close these issues.

None yet

2 participants