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
Feature/fix vcvars filter known #3941
Feature/fix vcvars filter known #3941
Conversation
conans/client/tools/win.py
Outdated
path = new_env.get("PATH", "").split(";") | ||
path = [entry for entry in path if relevant_path(entry)] | ||
new_env["PATH"] = ";".join(path) | ||
path_key = ([name for name in new_env.keys() if "path" == name.lower()] or ["PATH"])[0] |
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 new_env["PATH"]
is always a list, not a string (first bug)
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 name of the variable PATH
could be in a different casing Path
or path
or PATH
so check the correct one. (second bug)
conans/client/tools/win.py
Outdated
path = new_env.get("PATH", "").split(";") | ||
path = [entry for entry in path if relevant_path(entry)] | ||
new_env["PATH"] = ";".join(path) | ||
path_key = ([name for name in new_env.keys() if "path" == name.lower()] or ["PATH"])[0] |
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.
This will add "PATH"
variable (empty) if it was not already there. Remove the or ...
and check for path_key
in order to add it to new_env
or not in the following lines.
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.
Yes, but that was the previous behavior too. If we think that it shouldn't be added, that might be a breaking feature and a new issue for Conan 2.0 should be filed.
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.
It is a bug and not a big deal I think. I'm removing the assignment when path is empty.
conans/client/tools/win.py
Outdated
path = new_env.get("PATH", "").split(";") | ||
path = [entry for entry in path if relevant_path(entry)] | ||
new_env["PATH"] = ";".join(path) | ||
path_key = ([name for name in new_env.keys() if "path" == name.lower()] or ["PATH"])[0] |
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.
Yes, but that was the previous behavior too. If we think that it shouldn't be added, that might be a breaking feature and a new issue for Conan 2.0 should be filed.
Agree with the reviews. Please @lasote take a look |
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 breaking, but not much (I would say it was a bug, too) . I'm ok with the changes.
* before win * added test * Avoid empty path
Changelog: Bugfix: Fixed bug in
vcvars_dict
tool when usingfilter_known_paths
argument.