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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to GitHub Actions for CI #1085

Merged
merged 12 commits into from
Mar 26, 2024

Conversation

bashbaug
Copy link
Contributor

This change:

  1. Switches most CI to GitHub actions for ordinary spec builds on pushes and pull requests.
  2. Disables Travis for CI on pushes and pull requests, except for tags, where we'll still use automated Travis builds and deployments.

We'll want to test (2) carefully before any spec releases since I suspect the Travis builds will eventually stop working. We eventually should switch deployments to GitHub actions, at which point we can stop building with Travis entirely. In the meantime, though, this should enable ordinary CI builds to finish much faster. As an example: my test builds were completing in ~3 minutes, whereas Travis builds from this morning are still waiting.

@bashbaug bashbaug requested a review from kpet March 19, 2024 19:11
@bashbaug
Copy link
Contributor Author

I've updated this so it works with the new spec toolchain, now.

Copy link
Contributor

@kpet kpet left a comment

Choose a reason for hiding this comment

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

Looks fine overall. Left one question.


- name: Generate core + extension specs (HTML)
run: |
python3 makeSpec -clean -spec khr OUTDIR=out.khr -j 12 html
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't you generating the PDFs as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to more-or-less faithfully follow what the Travis script was doing previously. It generated both PDF and HTML versions of the "core" specs, but only HTML versions of the "khr" specs.

Here are the relevant lines from the Travis script:

  - python3 makeSpec -clean -spec core OUTDIR=out.core -j 5 api c env ext cxx4opencl
  - python3 makeSpec -clean -spec khr OUTDIR=out.khr -j 12 html manhtmlpages

@oddhack any specific reason for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Generating the PDF is in there just to verify the PDF pipeline is working. There aren't structural elements in the extension spec that would challenge it any more than the core spec, and PDF generation is by far the slowest operation, so I only do it once.

The -j 12 is because there are a lot of manhtmlpages to generate, each quite fast. Very unlikely we would actually get 12 cores of CI though, I think the GH runners only have two.

If there's some reason people need the PDF artifacts then they can be built, of course. But for Vulkan purposes, CI is just checking that the pipeline works - our publication artifacts are built outside of CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this running as a pre-commit filter, as the YML filename suggests? If so, that's another reason to avoid PDF generation as much as possible since that lengthens the loop.

We've done OK in Vulkan just making spec generation part of the regular post-commit CI step and fixing things after the fact, FWIW.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this running as a pre-commit filter, as the YML filename suggests?

We currently run it on pushes and pull requests, so it's both a pre-commit filter and a post-commit validation. I suppose we could separate that if needed, although the current job executes pretty quickly. Looking at the latest run, it completed in ~3 minutes: One minute of that was installing packages, and another minute was building the online reference pages. Spec-wise, HTML generation took ~10 seconds, and HTML + PDF generation took ~35 seconds.

@SunSerega
Copy link
Contributor

SunSerega commented Mar 22, 2024

What I like to do in workflows like these is to also push the changes in generated files to another ref:
https://github.com/SunSerega/POCGL/blob/8fb6f3c62724db62731283fda4c6dd3705f9635c/.github/workflows/on%20commit.yaml#L147-L163
That way, the generation process is checked not only for errors but also for unintended changes.
And if the changes are intended, you can fast-forward your branch to those changes, without building locally.
Once you push your branch together with the fast-forwarded changes, the build changes branch is deleted automatically, because this push also triggers another workflow run on GitHub.

FYI this cannot spawn another workflow on the branch with changes - GitHub is protected against recursive workflow runs even if you don't specify branch exclusion in triggers.

@oddhack
Copy link
Contributor

oddhack commented Mar 23, 2024

What I like to do in workflows like these is to also push the changes in generated files to another ref:

There are always going to be differences in the generated PDF / HTML specs just because they encode the generation time and commit ID into the documents, if nothing else. Also PDF / HTML specs are large and will build up rapidly over time, so some mechanism to be confident that the deleted artifacts are really removed would be helpful if this is done.

@SunSerega
Copy link
Contributor

SunSerega commented Mar 23, 2024

some mechanism to be confident that the deleted artifacts are really removed would be helpful if this is done

Well, how I always implemented it, the changes/* branch is deleted and recreated every time workflow runs, so only the latest artifacts exist (tho GitHub might hold onto the deleted commit for a bit, but that's on them)

However, having constant changes does make this pattern less comfortable to use.
In the example I linked above, I already have a similar case. I ignore changes in LastPack.log when checking if the changes/* branch should be created again, but still commit this file if there are other changes.
Since here it's a small part of the important files that have to be ignored, I guess this check needs to be more fine-grained somehow. Not sure how it could be done.
The best I can think of is committing without pushing with the new timestamp and commit hash, then replacing the timestamp and commit hash with old ones and diff that instead. But that's kind of ugly.
And maybe not necessary, depending on how you use this pattern? Well, ig you'd need good visual PDF and HTML diffs to make this work then...

@bashbaug bashbaug merged commit ce502ec into KhronosGroup:main Mar 26, 2024
2 checks passed
@bashbaug bashbaug deleted the switch-to-github-actions branch March 26, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants