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

attempt to use local actions #10503

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geekosaur
Copy link
Collaborator

@geekosaur geekosaur commented Nov 1, 2024

Extremely experimental, and with all the limitations and restrictions I keep finding in GitHub Actions it'll probably fail in the messiest way it can.

At present this is incomplete but sufficient to see if this has any chance of working to begin with. If it somehow does, I'll look into abstracting out the other sub-jobs, then making an overnight validate for Tier 2 platforms and probably a prerelease job (which would fix the recently revealed problem where if there is no need to rebase on merge, no prerelease is made).

Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions). (No point in backporting, as even the LTS prerelease part has to live on master.)

@geekosaur geekosaur self-assigned this Nov 1, 2024
@geekosaur geekosaur force-pushed the validate-actions branch 29 times, most recently from 8565a7c to 501bde6 Compare November 2, 2024 13:59
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Amazing, thanks!

@ulysses4ever
Copy link
Collaborator

But in a follow-up, we really need to sweep the less-relevant comments (like # look familiar?): They make the reader not want to read the comments at all because of the low signal/noise.

@geekosaur
Copy link
Collaborator Author

They're not entirely pointless: they mark the (IMO unnecessary except that GHA is stupid) duplications.

@ulysses4ever
Copy link
Collaborator

well, it's too subtle. I'd appreciate something less playful and more direct. E.g. for the matrix, duplicate matrix yet again see [Note: matrix duplication].

@geekosaur
Copy link
Collaborator Author

At this point I do not recommend merging this unless you have someone, preferably several someones, who understand it; otherwise your bus factor situation is looking pretty dire. If necessary I can expand the prerelease jobs out to full form and submit those separately, as they're the most urgent part of this and I don't want to leave you in a bad situation.

Copy link
Member

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

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

Let's put this PR away for the moment

@ulysses4ever ulysses4ever marked this pull request as draft November 7, 2024 16:41
@9999years
Copy link
Collaborator

At this point I do not recommend merging this unless you have someone, preferably several someones, who understand it; otherwise your bus factor situation is looking pretty dire.

Ehhh, GitHub Actions stuff is kinda always like that. It's usually not too miserable to make small tweaks to, but changes in design goals require rearchitecting, as I think you're running into with this!

I haven't read over this with eagle eyes but it looks largely fine to me. A couple notes:

  • Try running over this with actionlint and see if it catches anything.
  • It might be worth porting some of the logic to Haskell, like cabal-validate; then, it's easier to test more of the logic locally, and you can cut down on the scary GitHub Actions shell-scripts-templated-YAML stuff. (In this design, the actions ideally become fairly thin wrappers that set up some inputs and then call the relevant script with the relevant arguments.)

@geekosaur geekosaur force-pushed the validate-actions branch 4 times, most recently from 32f70a0 to c262de2 Compare December 20, 2024 01:25
@geekosaur geekosaur requested a review from Kleidukos December 20, 2024 01:26
@geekosaur geekosaur marked this pull request as ready for review December 20, 2024 02:30
@geekosaur
Copy link
Collaborator Author

Dammit, GitHub, this validate.yml is essentially unrelated to master. Quit whining about "conflicts" that are pretty much you trying to rewrite it into the one on master!

@geekosaur geekosaur force-pushed the validate-actions branch 2 times, most recently from 61e315d to 66bd645 Compare December 25, 2024 00:58
@geekosaur
Copy link
Collaborator Author

Installed actionlint and fixed a number of things CI wouldn't have caught for various reasons.

@geekosaur
Copy link
Collaborator Author

geekosaur commented Dec 25, 2024

Sigh. NTS: GitHub's insistence on turning validate.yml back into the one on master means it will include messing up validate.sh in cabal-setup in the process; remember to fix it after rebasing.

Extremely experimental, and with all the limitations and
restrictions I keep finding in GitHub Actions it'll probably fail
in the messiest way it can.

At present this is incomplete but sufficient to see if this has
any chance of working to begin with. If it somehow does, I'll
look into abstracting out the other sub-jobs, then making an
overnight validate for Tier 2 platforms and probably a prerelease
job (which would fix the recently revealed problem where if there
is no need to rebase on merge, no prerelease is made).
@geekosaur
Copy link
Collaborator Author

The tests suddenly aren't finding the caches from the build step. This could be really bad.

@geekosaur
Copy link
Collaborator Author

GitHub has started aggressively pruning caches; we went from around 300GB in use to 25GB, which means they can't be used as shared state between the build and test stages any more. Will rewrite to use artifacts to pass the important parts around, but that will have its own limits and may have its own problems.

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.

4 participants