Fixes to get pex to work on windows. #198
Conversation
mikekap
force-pushed
the
windows_fixes
branch
from
4744c4d
to
43a106a
Compare
| if e.errno == errno.EEXIST: | ||
| # File already exists. If overwrite=True, write otherwise skip. | ||
| if overwrite: | ||
| if hasattr(os, 'link'): |
There was a problem hiding this comment.
how about a quick comment here to indicate the reason for doing this (i.e. windows) so that a casual observer doesn't get confused by the otherwise odd hasattr check here.
|
lgtm overall mod a few comments. also, test coverage simulating windows environmental conditions (+ a quick unit test for |
mikekap
force-pushed
the
windows_fixes
branch
from
43a106a
to
e88b5f1
Compare
|
Might it be better to set up windows CI (e.g. Appveyor) to make sure this works? Unfortunately I don't think there are currently tests for _site_libs other than integration tests. |
mikekap
force-pushed
the
windows_fixes
branch
2 times, most recently
from
af6192e
to
475cbc4
Compare
mikekap
force-pushed
the
windows_fixes
branch
from
475cbc4
to
9571a20
Compare
|
generally speaking, pex doesn't claim or strongly desire windows support - my understanding is that it's essentially "best effort" via submission of PRs from interested contributors. with this said, I'm not sure it makes sense for us to add Appveyor at the moment without a stronger sense of ongoing Windows support ownership - hence the suggestion to simply mock out some of the windows conditions in test. |
The fixes are: - os.link doesn't exist on windows. Always copy instead. - NamedTemporaryFile doesn't work correctly on windows (see https://bugs.python.org/issue14243) - sys.prefix is part of site.getsitepackages() on windows. Don't remove it.
mikekap
force-pushed
the
windows_fixes
branch
2 times, most recently
from
706536c
to
e787836
Compare
mikekap
force-pushed
the
windows_fixes
branch
from
e787836
to
8028e99
Compare
|
Added a couple of tests for site_libs. Let me know if that's what you had in mind. I also managed to finally get all the tests to pass on windows (you can see the commits @ https://github.com/pantsbuild/pex/compare/master...mikekap:windows?expand=1 & the build @ https://ci.appveyor.com/project/mikekap/pex/build/1.0.33). I'll open another PR once this one goes in since the changes there are a bit larger. |
|
ping :) |
|
The Buck project would love to see these upstreamed so we can stop having a variety of changes to PEX in our codebase :) |
Fixes to get pex to work on windows.
|
lgtm, thanks @mikekap! will you guys need a pypi release to consume this or are you vendoring? |
The fixes are: