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
Remove duplicate entries while modifying PATH-like environment variables #7891
Conversation
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.
Could you please add a test that will cover this case? Ask for guidance if it is not evident how/where.
Thanks for the contribution!
conans/client/tools/env.py
Outdated
@@ -65,6 +69,7 @@ def _environment_add(env_vars, post=False): | |||
apply_vars[name] = old + os.pathsep + apply_vars[name] | |||
else: | |||
apply_vars[name] += os.pathsep + old | |||
apply_vars[name] = os.pathsep.join(_unique(apply_vars[name].split(os.pathsep))) |
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 seems that it can be done in place with a more common python idiom like:
items = apply_vars[name].split(os.pathsep)
apply_vars[name] = os.pathsep.join(OrderedDict.fromkeys(items))
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 seems that it can be done in place with a more common python idiom like:
items = apply_vars[name].split(os.pathsep) apply_vars[name] = os.pathsep.join(OrderedDict.fromkeys(items))
Yes, indeed. I was under the impression that OrderedDict
was the Python3-only thing but actually it's included in Python 2.7 as well.
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.
Great. If you could manage to add some test (ask for help if necessary), this can be merged for next Conan 1.31.
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.
Could you please add a test that will cover this case? Ask for guidance if it is not evident how/where.
Yes, please. I'm not familiar with Conan testing framework. Is
def vcvars_env_not_duplicated_path_test(self): |
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 seems that the VirtualEnvGeneratorTest
in conans/test/unittests/client/generators/virtualenv_test.py would be a good place, adding another variable with repeated paths.
Locally, you can run it with: nosetests conans/test/unittests/client/generators/virtualenv_test.py
. You might need to pip install -r requirements_dev.txt
and pip install -r requirements_server.txt
if missing deps. Please let me know if this helps, otherwise I can contribute the test myself. Thanks again for the contribution!
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.
Please let me know if this helps, otherwise I can contribute the test myself.
Looks like sorting it out needs some time but right now I'm a little bit busy. If you can write tests yourself with little efforts please do!
c9700ad
to
0605380
Compare
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.
Thanks a lot for the contribution, @db4 🎉
Changelog: Fix: Remove duplicate entries while modifying PATH-like environment variables internally. Especially important for Windows where system PATH size is limited by 8192 charachers (when using cmd.exe).
Docs: omit
Partially solves #7587, especially when
conan
is launched from MSVC develper command prompt.