Add Windows support to file locking #1157
Conversation
|
Thanks @chrisjbremner. I hope it was clear from #1155 that the blocking issue from the Pex side is having CI set up. I do not want to continue to give a false impression that Pex supports Windows. It does not. Even with this fix it does not. Only with this fix + a CI setup for Windows that's green can Pex support Windows in good faith now and going forward. What's your plan or suggestion to get the CI side of this squared away? |
| fcntl.lockf(lock_fd, fcntl.LOCK_EX) # A blocking write lock. | ||
| if WINDOWS: | ||
| while True: | ||
| # Force the non-blocking lock to be blocking. LK_LOCK is msvcrt's implementation of |
There was a problem hiding this comment.
Nit: presumably then using LK_LOCK is a bit better here. You still need to loop, but presumably its slightly more efficient to let Windows do the 1st 10 tries before sleeping here and looping than to do the 1st 10 tries here manually.
There was a problem hiding this comment.
Makes sense, it also raises a clearer error (OSError: [Errno 36] Resource deadlock avoided) when the locking operation times out after 10 tries, so we can use that directly rather than an OSError catchall. Fixed in edc8435
There was a problem hiding this comment.
Excellent - that's much better.
I am willing to try and help set up some CI for this. I haven't worked much with Travis before, but can give it a shot. What's the process for getting started (do I need access to somewhere else to add a pipeline)? |
That's kind of the point - no clue here - I've never set up a Windows CI for an OSS project and haven't used Windows in ~15 years. It does look like Travis may support Windows now: https://docs.travis-ci.com/user/reference/windows/. If you want to try that you'd just make the appropriate edits to the .travis.yml in this commit. But Travis is not a requirement. I've seen https://www.appveyor.com/ used over in the Pip project and I'm sure there are other options too. If you're willing to dive in I can provide any support needed to click buttons to enable services for the pantsbuild org / the pex repo. |
|
Sounds good, I'll survey other OSS projects to get an idea of what they do, then let you know. Is CI a blocker for merging of this PR, or just to add the |
It's a blocker for this PR. All other PRs must be green and this one is only fake green. We really need to know it works independently of you or I. Thanks for taking a crack at all this. |
|
Ok, I've created #1158 for the Windows CI effort |
|
I've spent quite a while trying to figure out why some tests are failing on Windows and it seems to be isolated to Tox (wheels get downloaded, but not installed for a currently unknown reason), since I am able to build a Pex successfully outside of Tox. While I will continue trying to get Windows CI to work, I don't necessarily think it should be a requirement to merge this PR. Given the presence of |
|
Since you are our current Windows expert, you should feel free to not use tox for the windows CI. Whatever works and can also be easily run by a Windows maintainer on their own machine is great. That could simply be a PowerShell script or batch file that wraps up the testing and can be used both locally and in CI. As to needing CI green to land any Windows fixes, I really don't want to budge on that. We need to make forward progress, and the only way to do that is with a green base. Forward progress for you is modern Pex working on Windows - I understand - but that is not forward progress for the project - its just the status quo - Windows is still unsupported and will break at any minute.
No. See the limp along comment below, but I've been the primary maintainer for 2+ years and - at least to me - no open source project should ever support something it doesn't test (you can be assured I was not around to accept the compatibility change you referenced).
For that - you can just use older versions. If you need newer features of Pex then we really need you, or some one, to own getting CI working; otherwise, we just re-engage limp along mode where Pex happens to work on Windows but is not at all supported. |
|
Landed here after trying things out on WIndows (didn't really check it wasn't supported). This PR was helpful and I was able to hack around and factor it into @jsirois, I can see that at some point progress was made towards Windows compatibility (as per this comment #111 (comment)). Is the CI issue mentioned in the past "easier" now that GitHub actions are used? Was wondering since some of the tweaks I've made could potentially be worked into proper patches that don't cause regressions for already supporting platforms, but would be good to have CI test coverage - even if pex isn't fully functional on Windows yet |
|
@jcar87 the issue has never really been getting Windows boxes for CI, its been my insistence that Windows support is added with green CI to back it up. No one has been willing to put in all that work. I made significant progress (https://github.com/jsirois/pex/tree/Windows/wip) but when I was last diverted to other priorities, there were still ~80 integration tests failing (I believe I got all unit tests passing). My stance on only going forward with Windows support under a fully green CI PR remains and I will circle back to that branch, but that's cold comfort with no schedule. If you want to put in the work to meet the fully green CI goal, that works too of course. It's alot of work though! |
|
@jcar87 I assume you're aware Pex works fine under WSL, but in case not. In fact, I bought a Windows laptop a year or so ago just to be able to finish this project. I've been torturing myself with Windows as my OS as a result and living mainly in WSL. The self-flagellation has not been enough to get this done yet, but it will! I look forward to wiping Windows and installing Linux again. I just want Pex support done right and completely so that it is supportable going forward by a limited set of maintainers, none of which use Windows. |
While this project does not officially support Windows, it was compatible with
1.x.xversions of this package, and there's minimal changes necessary to restore that functionality in future2.x.xversions as well. The only noticeable incompatibility at the moment is around the use offcntlfor file locking, which is not available in Windows (msvcrtis the alternative), which was discussed in #1155. This PR adds logic to choose the correct package for file locking based on what platform is being used.I was able to successfully build and test a
.pexfile on Windows 10 after making this change. As noted here, a user will need to build the.pexas an Administrator, Developer Mode, or enable theSeCreateSymbolicLinkPrivilegeprivilege in order for symlinking to occur correctly.