CI: Add a Cygwin run to GitHub Actions #5878
Conversation
Requested for the _imagingtk fix.
I think this is how specifying the shell works. The documentation isn't terribly clear.
For some reason wheel wasn't installed properly.
setup.py bdist_wheel goes to dist/*.whl pip wheel goes to *.whl
I'm using selftest.py to check whether I've installed everything. Pytest actually finds and runs the tests. For some reason that wasn't running earlier.
radarhere
changed the title
radarhere
changed the title
There was a problem hiding this comment.
Thanks for the PR!
Please could you also add this to the "Continuous Integration Targets" table in docs/installation.rst?
https://pillow.readthedocs.io/en/latest/installation.html#continuous-integration-targets
And not necessary for this PR, but do you think it's possible to include test coverage and upload to Codecov?
Upload coverage information, add Cygwin to the list of systems with CI, space out entries.
.github/workflows/test-cygwin.yml
Outdated
|
|
||
| - name: Upload coverage | ||
| run: | | ||
| C:\tools\cygwin\bin\bash -c 'bash <(curl -s https://codecov.io/bash) -F' |
There was a problem hiding this comment.
The bash uploader is deprecated and will stop working on Feb 1, 2022:
https://docs.codecov.com/docs/about-the-codecov-bash-uploader
Maybe use https://docs.codecov.com/docs/codecov-uploader or https://github.com/codecov/codecov-action?
There was a problem hiding this comment.
Is the current version better? The Windows and Linux codecov uploaders will also need to be updated, but that's a separate PR, I think.
The actual test step wasn't running, so try to run that as a shell script rather than an executable. Also get more of the dependencies installed.
The old bash downloader will be removed soon.
|
Is there some way to tell GitHub Actions "I'm trying to set up a Workflow; only run the Workflows that have actually changed"? |
|
I would create a separate branch to this one (just to keep things neater here) and remove any workflow files you don't want to run. Then when you push, you should only the workflow you want to run at https://github.com/DWesl/Pillow/actions |
Cygwin is trying to use the highest-available Python version. One of the Python packages has scripts in /usr/bin that should be in the python39- subpackage.
|
I can do that, though it's probably fine now, and will report the two or three ImageTk errors from #5795 just fine. If not, off I go to a new branch to tinker with things. |
docs/installation.rst
Outdated
| @@ -460,6 +460,8 @@ These platforms are built and tested for every change. | |||
| +----------------------------------+----------------------------+---------------------+ | |||
| | CentOS Stream 8 | 3.9 | x86-64 | | |||
| +----------------------------------+----------------------------+---------------------+ | |||
| | Cygwin | 3.8 | x86-64 | | |||
There was a problem hiding this comment.
Could move this under Windows 2019 below, like there is a separate line for MinGW?
They segfault in one of the NumPy tests, but they run.
Segfaults are annoying to debug.
From a suggestion on the PR, make it clearer that Cygwin runs on Windows. Also record the Cygwin versions in the check against Python versions.
|
32-bit Cygwin Python 3.8 seems to have started failing between 3453b0e and 143e57b. Specifying precisely which version of python to use shouldn't cause problems, and it definitely shouldn't cause a GCC crash. There are Cygwin updates around that time, but DWesl#5 assumes this is some kind of new Oh, and now the failure has migrated to Python 3.9, and 3.8 works fine. Is this a new manifestation of the old "sometimes Windows decides it hates how Cygwin works" problems, or is this another bug that depends on the architecture of the CI runner? This is such a weirdly specific failure, and reproduced somewhat consistently whenever the workflow managed to get that far, but there's no apparent cause in the code, nor in the part of the environment I can control. The change in the length of the command line, or of the template generating the file that the CI runner actually runs, is about the only thing similar between the start and end of the run of failures; however, I saw similar failures on the test branch where I ironed out the syntax for alternatives. None of this should be causing any problems, but any problems I can think of should be producing at least three failures, not merely one. I have no idea how to go about debugging this. The best idea I have is that roughly a third of the 32-bit Windows GitHub Actions runners are older machines than Cygwin's GCC maintainer assumed when compiling 11.2, but I don't think there's been that many updates to 32-bit processors recently, and I don't know if there's a way to convince cygwin-install-action to install old versions of GCC to test the theory. |
|
Would it make sense to only test 64-bit Cygwin? Or at least to get this PR merged, and 32-bit could be investigated in a new PR? |
|
Given that the Cygwin website recommends not using 32-bit Cygwin if possible, and that the Cygwin 3.3.0 announcement said 3.4.0 will not include a 32-bit component, it would probably make sense to omit regular testing for 32-bit Cygwin. Should I update the matrix? |
|
That's quite a warning!
Yeah, let's just ditch 32 bit. The community is gradually dropping support for 32 bit anyway, for example see NumPy. |
Cygwin recommends using 64-bit if at all possible, and will discontinue support entirely within the next year or so. This also reduces CI load, which I suppose is polite to those who provide it at no cost.
.ci/install.sh
Outdated
| cmake meson imagemagick libharfbuzz-dev libfribidi-dev | ||
| fi | ||
|
|
||
| if [[ $(uname -mo) != i*86" Cygwin" ]]; then |
There was a problem hiding this comment.
Is this a 32-bit check? If so, let's revert it.
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
Install seems to be working now. Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>
Removed steps that are currently unnecessary. Hopefully they stay that way.
Added Cygwin badge
Let's see if this works. Co-authored-by: Ondrej Baranovič <nulano@nulano.eu>
|
@DWesl Thank you for your work on this, and everyone else for the reviews! |
Requested for the _imagingtk fix.
Changes proposed in this pull request: