Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Github Actions parallel builds, python 3.10 CI, pip wheel caching and other minor fixes #2605

Merged
merged 2 commits into from Oct 5, 2021

Conversation

ankith26
Copy link
Contributor

@ankith26 ankith26 commented May 24, 2021

Continuing from #2600 where we configured GH Actions as CI for pygame. illume left some good points towards the end of the PR, this one tries to address some of those.

What this PR does

Split the single manylinux build into 5 matrix builds that run parallel, x86 + CPython, x64 + CPython, x86 + PyPy and x64 + PyPy, and aarch64 + CPython
We should be seeing the manylinux builds take much less time (like one-fourth) the time it takes right now.

This PR also adds CIBuildWheel to help generate wheels, on Windows and Mac. The ManyLinux job still runs without CIBuildWheel because we have our own docker system for handling those anyways. Maybe in the long term it too can be migrated to CIBuildWheel, but is out of scope for this PR.

Along with the addition of CiBuildWheel, this PR also adds 3.10 wheels on Windows and Mac
Going forward, I guess we should keep appveyor only for py2 builds (because that does not happen on GH actions)

This also does two more minor fixes, it fixes an intermittent random error that happens on SDL1-sdist build, and also adds a library path in the mac build script that was missing (this does not fix an existing bug, rather it is done to accommodate for any future package manager that can put lib files in those paths)

What this PR does not do (fix MacOS wheels)

A note on future improvements that need to be done on MacOS wheels.

This PR does not make any changes on the fact that we are still using homebrew to install deps. Homebrew installs binaries that work only on a specific version, the version of the host CI machine (in this case 10.15). So our Mac wheels are still very much not ready for a release. I tried experimenting a bit on this PR, and I did not get too far. I primarily tried homebrew and macports, and failed. We need a way to handle dependencies, that can work on 10.9+, or atleast, 10.13+, since I assume most mac users use 10.13, 10.14, 10.15 and 11.x (Intel). This we have to achieve by this release.

As a bonus, we would need to try generating arm64 wheels (or even better, universal2 wheels) on Mac. CIBuildWheel already provides much of the underlying code to make this possible, we just need to uncomment a single line to make CiBuildWheel make universal2 wheels on CI. The only problem is, you guessed it, deps. We would need universal2 binaries of the deps to make this possible. If we could get it done by this release, it would be great IMHO, but this can probably wait for the next version too.

Some options to consider on MacOS deps handling

  1. Using vcpkg or some other package manager, that can solve the problems that arise with using homebrew packages
  2. Using SDL downloads from SDL website. We would need a script that pulls and installs these DMGs. This has advantages and disadvantages. Advantages being that these binaries work on any 10.6+ Mac versions, including 11.x series. About universal binaries, as far as I can tell, only the SDL binary is universal2 while SDL_ttf, SDL_image and SDL_mixer are not (untested!). I submitted a few PRs to these SDL repos to add a universal2 build script, but those PR did not get any attention yet. Even if those get merged, we would still have to wait for the next release of those SDL sub-projects, so that can take a while. Also if we take this option, we would probably need another manual way of downloading and setting up other deps, so without a package manager, this option can be too "manual" and take some code grinding.
  3. Uploading local binaries, taken from anyone who has a Mac (Starbuck5 said he can do this)
  4. A mix of 2 and 3, maybe using SDL installs from the official website
  5. Or something else that IDK (maybe some magic, anyone can do that here? 馃槃)

@ankith26

This comment has been minimized.

@ankith26 ankith26 closed this May 24, 2021
@ankith26 ankith26 reopened this May 24, 2021
@ankith26
Copy link
Contributor Author

Okay, pip caching works now. The mac builds and windows builds have been sped up. The manylinux one also sped up thanks to parallelization, but if we could cache docker deps images, it would get even faster.
Unfortunately IDK docker stuff, so anyone who knows how pygame handles docker stuff for manylinux, could you please help me out by telling me the path of the directory where the docker images are cached after they are pulled (so that I can configure GH actions to save those caches, I guess)

Copy link
Contributor

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! It will be great to have quicker manylinux builds. And also great to have less reliance on Appveyor, which has been spitting out intermittent and random errors on my PRs.

@illume
Copy link
Member

illume commented May 29, 2021

Hi ya,

The reason I kept the appveyor builds was because those can still be released. That's the main reason why I'd like to keep them. We are currently blocked on releasing, mainly with the macos builds (We can't use homebrew for these dependencies).

I left the Travis stuff uncommented for now, because there is a chance I might need to switch back to it for a release.

If something fails, it is quite wasteful to trigger all the builds again - instead just mention in the PR that it looks like a non-related issue. Please don't do the close/reopen dance just to get green ticks.

99% of the time with intermittent crashes, those are real bugs. The main windows intermittent issue (prebuilt download failures) seem to have stopped with the switch back to using "requests" (which retries many types of download failures). However like with ARM, it seems there's something crashing stuff (seems freetype and buffer related). If it crashes intermittently in CI, then it'll keep crashing in CI and probably locally for people too. This is the second reason why close/reopen of the PRs to redo the builds isn't good - we want to know if there's an intermittent issue (especially new ones). If builds are restarted until they pass, then that's dangerous because new intermittent issues can be introduced.

Caching docker downloads isn't really useful. We already have a base image where all the dependencies are compiled. So there shouldn't be a speed advantage to caching docker downloads (unconfirmed). This is the reason I suggested trying the manlinux builds split up into one job per base image. There are overheads to each job, so maybe even splitting this way is slow.

I mentioned this before, but I'd like to mostly switch to using cibuildwheel. To reduce our config, take advantage of their maintenance, and to make our builds portable to different CI systems.

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to experiment with changes in this PR... but at the moment I'd like to mainly focus on changes that fix intermittent issues, or help us release.

@ankith26
Copy link
Contributor Author

ankith26 commented May 29, 2021

Hello!

True. We would need to migrate to cibuildwheel as a middle-term goal.
And also, homebrew might create problems (especially with older macs, I think), so you are right, we cannot release with those.

The windows wheels from github actions are pretty stable I guess, I have tested few of them myself, and also, I have linked the GH Actions wheels output of this PR on discord, for some people interesting in trying out the latest dev version, and I got no complaints about it.

But yea, the main thing this PR is addressing is pip caching, splitting manylinux into sub builds, and also fixes an intermittent SDL1-sdist issue that has been going on in some PRs

If you like, I could un-comment out appveyor builds and travis releases (so that we can do the next release with those, if you want), but having GH actions running for windows in the background does not have any negative effects, so why not?

@ankith26 ankith26 added this to the 2.0.2 milestone Aug 14, 2021
@ankith26
Copy link
Contributor Author

ankith26 commented Aug 14, 2021

Hmmm travis is officially shut down now, so no use of hanging on to travis scripts anymore, So I will remove travis completely. But appveyor is still useful, and I guess we can still have it running, so imma uncomment appveyor stuff so that we could release with it.

And I will force push to clean messy history at last to keep only the good commits, and it becomes more easy to review ;)

EDIT: seems like travis is still required, so I left that part untouched

@ankith26 ankith26 force-pushed the gh-actions-update branch 2 times, most recently from f90ff73 to 7e37dea Compare August 14, 2021 05:32
@ankith26

This comment has been minimized.

@ankith26 ankith26 changed the title Github Actions improvements Github Actions improvements, parallel builds, python 3.10 CI and other minor fixes Aug 15, 2021
@ankith26
Copy link
Contributor Author

ankith26 commented Aug 15, 2021

Ohh.. seems like travis is back, well, for arm64 testing anyways. I think I should be reverting the travis removal this PR is doing

And now this PR is ready for review

EDIT: This PR also seperated GH actions aarch64 build into a seperate matrix, for concurrent builds

@ankith26 ankith26 changed the title Github Actions improvements, parallel builds, python 3.10 CI and other minor fixes Github Actions parallel builds, python 3.10 CI, pip wheel caching and other minor fixes Aug 16, 2021
@ankith26
Copy link
Contributor Author

I updated the opening comment on this PR to explain what this PR does, and some future ideas. To avoid some confusion, I will hide some of my comments that are redundant

Copy link
Contributor

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've still got my support on this one.

It's important that we get 3.10 wheels out soon, and faster manylinux builds are a great bonus.

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤 馃帀 very good. I like how this reduces the config we have so much too!

@illume illume merged commit 4222e2a into main Oct 5, 2021
@illume illume deleted the gh-actions-update branch October 5, 2021 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants