Do a small builder refactor. #1924
Conversation
| @@ -180,7 +180,6 @@ def setup_base(self): | |||
| '--name', | |||
| self.version.slug, | |||
| 'python={python_version}'.format(python_version=self.config.python_version), | |||
There was a problem hiding this comment.
Probably can lose this comma, but it's not a big deal.
|
Looks cool. Should there be something for virtualenv, as well? Not sure if that's being used. It seems to just add to the correct python to the path so would be simple. ( https://github.com/pypa/virtualenv/blob/18b374dc3eeb0e37aa3ad1b22c442712d9dca103/virtualenv_embedded/activate.sh ) |
|
True. We're still injecting the bin_path manually, because it varies by command, and we don't want to override the default $PATH -- so bin path's will still be there. We should be setting the Virtualenv default PATH though, +1 |
| @@ -229,6 +222,43 @@ def setup_vcs(self): | |||
| raise BuildEnvironmentError('Failed to import project', | |||
| status_code=404) | |||
|
|
|||
| def get_bash_env(self): | |||
| """ | |||
| Get bash environment variables used for all builder commands. | |||
There was a problem hiding this comment.
BuildCommand defaults to shell=False, which should be what we're using for almost all commands. Naming/referencing bash here is misleading, these are just env variables as far as subprocess is concerned.
There was a problem hiding this comment.
We need some way to name them different than the 2 other types of environments we use. Any suggestions?
There was a problem hiding this comment.
env_vars + get_environment_variables would make sense here
|
It might be good to include a test to ensure how we're settings |
|
The only other point I would raise is the same one we already discussed, @ericholscher. That is we now may have an environment active before it is created. I'm not sure what this will do for virtualenv. With |
|
@jakirkham I went ahead and fixed this by passing an explicit bin_path=None to the environment creation commands. Nice catch. |
Do a small builder refactor.
This adds: