vex
vex copied to clipboard
sashahart
Windows compatibility when run inside a virtualenv
There should probably be a function for getting the bin dir of a virtual env, but this is not included in this commit for brevity.
Edit: Looks like this is essentially equivalent to one of the changes in #38, which was closed, however this issue still exists so it seems to have been closed erroneously.
Please be more precise with words like "should" and "error." Please do not just make PRs saying that they "should" be included without explaining the technical details of what you're doing and why.
On #38 I wrote "This was addressed in October of last year." That is why I closed the issue. My finger didn't slip.
The October 2014 commit, https://github.com/sashahart/vex/commit/685522d6a64117da709bede12df63681bab2c243, addresses a separate issue than #38 and this PR. This is why I said that #38 was closed erroneously. I thought it would be pretty easy to check and see that this PR fixes an issue that has not yet been fixed, but just to clarify, you can look at the block of code changed by this PR (and #38) in the current version of vex:
https://github.com/sashahart/vex/blob/b7680c40897b8cbe6aae55ec9812b4fb11738192/vex/run.py#L42-L59
Vex tries to remove bin paths of any existing venvs and failing that it raises an error. This is a problem on windows because if you are in an existing venv, there won't be a bin dir in the path, there will a Scripts dir instead. The effect of this is that if vex is "run inside a virtualenv", it will always fail on windows.
I suggested that the behavior of getting the correct bin/Scripts dir of a venv "should" be put in a function because this is done in a couple different places throughout the code, so creating a function (or a BIN_DIR constant) would simplify the code and potentially could help to avoid future bugs.
I think that #38 has some advantages over my fix since it deals with paths that are prefixed with something different but equivalent to the VIRTUAL_ENV environment variable. I no longer use either vex or windows, so I haven't played around to see how often this actually comes up, but it seems like a good idea. However, #38 does have some problems too. For one, map returns an iterator rather than a list in Python 3, so .remove() won't work on it. Also, I think that normalizing every path in the PATH could cause problems for other tools that expect the PATH to look a certain way. If you want, I can update my PR to improve upon the ideas of #38, but I'd need to boot up windows to test it...
I decided to add a commit to (hopefully) do what #38 was trying to do wrt normalization correctly. I have not tested it yet though, so don't merge yet.
I removed the error altogether because I don't think it is particularly useful and it doesn't arise naturally from the way I did things.
Ok, I'm convinced that my new commit works now. I'm still not entirely convinced it is necessary, but IDK, maybe some venv tools setup the environment variables in a weird way that makes it necessary. I wonder if @cwacek ran into an actual issue with it that prompted him to do the normalization.
Yes I did. I wasn’t doing it for fun ;)
However, that was a long time ago so I don’t remember what might have caused it.
The way that virtualenv activation mangles PATH actually isn't reversible in the general case, and it doesn't export _OLD_VIRTUAL_PATH for us to sidestep the issue. Because the creators of virtualenv didn't actually intend any of this to be interoperable with other tools. That can't be helped.
But it also doesn't matter that much, because the case of trying somehow to run one virtualenv inside another is bizarre and has no real application. It's impossible to tell exactly what the user might have meant from outside a shell that sourced activate. Really vex should just barf and bail if VIRTUAL_ENV is already set.
At the time I wrote this check, I was willing to deal with a simple user error that was likely to occur: forgetting you're in a virtualenv and then trying to activate another one. I was willing to handle this in a way reasonably similar to what you'd get if you ran activate again, given the kind of environment that migth be produced by using virtualenv.
This doesn't mean I was committing to rescuing every conceivable nonsensical environment by guessing what the user really meant to happen, when it is impossible to know. If the environment is too wacky then really the user should know that and bug reports should make their way upstream. You might think vex should be changed any time you meet any inconvenience (instead of figuring out where the inconvenience was actually caused) but it's actually making everything worse if vex is guessing what the user wants and suppressing other tools' bugs.
If VIRTUAL_ENV is set, that should have been done by an activate script, or something emulating an activate script closely enough that I don't have to worry about it. Which means that it prepended its bin path to PATH. The environment where this didn't happen is a broken environment. I don't really care whether it was broken by the user manually setting VIRTUAL_ENV for no reason, a buggy emulation of virtualenv's activate, or what. I don't believe in silencing errors. BadConfig is being raised with an educational message entirely on purpose. If the simple thing doesn't work, the user should know that their environment is a non-Euclidean horror that should be sorted out properly instead of expecting vex to do the impossible by somehow reversing the working of activate inside an environment where activate was used.
This kind of nonsense is exactly why I wanted vex to use a subprocess instead of munging the environment it lives in.
I think the idea of removing all occurrences of a bin path from PATH is wrong. If you read activate, which I did in order to write vex, you see that each time activate is invoked, it prepends to PATH just once. If you invoke activate a second time, it runs deactivate first thing, so it should only ever have its bin path on PATH once. (Though it seems plausible that something might prepend to PATH after activate was sourced.) If your PATH has two or more instances of the same bin path, what on earth achieved that ridiculous state and why are we trying to reverse its actions? At that point I have to say the environment is so irrational that trying to "fix" it will only make things worse. Maybe something intended that bin path to be present, and activate redundantly added it again at the beginning - not my problem. The best possible case is that it means vex is suppressing the bugs of some totally broken alternate implementation of virtualenv. I don't see the point.
normpath's doc says: This string manipulation may change the meaning of a path that contains symbolic links. It's both possible and probable that PATH will have paths to symbolic links on some systems and I don't see any reason vex should be overriding the intent of the user, the distro or whatever software has edited PATH in a way that put a symlink in there instead of the normalized path.
I think you're missing the main point of this pull request. As I've said earlier, the main thing this change is meant to fix is that on windows the bin dir is Scripts, not bin, so you have incompatibility with windows and other platforms in this one particular place in your code. If you just take my first commit, that would probably be fine (except that apparently cwacek ran into some issues like that). If you want to always bail when VIRTUAL_ENV is set, that would be fine too (since it would also remove the incompatibility).
In retrospect, you are correct that removing every matching path from PATH was not a great idea on my part. I wasn't really intending on removing multiple paths from PATH, I just did it like that because it was easy and because I recognized that changing every path in PATH to be normalized (like cwacek did) would have been an even worse idea. If you want to deal with weird path issues on windows, I think the best thing to do would be to remove the last bin (or Scripts) path that matches the bin (or Scripts) path of VIRTUAL_ENV when normalized. You can implement that however you see fit. Otherwise you can just take the first commit of this PR which simply deals with that tiny little bin vs Scripts issue which shouldn't be controversial at all.