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 a file to store environment variables to activate for virtualenv generators #5989

Merged
merged 71 commits into from Dec 4, 2019

Conversation

@jgsogo
Copy link
Member

jgsogo commented Oct 28, 2019

Changelog: Feature: Environment variables for virtual environments are stored in .env files containing just the key-value pairs. It will help other processes that need to read these variables to run their own commands.
Changelog: Bugfix: Conan's virtualenvironments restore the environment to the state it was before activating them (previously it was restored to the state it was when the conan install was run).
Docs: omit

#5821

  • This PR replaces the activate/deactivate scripts for virtualenvs. It adds a environment...env file with the variables to modify and the scripts read them from this file.
  • Deactivate script restores the environment to the state it was before activating (bugfix), which may fix some use-cases.
  • TODO: prompts

For future PR:

  • Refactor of the VirtualEnvGenerator to remove duplicated code
  • Identify the OLD_XXX variables with a unique id like OLD_<unique>_XXX, this way the user would be able to activate/deactivate nested environments (user should take care of the stack order, FILO)

Useful links:

jgsogo added 30 commits Sep 26, 2019
Copy link
Contributor

lasote left a comment

I think this is looking good and we should push to be on the release. Fix the PROMPT issues you mentioned in windows, remove the WIP/draft, and please @memsharded review.

conans/client/generators/virtualenv.py Outdated Show resolved Hide resolved
been set by the 'activate' script doesn't exist when we run 'deactivate' in a
different shell...
TODO: Remove this test

This comment has been minimized.

Copy link
@lasote

lasote Dec 2, 2019

Contributor

Sounds like a useless test, but to confirm, shouldn't linux and mac be affected also?

@@ -156,6 +156,7 @@ def _run_virtualenv(self, generator):
with environment_append({"PATH": [self.ori_path, ]}):
# FIXME: I need this context because restore values for the 'deactivate' script are
# generated at the 'generator.content' and not when the 'activate' is called.

This comment has been minimized.

Copy link
@lasote

lasote Dec 2, 2019

Contributor

Could we remove the context then?

jgsogo added 3 commits Dec 2, 2019
…s are computed at the time of activating the environment
@jgsogo jgsogo changed the title [WIP] Use a file to store environment variables to activate for virtualenv generators Use a file to store environment variables to activate for virtualenv generators Dec 2, 2019
@jgsogo jgsogo marked this pull request as ready for review Dec 2, 2019
Copy link
Member

memsharded left a comment

I am not sure that restoring only the variables modified by the venv is the most evident behavior:

  • If the venv modifies the PATH env-var, then user changes to the PATH are lost when the venv is deactivated
  • But if the venv does not modify the PATH env-var the user changes to the PATH when inside the venv are maintained after deactivation.

This sounds asymmetric and prone to difficult to debug errors. I would suggest to make virtualenvs a sandbox of environment variables, when it is deactivated, the exact environment that was active when activated is restored.

Copy link
Member

memsharded left a comment

It is looking a good improvement.
I have a minor concern regarding changes in the quotes. Some quotes that were being used are no longer there. I guess this has been verified and it will not be breaking for cases with path with spaces, etc.

If this is ok, it is good, it could be merged.


if platform.system() != "Windows":

contents = load(os.path.join(client.current_folder, "environment_run_python.sh.env"))

This comment has been minimized.

Copy link
@memsharded

memsharded Dec 3, 2019

Member

client.load("environment_run_python.sh.env")

This comment has been minimized.

Copy link
@jgsogo

jgsogo Dec 3, 2019

Author Member

There are many load(os.path.join(client.current_folder in the codebase, let's leave it for a dedicated PR

This comment has been minimized.

Copy link
@memsharded

memsharded Dec 4, 2019

Member

Sure, there are like 40 remaining, I already fixed like a couple of hundreds in another PR. We don't need to fix them in this PR, but new tests would benefit from simpler syntax, I see no reason why not using it already in this PR

This comment has been minimized.

Copy link
@lasote

lasote Dec 4, 2019

Contributor

I'll merge and create engineering issue.

conans/client/generators/virtualenv.py Show resolved Hide resolved
@lasote lasote merged commit 82ab228 into conan-io:develop Dec 4, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
Conan virtualenvs automation moved this from High priority to Closed Dec 4, 2019
@jgsogo jgsogo deleted the jgsogo:feat/environment-vars branch Dec 4, 2019

activate, deactivate = self._ps1_lines()
activate, deactivate, envfile = self._ps1_lines()

This comment has been minimized.

Copy link
@ytimenkov

ytimenkov Dec 4, 2019

Contributor

There is a PowerShell (Core) for Linux and Mac...

@jgsogo

This comment has been minimized.

Copy link
Member Author

jgsogo commented Dec 5, 2019

Hi, @ytimenkov . Never is too late, and your considerations are very valuable. Let me try to comment some of your points (I'm moving from the comment inline in the code to the main conversation in the PR).

Remembering old variables is done here explicitly, while setting new vars are read from the file.

I agree with you, and I thought about how to make it more consistent. In the file there are only the new or modified variables, while we want to remember only the modified ones... I probably could do some kind of if in the bash script, but it was easier to do it in Python before writing the file than doing it in cmd/sh/ps1.

Besides, I can't find quickly where concatenation with the current environment happens...

Concatenation is done if the variable is a list, as it was before: https://github.com/conan-io/conan/pull/5989/files/a65f89468ce0e9f10c0b664d6e3b1ea8bad1f0ea. Thanks to you there are tests that are actually checking that.

Major problem I have with virtual environment generator using os.environ is that it captures variables at the moment of calling conan install. So if I (happen to) run it from an activated environment (probably from another project or with another settings / options) the activate and deactivate scripts will be a total mess.

Yes, this is a problem and the next step to consider is this one #6175 (probably for 2.0 as it might break someone). With this new implementation the old values are remembered when the environment is activated not when conan install runs. It could be breaking, but it is improving current behavior.

Also, as you mention, right now it is not possible to activate nested environments (remembered values will be overridden), but, if we use unique keys for each environment (CONAN_OLD_<unique>_****) then we could have nested environments, although the user will still be responsible of activating/deactivating them in the proper order (FILO stack).

@ytimenkov

This comment has been minimized.

Copy link
Contributor

ytimenkov commented Dec 6, 2019

I've got a crazy idea yesterday :)

You've already introduced Jijna templates (which I think is really good), so why not to make a step further and move generation of activation / deactivation files altogether to Jinja Templates?

I mean, all that logic for generating files could be done completely in Jinja. virtualenv generator just prepares variables, sets them into some Jinja context or environment and then just renders the template. If needed some helper functions could be also provided as some env=os.,environ if needed.

Then we do one more step and allow overriding it by user, like now with hooks or layouts.
Like some setting virtualenv.templates = activate.sh.in, deactivate.sh.in, ... and then the generator just goes through the list and renders each template. No need for any conditions in the generator itself. (I don't see any harm in generating .bat file on Linux, at the same time : vs ; and / vs \ are specific to the file type not the os: .cmd always uses semicolon, while sh always uses /).

This way we have 100% compatibility (if default template produces the same scripts as today) while users getting good control for overriding this behavior.


from conans.client.tools.oss import OSInfo
from conans.model import Generator


sh_activate_tpl = Template(textwrap.dedent("""
#!/usr/bin/env bash

This comment has been minimized.

Copy link
@ytimenkov

ytimenkov Dec 6, 2019

Contributor

Noticed this...
I don't know why, but our GitLab executes its commands using POSIX shell, not bash.

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