Use a clean, whitelisted environment with Popen (#16118) #16119
Conversation
Do not allow the user's shell environment to leak into the build environment. Instead, create a clean environment with just white-listed environment keys.
|
|
|
I'm not sure we should include the python path in the whitelist. I noticed ICU complained when it didn't find python (warned that it wouldn't be able to build its data from source), but seemed to build anyway. |
conans/util/runners.py
Outdated
| # This whitelist could potentially be slimmed down further. | ||
| def _env_for_Popen(): | ||
| newenv = None | ||
| if platform.system() == 'Windows': |
There was a problem hiding this comment.
I'd say that this sounds super risky, and it will certainly break users that depends on some of these variables for their usage.
In general it is not the responsibility of Conan to do this kind of task, it should be the users that need to spin a clean environment and then run Conan in it.
Conan has some functionality, like it can undefine/unset variables if desired in the profiles, so if you are having problems I'd recommend to define a profile that unsets the environment vars you want to filter out and then inject that profile when you want.
There was a problem hiding this comment.
The problem is the variables I'm having problems with are not things I'm adding. It is bash that is creating !ExitCode multiple times in different cases.
This would affect all users.
I thought it should be conan's responsibility though... I thought it was conan's job to control the build environment as best it can...
I don't think recipes should be depending on environmental variables from outside the conan-defined environment. The recipe, lock file, and profiles, should say everything.
If users want something added to their environment, add it to their profile. Isn't that what it is for?
There was a problem hiding this comment.
I am afraid that you are looking for a level of isolation that is outside of Conan scope. You might be able to implement that isolation with some Conan mechanisms, but it is not something that should be in the Conan scope, as Conan is a package manager not a "build environment" or a "virtualenv manager" (as a note, not even Python virtualenvs provide isolation from defined environment, if you have some env-var defined and activate a virtualenv, it doesn't provide a clean/blank environment, it inherits the env-vars already defined in the system)
There was a problem hiding this comment.
I'll continue the discussion on the merits in the issue
There was a problem hiding this comment.
This approach has been dropped in favour of clearing the env at the start of conan's execution.
More discussion in issue.
There was a problem hiding this comment.
I have been discussing this with the team.
The feedback from the team is quite unanimous, this seems something outside of Conan responsibilities. While I understand your intent to have somehow "clean" and reproducible environments, this is way beyond the scope of Conan. This probably should be handled with other approach, like clean VMs in CI or other custom tools that isolate and reproduce the environment, etc.
There are several issues why these changes cannot be accepted and merged into Conan:
- They are very specific to your use case, but many other users will rely on other environment variables not allowed here, so this change would be massively breaking. This is not just a risk, this is 100% sure that will be breaking to many users.
- The configurability of this functionality would be too much. Every different user will have a different subset of variables to allow/disallow, and that makes the UI/UX more complex, as well as the internal code and future maintenance, support, etc.
- This is very partial to one OS, but other OSs would have other variables.
Thanks very much for your contribution anyway, I am sorry that this won't be possible to be merged, but appreciated in any case.
|
Closing this PR for the reasons above. |
Do not allow the user's shell environment to leak into the build environment. Instead, create a clean environment with just white-listed environment keys.
Changelog: Fix: Provide a create a clean environment to Popen() with white-listed environment keys.
Docs: Omit