Add Cygwin build on AppVeyor #8463
Conversation
|
Also I fully recognize the commit history is a mess, and I can squash/clean it up if need be. |
|
I'd be fine with an unstable buildbot either first or in addition to this. Personally, I would find the appveyor build more immediately useful, because I feel that not having it has made people more wary of reviewing PRs addressing issues that affect Cygwin. I'll also add that it's not quite right to call it a "new platform", but rather a platform that has been varyingly supported off-and-on and never quite officially "supported" or "unsupported", but mostly "supported" in principle but lacking a buildbot and a champion. |
|
I don't want to see a new CI on PR until the platform is fully supported:
If you want a CI to enhance the platform support, maybe start with a custom buildbot run manually? |
|
I'm fine with going ahead and adding a buildbot; my only concern with adding it to AppVeyor is slowing it down. |
|
What does "all tests should pass", actually mean? Some necessarily need to be skipped due to platform limitations. This is true on many other supported platforms. |
Okay, but then will you (by "you" I mean not you personally, but in general) agree to merge PRs that fix/skip (with valid reasoning) tests on the platform even if it's not supported? Because in the past you've told me you won't merge things for platforms that you consider "not supported". I could get a custom build on a buildbot fully passing, but then I'm left with possibly dozens of fixes that individually need to be reviewed, and that's a very slow way to go about things. I feel like you're putting me in an impossible position. Having it on AppVeyor I believe will help fixes get reviewed and accepted faster:
That's a valid concern, though I'll point out that even now it's not the slowest PR check generally, and can even be faster than the Win32 build. That's helped of course by the fact that I'm skipping about a fifth of the tests, but that only helps so much. |
|
"What does "all tests should pass", actually mean?" Running "python -m test" must complete with SUCCESS. "Some necessarily need to be skipped due to platform limitations. This is true on many other supported platforms." I'm fine with that. No platform run all tests. |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Your Modules/getpath.c change (adding "add_exe_suffix(wchar_t *progpath)") must be done in a separated change.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Ok, that's reasonable. It might just mean skipping some tests, but I agree that those skips should be made explicit in the test modules themselves, not by skipping whole modules with
That's #4348. This PR isn't even worth considering merging without having some form of #4348 as well as the other two PRs I mentioned in the description as prerequisites. I wish GH had a way to make prerequisites clearer. |
|
It occurs to me that #8445 is not actually strictly necessary for this, just #8446 (or a better fix to the same problem). |
…ate build instructions for Cygwin yet
… in building on Cygwin; temporarily disabling Win32 for now while we get Cygwin up and running
…convert the path to a Cygwin path
… for now while I debug
… sourced when needed in each build stage
…on appveyor: - test_argparse - test_ioctl - test_os - test_py_compile
…eption' even if the last command existed successfully this should work around that...
|
I updated this branch to not include the changes from other PRs, in order to make this easier to review. Now the AppVeyor build for Cygwin will fail, but I feel I've at least demonstrated with previous builds of this branch that it can work if the necessary fixes are incorporated as well. |
|
I am sorry, but with AppVeyor already being our slowest and most-blocking CI, I don't want to slow it down any further and am rejecting this PR. I would be more interested in adding a Cygwin build to the Azure build. |
6 participants
Note: Depends on #4348, #8445, #8712 as prerequisites, without which Python does not even build on Cygwin.
This adds a configuration to our AppVeyor builds to build/test on 64-bit Cygwin in addition to the native Windows build (32-bit Cygwin is increasingly less supported by the Cygwin community, and I'm not interested in supporting it for Python).
This PR should not be merged until at least #4348, #8445, and #8712 (or some equivalent alternatives) are merged, as they are the bare minimum necessary to get Python building on Cygwin. (Some extension modules fail to build as well--this is a bug with a known fix, but I have not included it for now since it only means some extensions can't be built/tested).
I started this work last year, but took an approach that was self-defeating: There are still numerous test failures on Cygwin, and I had hoped to fix all of them before requesting to add Cygwin to our CI. However, it's rather impractical time-wise to get all those issues fixed, especially considering the catch-22 of not having CI for Cygwin in the first place with which to test the fixes.
Therefore my approach this time is to brute-force disable all the modules that contain failures (~45) so that we can at least start with a nominally "passing" build on Cygwin. If we can get that up and running, it will decrease the number of new failures. Meanwhile, I volunteer to hack away at reducing the number of test modules that need to be disabled.
In fact, I already have fixes for roughly half of them. I just need to make issues/PRs for many of those fixes, and it will be better to have the CI implemented in the first place to help get those fixes merged.
I would consider this a prerequisite for getting a buildbot working for Cygwin, which I also have preliminary work on; again it's mostly a matter of disabling some known failures.