Prepare for 1.0.0 release

[ioquatix]

This code is a quick hack/extract of the existing code from script-runner. and was motivated by Glavin001/atom-beautify#164 (comment)

I would suggest the only things that need to really be addressed:

1. Naming and scope - is this the correct name and is this module covering the right set of functionality?
2. Platform support - it is currently borked on Windows - should we simply return platform.env on windows?
3. API - only one function.. should we provide some way to override the shell or exports function called? Perhaps an options hash?
4. Testing - can we improve the testing? I've done some basic stuff.
5. General code stuff - this is my first NPM module and I welcome feedback about it's structure.

/cc @Glavin001 @huba

[Glavin001]

My current version for the next release of Atom Beautify: https://github.com/Glavin001/atom-beautify/blob/240368fb09efaf67b31f10f0cf7f3fdba46fa65f/src/beautifiers/beautifier.coffee#L80-L131

###
    Get Shell Environment variables
    Special thank you to @ioquatix
    See https://github.com/ioquatix/script-runner/blob/v1.5.0/lib/script-runner.coffee#L45-L63
    ###
    _envCache: null
    _envCacheDate: null
    _envCacheExpiry: 10000 # 10 seconds
    getShellEnvironment: ->
        return new @Promise((resolve, reject) =>
            # Check Cache
            if @_envCache? and @_envCacheDate?
                # Check if Cache is old
                if (new Date() - @_envCacheDate) < @_envCacheExpiry
                    # Still fresh
                    return resolve(@_envCache)

            # Check if Windows
            isWin = /^win/.test(process.platform)
            if isWin
                # Windows
                # Use default
                resolve(process.env)
            else
                # Mac & Linux
                # I tried using ChildProcess.execFile but there is no way to set detached and
                # this causes the child shell to lock up.
                # This command runs an interactive login shell and
                # executes the export command to get a list of environment variables.
                # We then use these to run the script:
                child = spawn process.env.SHELL, ['-ilc', 'env'],
                  # This is essential for interactive shells, otherwise it never finishes:
                  detached: true,
                  # We don't care about stdin, stderr can go out the usual way:
                  stdio: ['ignore', 'pipe', process.stderr]
                # We buffer stdout:
                buffer = ''
                child.stdout.on 'data', (data) -> buffer += data
                # When the process finishes, extract the environment variables and pass them to the callback:
                child.on 'close', (code, signal) =>
                    if code isnt 0
                        return reject(new Error("Could not get Shell Environment. Exit code: "+code+", Signal: "+signal))
                    environment = {}
                    for definition in buffer.split('\n')
                        [key, value] = definition.split('=', 2)
                        environment[key] = value if key != ''
                    # Cache Environment
                    @_envCache = environment
                    @_envCacheDate = new Date()
                    resolve(environment)
          )

It's Promise-style using Bluebird.

We can convert it over to callback-style for shell-environment package.

It supports Window (falls back to process.env), Linux/Mac (your code), and caching (using shell-environment code about doubled my unit tests execution time for Atom Beautify, and I was able to cut it back in half to normal by caching the result).

[ioquatix]

Regarding caching

I don't believe caching belongs in the core API of this library. It might be the case that we can provide an easy caching wrapper though. I do understand the value of caching the results.

The main reason against caching is that some tools change the login environment, e.g. rvm which sets the default ruby interpreter, and can be changed. If you used script-runner and it wasn't using the default ruby interpreter it would probably be considered a bug.

I don't really know what to do in this area, but perhaps there is an easy module for adding caching support or perhaps we should roll our own? Or perhaps we leave it up to application code to make their own caching policy - it isn't that hard?

Regarding windows support

Please feel free to add windows support to the existing code.

Regarding promises

If this makes sense let's make a PR after all the other issues are addressed.

The main thing which counts for it IMHO is consistency and ease of use. The thing which counts against it is integration with other systems which might not be able to use that dependency or might want to keep dependencies to a minimum.

[ioquatix]

So, on the following points

1/ Naming - changed name to shell-environment.
2/ @Glavin001 to add windows support to existing function.
3/ Needs more discussion.
4/ @Glavin001 did you have any ideas for improved testing?
5/ Everyone feel free to give feedback.
6/ Promises - let's think about whether this makes sense in this low level API... @Glavin001 ?
7/ Caching - perhaps if we add options per 3/ we could add support for caching, or perhaps another entry point which includes caching, or perhaps something else?

[Glavin001]

1/ Naming - changed name to shell-environment.

Totally agree.

2/ @Glavin001 to add windows support to existing function.

At least we detect Windows now and don't run the detecting environment code, that only works on Linux, on Windows

4/ @Glavin001 did you have any ideas for improved testing?

We could run this in Travis CI. Let's inquire about their Multiple-OS feature: http://docs.travis-ci.com/user/multi-os/
Then we can test it on multiple OSs and make it work on Linux and Mac. I do not believe Travis CI has Windows support though.
I don't have a Windows computer, however I could boot up a virtual machine to test later.

6/ Promises - let's think about whether this makes sense in this low level API... @Glavin001 ?

We can probably stick with callback style and the user can make it promise style if they like by wrapping it with promises.

7/ Caching - perhaps if we add options per 3/ we could add support for caching, or perhaps another entry point which includes caching, or perhaps something else?

I like having caching and while it is kind of out of scope for this, I think it adds value (since this does take time to process) and would be nice to be reusable, since I may not be the only user who wants it to cache. This way developers, like ourselves, would not be required to implement their own caching layer.

[ioquatix]

@huba do you want to have a go at integrating this into script-runner?

[huba]

@ioquatix working on it.

[huba]

So I'm still getting these https://gist.github.com/huba/6849a7d00475974bfe1e but this time I'm getting useful key-value pairs too.

[ioquatix]

I guess the logic for extracting values needs to be improved. One option is to use the "unique identifier" idea I had. Another is to improve the split('=') logic which is probably a bit foobar. Can you give me the entire output of bash -ilc env and I will make a unit test to verify the correct behaviour.

[huba]

Do you think there is any way to do something like bash -il and wait for the prompt, write to stdin and only start reading the output after that?

[ioquatix]

Yes, this was one of my other ideas:

#!/usr/bin/env ruby

read_io, write_io = IO.pipe

puts "Starting login shell.."

pid = Process.spawn 'bash -ilc ls -lah; env >&4', 1 => :close, 4 => write_io

write_io.close

puts "Waiting to finish.."

Process.wait

puts "Reading output of fd 4..."
puts read_io.read.inspect

The output of env goes into fd 4 only. So, you can just read from that. Whether or not that works in node.js/child_process is another thing entirely.

[ioquatix]

If you want to have a go at implementing that in Node.js I think that would be a good solution. The only problem I can see is that not all shells will have the same output redirection, i.e. env >&4 .. but that seems to work for me on zsh and bash..

[huba]

Yea there are over 9000 shells (exaggeration) to consider. It seems to work just as bash -il "env>file" just from the terminal. It gets rid of the login script getting piped into the file, I guess we could make something like the commandMap in script-runner, for all the known ones that are different and a generic one. Because some shells might not even take the -il options.

[ioquatix]

Yeah, that's why making a generic script-env.py might make sense... It should be possible to guarantee that fd 4 is writable from python and then dump os.env or whatever it is in python out via fd 4. It wouldn't depend on anything except having some kind of login shell. That login shell is already provided via process.env.SHELL so in theory as long as that shell is set to something that understands -ilc we would be okay.. The point you seem to be arguing is that -ilc might not be supported. In that case, if the command fails to execute (i.e. non-zero status), let's just return process.env. In the future, we could add some additional options to the function to handle cases other than -ilc or as you say, perhaps we can look at basename(process.env.SHELL) in a map to get the appropriate options, etc.

I think in any case, bash and zsh cover like 99% of all shell users, so lets just try to get that part right to start with, and then deal with the edge cases if bugs are reported. I'm pretty sure, except for really obscure shells, -ilc should be compatible?

[ioquatix]

So I'm still getting these https://gist.github.com/huba/6849a7d00475974bfe1e but this time I'm getting useful key-value pairs too.

So, the reason why this happens when you run it in atom.. using script-runner.. is because script-runner has environment variables being defined incorrectly, and the shell process is picking them up.

[ioquatix]

Okay, so I'm going to close this issue now that we are officially using this in script runner. But I'm going to open separate issues for any outstanding things.