-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Supposed typo: https://github.com/pypa/virtualenv/issues/565#issuecom… #1010
Conversation
…cts fails without this "fix". Also relevant, spotify/dh-virtualenv#150 and numpy/numpy#7570.
This commit fixed an error for me using the Here's the traceback which no longer occurs:
Hopefully it gets merged. Thank you. |
This patch also fixes this issue: #985 |
+1 for merging this - I need this fix on Centos 7. Good job on finding the root cause! |
The comment in #565 (comment) seems to be saying that this fix will cause issues on some other systems (CentOS 6, AIUI). Any fix that gets merged should work everywhere, not just trade a failure on one system for a failure on another. |
PLEASE MERGE IT |
@aelnaggar Shouting at the developers is neither polite nor likely to be productive. We're aware of the potential benefit, but haven't a clear picture of the potential issues on other systems yet. It's extremely hard to maintain virtualenv precisely because of the various distribution idiosyncracies we have to cater for, and the virtualenv developers (who are all volunteers) have little enough time as it is. If this issue is important enough to you, you're perfectly welcome to maintain this change as a local patch to your copy of virtualenv. Otherwise please be patient and respect the fact that you are getting virtualenv for free, as a result of others' volunteer efforts. |
@pfmoore Should I/we change the pull request, so that the updated pull request can distinguish a Debian system from a RHEL system? That would be a more appropriate fix right? |
If any updates are required to get this PR merged in, please let me know and I will gladly take over dev responsibility for this PR. If what is mentioned in comment #565 is what is needed, please let me know and I can ensure this only is run on affected distros. |
I don't honestly know, myself. I'm not familiar with the differences between the various Linux distros, and how this change would need to cover that. Someone needs to do the due dilligence on how things vary between the various platforms (RHEL/CentOS, Fedora, Debian/Ubuntu, SuSE, Gentoo, Arch, ...?), and confirm that. Then I guess one of the virtualenv maintainers who does work with Linux should review it and take it from there. (The PR is described as a "typo fix" - it may well be that to someone who knows the platforms, it's "obviously right", but I can't say on that). |
From what I can see in the suggested change, it's not really a "typo fix" and moreso branching behavior based on which distro is currently being used. If that's the case, I could add in the logic around Thoughts? |
2b3f876
to
9dfcb43
Compare
Supposed typo: #565 (comment) On CentOS 7 building certain projects fails without this "fix". Also relevant, spotify/dh-virtualenv#150 and numpy/numpy#7570.