-
Notifications
You must be signed in to change notification settings - Fork 162
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automate creating Windows installers for GAP releases with GitHub Actions #4389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, this looks like it was a ton of work @wilfwilson @ChrisJefferson.
Disclaimer: I haven't looked into the sage-windows repository for more than 30 seconds.
I've left a few minor comments. Besides from those, everything looks fine to me.
I also added the label infrastructure.
Looking at /~https://github.com/wilfwilson/gap/actions/runs/738243905 it took about ~1 hour to produce the windows binaries. So I am wary about running this too often. Looking at the x86 builds, it takes about 10 minutes of preparation before it gets to building GAP. I wonder if the GH Actions cache system could be used to help with that? Compiling all packages: some of them fail (e.g. CddInterface), so once #4386 is merged, that may lead to failures here (?). I think compilation of all packages then finishes after another 30 minutes, so roughly 40 minutes in total were spent so far. The log visible on the website then ends as is too long; it says I can download the full log, but I didn't do that yet. So I wonder if https://ccache.dev works on Windows with Cygwin; if we could use it and use the GitHub cache system on the ccache, it might reduce the compilation time. Just a thought, though, it may be too much of a hassle to get working. BTW, in the x86 logs, I saw this (didn't look at the x86_64 logs); not sure if it is "normal" or we should worry about it:
|
My general feeling is there shouldn't be need to run this more than once a release, and maybe also once a week, so I'm not too worried about the time, also, any type of caching is dangerous with cygwin because if you cache binaries built against a different version of the cygwin DLL, bad things happen. I do want to look at the remaining packages which aren't building yet -- first I'd like to get testing of all packages working in Cygwin, so I could easily see what is / isn't working, as I suspect some packages might be building but not running. |
I don't think running this just once per release is enough, we must run this regularly if we want to notice regressions quickly (experience tells that it then is much easier to resolve them). Running it on each update of In either case, we badly need CI failure notifications again, see issue #4390. Another question: how much work would it be to split the "Compile GAP and its packages, and create the installer" step up? E.g. into these:
That would help with pinpointing issues and generally make the GH Actions log a bit easier to navigate. |
I'm happy for there to be manually-dispatched workflow runs and scheduled workflow runs. That should be no problem, I'll implement those sometime, and we discuss the precise parameters that they should have. In my opinion, the details of how the script of the The purpose of this PR is to implement a GitHub Actions Workflow that:
I think that the comments and details relating to the steps involved in the |
I will now respond on a high level to some of the discussions about resource usage and making things faster. The summary of my initial suggestions for making this faster: Only build the 64-bit installer most of the time, and teach Other comments:
From #4389 (comment) (@fingolfin):
I agree! From #4389 (comment) (@ssiccha):
No, under the current setup a GitHub Actions run of It takes less time on a PR because there I only do From #4389 (comment) (@fingolfin):
#4386 only causes From #4389 (comment) (@ChrisJefferson):
Sure – that's great 🙂! I don't mind waiting for all packages to be working in Cygwin before merging this. But at the same time, I don't think broken packages should stop this being merged, because the Workflow runs successfully even with broken packages - it just means that those packages won't work properly in the resulting Windows GAP installation. From #4389 (comment) (@fingolfin):
I agree that it would be great to have this. But I think it would be a large amount of work, although I don't know. |
Oops, forgot one. @fingolfin those lines of the form
also appear in the CI of /~https://github.com/ChrisJefferson/sage-windows, so they're not particular to this PR. I've got no idea of their importance. |
368d9da
to
a3ea10f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, under the current setup a GitHub Actions run of release.yml on a pull request takes ~42 minutes (see e.g. /~https://github.com/gap-system/gap/actions/runs/738246766), of which ~18 minutes is the already-existing release scripts, and then that is followed by the parallel Windows installer jobs, which each take about ~24 minutes. Which in my opinion is a reasonable length of time. We could further reduce our Windows usage by only building on one architecture (i.e. 64-bit) on PRs.
Ok, that sounds great! I have no idea whether our usage of windows machines may become a bottleneck during GAP days. So I'm undecided on whether to include building 32 bit releases on every PR. However, we can still make that minor change in an extra PR.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for taking care of this!
Since it's just 24 minutes for a PR build, I'd be fine with just enabling this on all PRs after all. Anyway, feel free to restore that, or merge this as-is. |
6f92432
to
91a8a1e
Compare
This comment has been minimized.
This comment has been minimized.
I got the The artifacts are currently only being uploaded on the daily scheduled job, and only living for one day (i.e. roughly until the next job runs). I might upload artifacts for I've still got PR jobs disabled for now until I work out how to best only build on architecture. I might enable them in a separate PR. But the reason that I've still got the "DO NOT MERGE" label is that I have not actually tried running the resulting Windows installer(s). I don't currently have access to a Windows computer. But I'll work out a way, possibly ask some people to help. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure what the state of this PR is now. What is left to be done, exactly? Is a decision needed on something, or is it just a matter of finding time to implement some already made decisions?
It may also help to take this in steps: e.g. some changes in this PR could probably already go in, e.g. those to make_archives.py
look to me as if they are mostly cleanup and could mostly go in right away?
Regarding the 32 vs 64 bit issue: right now, I think we only test Cygwin 64 bit; so it would be a reasonable option if we for now just built the 64bit installer. And then open a separate PR/issue regarding the 32bit one. This way, we would reap the majority of the benefit now and postpone the icky technical discussions... I hope, at least? Of course that's just a suggestion, and it might be a bad one, I do not claim to fully understand the problem scope
This comment has been minimized.
This comment has been minimized.
92f222a
to
75c3677
Compare
75c3677
to
a1370ff
Compare
I think this is very nearly ready to be merged. The jobs run on PR (64-bit only), push branch/tag, on a daily schedule (on the master branch), and manually. You can see an up-to-date example of the workflow in this PR running:
I have asked @ChrisJefferson to install both installers from that If he reports no problems, then I'll remove the 'DO NOT MERGE' label and then merge, soon after (if no one beats me to it). |
@ChrisJefferson tried this installers and (at least on a quick check!) it all seemed in order. So I'll merge! |
(Phew, this was a lot of work, building on what must've been a lot of work by @ChrisJefferson (who was building on a lot of work by @embray and perhaps others at Sage...) and the others involved in the creation of GAP's existing GitHub Actions release system, and I'm sure many others too. Thank you everyone! Open source software collaboration can be nice. 🙂)
This PR uses my fork* of @ChrisJefferson's fork of SageMath's sage-windows repository to create Windows installers for GAP, and in particular, when a new version of GAP is released this creates an installer from the
gap-X.Y.Z.tar.gz
archive produced by the current release system, and adds it to the GitHub release.It adds a new
cygwin
job to thereleases
workflow, along with two new short Python scripts indev/releases
that it uses. The existingreleases
job is renamed tounix
. The new workflow job is heavily commented, so if you are reviewing, please read those comments carefully, in which I describe what the steps are doing, and in which I describe several possible changes/improvements to the way that it works (which would possibly remove the need for the two new Python scripts).In summary:
cygwin
job runs after each run of theunix
job (which creates all of the GAP/package tarballs and zip files).on: pr
, it wraps the merge commit of the GAP PR, with only the required packages, into a 64-bit installer.on: push: tags
, it wraps thegap-X.Y.Z.tar.gz
that it gets from the just-created GitHub release, and uploads the installers to the release.make bootstrap-pkg-full
. If the trigger ison: schedule
, then it also uploads the installers as artifacts (with a retention period of 1 day)* I have opened a PR ChrisJefferson/sage-windows#1 to merge my fork into Chris's fork, and eventually Chris's fork will be put somewhere more appropriate.