[bug] The new VirtualRunEnv never clean the deactivate script (it should use a temp deactivate file)

[CPickens42]

Environment Details

• Operating System+version: Linux (archlinux)
• Conan version: 1.47.0
• Python version: 3.9

Steps to reproduce (Include if Applicable)

conanfile.txt:

[requires]
fmt/8.0.0
# testing only deps below
doctest/2.4.6

[generators]
CMakeToolchain
CMakeDeps
VirtualRunEnv

$ conan install . --install-folder=.conanvirtualenv --build=missing --profile:build=default
$ source .conanvirtualenv/conanrun.sh
Capturing current environment in .conanvirtualenv/deactivate_conanrunenv-release-x86_64.sh
Configuring environment variables
$ source .conanvirtualenv/deactivate_conanrun.sh
Restoring environment
$ source .conanvirtualenv/conanrun.sh
Capturing current environment in .conanvirtualenv/deactivate_conanrunenv-release-x86_64.sh
Configuring environment variables
$ source .conanvirtualenv/deactivate_conanrun.sh
Restoring environment
Restoring environment

Explanation of the issue

The issue here is, the more we do activate/deactivate the more the string "Restoring environment" will be displayed (and after a while it can be very annoying on my shell) and the more the script deactivate_conanrunenv-release-x86_64.sh will be full. When using the VirtualRunEnv I found that very annoying after a while. I would recommend that:

• the activate script create a temp deactivate file with mktemp
• stores the place of the deactivate file in the env variable CONAN_DEACTIVATE_FILE
• source $CONAN_DEACTIVATE_FILE in the non temp deactivate script.

I could do a PR for linux with that, but since I don't know how to do that on Windows (I don't have Windows installed at all) I think my PR won't be accepted.

@memsharded @lasote @czoido: I think you guys are the one working on this recently (so I hope it is okay to highlight you), what do you think?

[memsharded]

I have tried this in Windows, apparently it is not happening there.
Also inspected the code in .sh, at least first inspection it is not evident why this is happening, but I will like to understand better the root cause.

Also, quick question, where does the conanvirtualenv folder comes from? It is not evident from your code why the scripts would be located there.

[CPickens42]

In my understanding it is happening because of the use of the ">>" operator in bash. See

conan/conan/tools/env/environment.py

Line 345 in 2550445

echo echo Restoring environment >> "{deactivate_file}"

It always append at the end of the same file without removing it. And it does make sense if you want to support multiple activate/deactivate with the same file. But I think this is not the proper way to handle that.

I think to support simultaneous (and multiple) source conanrun.sh a proper way would be to use a temp file per process that does the activate. In my understanding of the current code it is right now a bit unsafe otherwise. You could put the tempfile into a /tmp/conan/ folder for it to be clean. Do you want me to do a PR (just for linux)?

Regarding the .conanvirtualenv folder, I have patched my script. It is actually my install_folder.

[memsharded]

You are right, in .bat it doesn't happen because it has:

echo @echo off > "{deactivate_file}"
echo echo Restoring environment >> "{deactivate_file}"

So the first line removes the previous content of the file. Why this wouldn't be enough to do the same for .sh? That would be simple and remove the issue, wouldn't it?

And it does make sense if you want to support multiple activate/deactivate with the same file. But I think this is not the proper way to handle that.

No, this feature doesn't aim to support multiple deactivation in the same file. For every activation, you should be doing one activation. If this is complex, the composition should happen before, compounding the script, so user only needs to activate 1 and deactivate 1 instead of several (what the conanbuild and conanrun do, which is aggregating other scripts).

I think to support simultaneous (and multiple) source conanrun.sh a proper way would be to use a temp file per process that does the activate. In my understanding of the current code it is right now a bit unsafe otherwise. You could put the tempfile into a /tmp/conan/ folder for it to be clean. Do you want me to do a PR (just for linux)?

This reads a bit unnecessarily complex for me, at this moment, but maybe it is because I still don't fully understand the risk or why would it be unsafe. Clarified the above, that it doesn't aim to do multiple-deactivation, would this still be an issue?

[CPickens42]

This reads a bit unnecessarily complex for me, at this moment, but maybe it is because I still don't fully understand the risk or why would it be unsafe. Clarified the above, that it doesn't aim to do multiple-deactivation, would this still be an issue?

I think a scenario will be easier to grasp the issue. Let's consider that we have the following init script:

cd /tmp
mkdir example_project
cd example_project
virtualenv .pyvirtualenv
source .pyvirtualenv/bin/activate
pip install conan

echo """
[requires]
fmt/8.0.0
# testing only deps below
doctest/2.4.6

[generators]
CMakeToolchain
CMakeDeps
VirtualRunEnv""" > conanfile.txt
conan install . --install-folder=.conanvirtualenv --build=missing --profile:build=default

# The following line is optionnal, it's just here to show that the following bug will happen
# even if we "reset" the deactivate file (the current behavior on Windows)
# if we comment it, the same bug will happen.
sed 's/echo Restoring environment >>/echo Restoring environment >/' -i .conanvirtualenv/conanrunenv-release-x86_64.sh

You then need to open 2 shells, I will simply call them SHELL 1 and SHELL 2. And then, in that order you need to do the following:

In SHELL 1:

cd /tmp/example_project
echo $PATH
source .pyvirtualenv/bin/activate
echo $PATH

In SHELL 2:

cd /tmp/example_project
echo $PATH
source .conanvirtualenv/conanrun.sh
echo $PATH

In SHELL 1:

source .conanvirtualenv/conanrun.sh
echo $PATH
# The bug happen even if I uncomment those lines:
# source .conanvirtualenv/deactivate_conanrun.sh
# echo $PATH

In SHELL 2:

echo $PATH
source .conanvirtualenv/deactivate_conanrun.sh
echo $PATH

At the end of this scenario, the env variables of the SHELL 1 have been exposed to the SHELL 2. Which is very confusing and an unwanted behavior. Here a screenshot of the execution of this scenario on my computer (the number bubble represent the order of the execution of each commands):

I want to note that the Python virtualenv is just here to show that the env variables between SHELL 1 and SHELL 2 are not the same. It can happen in others scenarios.

I think we both agree that programmers tend to use a multi-shell environment. Sometimes I might want to activate some env variables into a shell and then restore my previous state. When I use virtualenvs (from Python, conan etc.) I expect them to be self-contained in my shell. I don't want to have weird bugs because the virtualenv leaked some env variables of my other shell on which I'm messing around with other stuff. Python virtualenv are self-contained, I think conan virtualenv should be as well.

They are two ways of doing this:

1. You preserve _OLD_CONAN_{NAME} env variables (currently how python virtualenv handle this)
2. You preserve a tempfile on which you keep the old env variables

When I thought about it earlier, I think the solution 1) is better to be honest, because we don't depend of the existence of mktemp or the fact that we can create temp files. I think it would be easy to handle, and we could take example on how Python virtualenv handle that. I'm eager to do a draft PR if you are okay with it. I won't touch to the Windows part though.

[CPickens42]

I'm currently checking of how Python virtualenv is doing thing, I think Conan should take example of this.
For linux:
https://github.com/pypa/virtualenv/blob/main/src/virtualenv/activation/bash/activate.sh

For Windows (powershell):
https://github.com/pypa/virtualenv/blob/main/src/virtualenv/activation/powershell/activate.ps1

For others shell:
https://github.com/pypa/virtualenv/tree/main/src/virtualenv/activation

Python virtualenvs proved their resilience over the time. I think that those scripts could be easily be adapted for conan.
Obviously we should use another name than deactivate for shell function to avoid conflict. I think conan_deactivate or something similar should do the trick.

[memsharded]

Ok, thanks very much for the detailed explanation.

Now I understand the issue, I thought we were talking about multiple sequential activation & deactivation of environments. I see that we are talking about concurrent usage of these environment scripts, and it is completely true that the current approach is not designed to support it.

I agree that learning from Python virtualenvs and improving the current approach is the way to go. The only thing is that this will not be a priority at the moment as we are focusing on getting 2.0 out. I'd say to try it in the 2.X scope. Contributions are welcome, of course, but they also require time to review and to understand well what is doing (because the truth is that in the long term we need to support, maintain and evolve it).

[CPickens42]

I understand completely. To be honest I thought that conan 2.0 would be out in a few month at least. I will do a PR when I'll have the time on my own (not until few weeks). I think doing it is mandatory on the long run to have a proper experience of conan.