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
New Environment model for recipes and profiles #8630
New Environment model for recipes and profiles #8630
Conversation
|
||
def environment_wrap_command(filename, cmd, cwd=None): | ||
assert filename | ||
filenames = [filename] if not isinstance(filename, list) else filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filenames = [filename] if not isinstance(filename, list) else filename | |
try: | |
from collections.abc import Iterable | |
except ImportError: | |
from collections import Iterable | |
filenames = [filename] if not isinstance(filename, Iterable) else filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have conans.utils.misc.make_tuple
that will work here (pay attention, a string is iterable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment below, filename
must be either a string value or a list. Other input types as sets, tuples, dicts, etc are not supported, and it will be documented as this.
if os.path.isfile("{}.bat".format(full_path)): | ||
bats.append("{}.bat".format(f)) | ||
elif os.path.isfile("{}.sh".format(full_path)): | ||
shs.append("{}.sh".format(f)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we raise for else
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is injecting the environment file if it is found, but the file might not always be there, for example if there are no environment variables to define, instead of generating a blank file, the file is not being generated. That is what allows to inject previously unknown build_requires
into a recipe that will create such conanbuildenv.sh
env file, that will be used if present, but it will not generate a blank conanbuildenv.sh
file that will always be there for absolutely all packages, even if they are not using build_requires at all.
save(filename, content) | ||
|
||
def save_ps1(self, filename, generate_deactivate=False, pathsep=os.pathsep): | ||
# FIXME: This is broken and doesnt work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what exactly is broken here? also it doesn't seem to be used anywhere at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole powershell implementation is not working neither tested. I need a PS1 expert to contribute it, maybe you or @solvingj can do something, but that can wait, it is not central to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a TODO: ...
and/or an exception is more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be happy to work on the PS1 implementation when ready.
|
||
@staticmethod | ||
def _list_value(value, separator): | ||
if isinstance(value, list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to accept several different types of inputs, it should be either a value or a list. Passing an iterable, like a set, that is not ordered could produce unexpected results. Lets keep the interface as narrow as possible, there is always time to extend if we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the environment part I think it is very nice. Only two very little comments.
I have doubts about the buildenv part that I would want to understand:
- How is it different from the
[env]
entry we already have in theprofile:build
? - If it composes with the
[env]
entry from theprofile:build
, which one takes precedence?
And to be clear, this profile:host[buildenv] is something nice to have (in fact, it is already possible and used because there is a bug), but it is not the same as the build/runenv declared in recipes at #8534.
""" | ||
clears the variable, equivalent to a unset or set XXX= | ||
""" | ||
self._values[name] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that the environment variable is assigned an empty value or that it doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value exists in the Environment
, but an empty list represents the need to unset it, actually removing it from the system environment with a unset VAR
or set VAR=
in Windows. It is a list, because an Environment.unset
still can compose with a Environment.define
or Environment.append
, and it is more uniform than having a None
value that would need to be handled differently.
if method == "unset": | ||
env.unset(name) | ||
else: | ||
if value.startswith("(path)"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this magic keyword, but I understand it is convenient to inform Conan it is a path... have we considered any other alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a few alternatives, like different operators, but this was the most readable and intuitive and easy to parse at the same time, I think it is difficult to beat this, but suggestions welcome:
# define
MyPath1=(path)/my/path1
# append
MyPath2+=(path)/my/path2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that (path)
is valid for a path name... and if something can happen, it will happen 😄
⇒ pwd
/Users/jgsogo/dev/conan/tmp/path(weird/(path)porqueyolovalgo
[py38]jgsogo@Javierg:~/dev/conan/tmp/path(weird/(path)porqueyolovalgo|
maybe we need to rely on comments?
# define
MyPath1=/my/path1 # path, type:path
# append
MyPath2+=/my/path2 # path, type:path,...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that there might be env-vars that need to contain a # too, so lines cannot contain comments either.
Strictly speaking, the only character that cannot be an env-var name is =
. Even the +=
or =!
could be colliding with values or variable names containing those special characters.
But in practice, POSIX shells only allow alphanumeric identifiers, so it is relatively extended practice.
As the environment variable values can contain any arbitrary value, then there is by definition impossible to have any VAR OPERATION VALUE
definition that implements the different logic operations and completely avoid all possible collisions. We would need a structured format, json, xml, or equivalent, but I doubt we want to go that way.
If we are concerned about (path)
colliding, the best action could be to make the token more specific, something like var+=(conanpath!)/path/to/something
assert env.value("MyVar") == "MyNewValue" | ||
assert env.value("MyVar2") == 'MyValue2 MyNewValue2' | ||
assert env.value("MyVar3") == 'MyNewValue3 MyValue3' | ||
assert env.value("MyVar4") == "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to actually unset the variable instead of assigning an empty value to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The operation done at the system environment will be an unset. This is a unittest checking the internal representation of the Environment
, which reads more like a set of actions to do on the environment rather than actual environment variable values.
Changelog: Feature: New
Environment
model for recipes and profilesDocs: conan-io/docs#2060
Extracted from #8534
Environment
ProfileEnvironment
[buildenv]
(and[runenv]
later) to define env-vars to be applied at build time (buildenv.sh
orbuildenv.bat
file)[buildenv]
composes for multiple profiles