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

Clean repeated entries in PATH after vcvars call #3598

Merged
merged 4 commits into from Sep 27, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 12 additions & 2 deletions conans/client/tools/win.py
Expand Up @@ -332,7 +332,18 @@ def vcvars_dict(settings, arch=None, compiler_version=None, force=False, filter_
# The new value ends with separator and the old value, is a list, get only the
# new elements
if old_value and value.endswith(os.pathsep + old_value):
new_env[name_var] = value[:-(len(old_value) + 1)].split(os.pathsep)
tmp = value[:-(len(old_value) + 1)].split(os.pathsep)
if name_var.lower() == "path":
# vcvars doesn't look if previous paths are already set so clean the repeated to avoid
# max path (or any other list env var) len issues:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be just one line:

new_env[name_var] = {v.lower(): v for v in tmp}.values()

new_env[name_var] = []
uniques = []
for i in tmp:
if i and i.lower() not in uniques:
uniques.append(i.lower())
new_env[name_var].append(i)
else:
new_env[name_var] = tmp
elif value != old_value:
# Only if the vcvars changed something, we return the variable,
# otherwise is not vcvars related
Expand All @@ -355,7 +366,6 @@ def relevant_path(path):

return new_env


@contextmanager
def vcvars(*args, **kwargs):
if platform.system() == "Windows":
Expand Down
32 changes: 32 additions & 0 deletions conans/test/util/tools_test.py
Expand Up @@ -710,6 +710,38 @@ def vcvars_echo_test(self):
self.assertIn("Conan:vcvars already set", str(output))
self.assertIn("VS140COMNTOOLS=", str(output))

@unittest.skipUnless(platform.system() == "Windows", "Requires Windows")
def vcvars_env_not_duplicated_path_test(self):
"""vcvars is not looking at the current values of the env vars, with PATH it is a problem because you
can already have set some of the vars and accumulate unnecessary entries."""
settings = Settings.loads(default_settings_yml)
settings.os = "Windows"
settings.compiler = "Visual Studio"
settings.compiler.version = "15"
settings.arch = "x86"
settings.arch_build = "x86_64"

# Set the env with a PATH containing the vcvars paths
tmp = tools.vcvars_dict(settings, only_diff=False)
tmp = {key.lower(): value for key, value in tmp.items()}
with tools.environment_append({"path": ";".join(tmp["path"])}):
previous_path = os.environ["PATH"].split(";")
# Duplicate the path, inside the tools.vcvars shouldn't have repeated entries in PATH
with tools.vcvars(settings):
path = os.environ["PATH"].split(";")
new_values = []
repeated_keys = []
for i in path:
if i not in new_values:
new_values.append(i)
else:
repeated_keys.append(i)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is working as expected. We are appending a path (lowercase) variable to os.environ and then we are working with the PATH (uppercase) one. I think that I need here, at least, a self.assertTrue(len(repeated_keys) != 0) check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.environ is not case sensitive.

for repeated in repeated_keys:
if previous_path.count(repeated) < 2:
# If the entry was already repeated before calling "tools.vcvars" we keep it
raise AssertionError("The key '%s' was not repeated previously but now it is" % repeated)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use self.fail("....") instead.


@unittest.skipUnless(platform.system() == "Windows", "Requires Windows")
def vcvars_amd64_32_cross_building_support_test(self):
# amd64_x86 crossbuilder
Expand Down