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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not modify slashes on env variables (EnvValues) #3836

Merged
merged 3 commits into from Oct 26, 2018

Conversation

Projects
None yet
5 participants
@jgsogo
Copy link
Member

commented Oct 24, 2018

Changelog: Fix: environment variables are passed verbatim to generators.

  • closes #3827
  • feedback about windows subsystems

Green CI, but I'm sure we are going to break something... 馃槹

@ghost ghost assigned jgsogo Oct 24, 2018

@ghost ghost added the stage: review label Oct 24, 2018

@jgsogo jgsogo added this to the 1.9 milestone Oct 24, 2018

@jgsogo jgsogo requested review from lasote and danimtb Oct 24, 2018

@memsharded
Copy link
Contributor

left a comment

I approve, but would like the review of @lasote too

@memsharded memsharded assigned lasote and unassigned jgsogo and memsharded Oct 24, 2018

@danimtb

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

Might this be breaking for windows subsystems when the virtualenv generator is used?

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

It's worth a test, @danimtb 馃檮

@danimtb

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

This might require some expertise on the topic. @SSE4 WDYT? We are are a bit worried as this PR might brake env variables in Windows subsystems

@SSE4

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

I didn't use VirtualEnv generator(s) with Windows subsystems, so this need to be tested (manually or automatically)

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

This changes the behavior or EnvValues class, it is not restricted to virtualenv generator. That's the reason it scares us so much.

@danimtb

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

I did the test of virtualenv manually inside a windows subsystem and calling activate.bat with \ in env vars worked well. However, using those env vars to call a subsystem utility might be the breaking thing. That's why this does not only applies to virtualenv generator but to env vars used in builds running inside a windows subsystem

@lasote

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

So, let's merge it.

@lasote lasote merged commit 0a7a3a3 into conan-io:develop Oct 26, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Oct 26, 2018

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

@lasote! @danimtb says:

However, using those env vars to call a subsystem utility might be the breaking thing. That's why this does not only applies to virtualenv generator but to env vars used in builds running inside a windows subsystem

We need to check not just the virtualenv generator, but the subsystem utility, as it is marked in the checklist of the PR.

Revert and open again.

@lasote

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

If I'm merging this is because obviously, replacing a char in a variable that it is not a path, is a bug. I'm totally sure we are breaking something, with the subsystems and much more. When the problems arise we will need to provide a better interface or simply will say to the user "do a replace in your variable manually". So anyway this should be merged. Keep the subsystems investigation to investigate the possible fix ASAP if needed. WDYT @memsharded @danimtb

@danimtb

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

I have just build ffmpeg with this change and seems unaffected but it might be not using env vars for include paths or other things, so I agree with @lasote. Modifying env vars implicitly is something we should not be doing

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

There are 2 opposed bugs here in this case: one bug is changing the slash, and the other one is not changing it when it is a path. I am not 100% sure, but I'd tend to favor the case we can reproduce in tests and we are sure it is a real bug (reported by user), instead of the case we don't fully know and can't reproduce yet, and when we reproduce it, the solution should be probably elsewhere rather in this automatic replace of slashes. Lets keep this merged and will come with a proper solution for the paths when possible/necessary.

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

Ok, agree with it. But this PR does not pay Goikos 馃崝

@jgsogo jgsogo deleted the jgsogo:issue/3827 branch Oct 26, 2018

ericLemanissier added a commit to ericLemanissier/conan-mingw-installer that referenced this pull request Nov 26, 2018

change slashes in CXX and CC environment variables
before conan-io/conan#3836 this change was performed by conan
since 1.9.0, conan does not change backward slashes into forward slashes. This leads to problems when using mingw32-make and sh.exe is in path, because sh.exe eats the backwards slashes

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Do not modify slashes on env variables (EnvValues) (conan-io#3836)
* do not replace backward slashes

* do not modify slashes for env variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.