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

Conversation

@lasote
Copy link
Contributor

@lasote lasote commented Sep 21, 2018

  • Refer to the issue that supports this Pull Request. Closes #3522

To clarify, there was no bug in Conan. vcvars prepends new variables to the PATH, but without looking if the entries in the path are or not already set with the same value there. With some recipes manipulating the environment, sometimes the max chars for PATHS is reached. So with this PR, the vcvars_dict will remove repeated entries in the PATH.

Changelog: Feature: Clean repeated entries in the PATH when vcvars is run, mitigating the max size of the env var issues.

@ghost ghost assigned lasote Sep 21, 2018
@ghost ghost added the stage: review label Sep 21, 2018
@lasote lasote added this to the 1.8 milestone Sep 21, 2018
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
Member

@jgsogo jgsogo Sep 22, 2018

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

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
Member

@jgsogo jgsogo Sep 22, 2018

It can be just one line:

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

new_values.append(i)
else:
repeated_keys.append(i)

Copy link
Member

@jgsogo jgsogo Sep 22, 2018

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

@lasote lasote Sep 24, 2018

os.environ is not case sensitive.

@lasote
Copy link
Contributor Author

@lasote lasote commented Sep 24, 2018

Thanks for the review, I simplified a lot how it is doing the deduplication.

jgsogo
jgsogo approved these changes Sep 24, 2018
@lasote lasote merged commit eb6226d into conan-io:develop Sep 27, 2018
2 checks passed
@ghost ghost removed the stage: review label Sep 27, 2018
grisumbras pushed a commit to grisumbras/conan that referenced this issue Dec 27, 2018
* Clean repeated

* only the path

* fixed test

* Simplified path dedup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants