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

Avoid templating GitHub Actions workflow #7952

Merged
merged 34 commits into from
Feb 19, 2022

Conversation

andreabedini
Copy link
Collaborator

@andreabedini andreabedini commented Feb 4, 2022

GitHub Actions workflow have sufficient power to express what we need.
We don't need to maintain and additional templating solution on top.

This converts: linux.yml, macos.yml and windows.yml to native GitHub Actions syntax. The result should closely (but perhaps not perfectly) match the original version.

EDIT:

It would be better to check GHC versions with startsWith(matrix.ghc, "8.10") otherwise bumping minor versions is going to be error prone I am in


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@andreabedini andreabedini marked this pull request as draft February 4, 2022 07:53
@andreabedini andreabedini marked this pull request as ready for review February 4, 2022 08:10
@Mikolaj Mikolaj requested a review from fgaz February 4, 2022 08:56
@Mikolaj
Copy link
Member

Mikolaj commented Feb 4, 2022

@fgaz is just extending our CI to GHC 9 and 9.2 and converting it in the process in #7907, so it may be beneficial to coordinate...

@andreabedini
Copy link
Collaborator Author

Thanks @Mikolaj, I think I was able to include most of the changes of #7907. Happy to coordinate to finish the job.

@fgaz
Copy link
Member

fgaz commented Feb 4, 2022

Hmm.. why isn't the linux job running?

@andreabedini
Copy link
Collaborator Author

Hmm.. why isn't the linux job running?

/shurg I don't know. I am working to be able to test this out locally (with act).

I will review the failures on Monday.

@fgaz
Copy link
Member

fgaz commented Feb 7, 2022

Hmm.. why isn't the linux job running?

/shurg I don't know. I am working to be able to test this out locally (with act).

Looks like the workflow is invalid https://github.com/haskell/cabal/actions/runs/1794333751

GitHub Actions workflow have sufficient power to express what we need.
We don't need to maintain and additional templating solution on top.
- Run bootstrap.yml on ubuntu-latest
- Use explicit matrix for linux.yml
- Drop containers in favour of haskell setup action
- Drop workaround for ancient git
I belive this is only necessary to run `cabal man` which we do not in
the CI.
@andreabedini
Copy link
Collaborator Author

Rebased

@andreabedini
Copy link
Collaborator Author

The workflows for macos and linux at this point are very similar and could be merged. They only differ in how they install cabal-plan (one downloading a binary, one installing from hackage)

- Remove cabal-plan, we actually never call it (I think)
- Remove vendored cabal-doctest
- Remove few stray allow-newer clauses no longer necessary, apparently

Originally done by @gbaz in PR haskell#7907.
@jneira
Copy link
Member

jneira commented Feb 18, 2022

The Haskell community has been severely burned by the treason of Travis CI. CI just vanished for hundreds (or thousands?) of Haskell projects when travis-ci.org was shut down. The community is still recovering from this blow. I donated a GHA CI to dozens of Haskell projects. This was possible through @phadej 's heroic migration of Haskell-CI from Travis to GHA.

That was a unsung epic travispocalypse, thank you for your work.
Lot of work was invested in travis and its sunset was really painful. The lessons from that pain should not be forgotten.
However here in Spain we have a proverb: "Gato escaldado, del agua fria huye" (in english it would be "once bitten twice shy" i think)

I think there are two directions in ci lock in (or adoption :-P):

  1. Up-down: this is about the top level inputs of workflows. The matrix of ghc versions and os's and the different parameters for each combination of them.
  • That was abstracted from ci thanks to GenValidate and that is which we are giving out to github with this pr
  • They are essential for the ci logic but
    • The size of data is relatively small and dont vary too much
    • They are not actual code so no ifs, no loops
    • All ci providers have similar ways to encode them so it is relatively easy convert between them
  1. Bottom: The actual individual steps of the workflow, full of logic and default values which needs to be programmed
  • Imho that logic should live, in that precise order:
    1. haskell code (test suites, benchmarks, haskell scripts, build config)
    2. other language scripts: sh, python, etc
    3. using ci specific features
  • cabal uses 1 and 2 (with validate.sh and make etc) and i think we should keep them (but we can discuss that in other issue)

And for the top inputs we always could have it encoded in a ci agnostic data format and inject it in the ci provider, if ci allows to do it (see the linked above https://www.cynkra.com/blog/2020-12-23-dynamic-gha/)

@jneira
Copy link
Member

jneira commented Feb 18, 2022

They are not actual code so no ifs, no loops

For completeness, they are not code but it could be computed, deriving it from other sources, to avoid duplication of such sensitive data.
For example we could extract the ghc versions from tested-with like haskell-ci does. But being a relatively small dataset, encoding its generation to avoid oversights and have to do greps should be very cheap to worth, imho.

EDIT: want to note GenValidate did not compute the values but they were duplicated and hardcoded in the haskell code

@jneira
Copy link
Member

jneira commented Feb 18, 2022

I have a minor nitpick: the experimental builds are marked as failing, so the entire build is marked that way. Those workflows will not be added to branch protection ruels and prs will be merged anyways but aesthetically it does not look nice
(Btw we have to remember add the succesful workflows to branch protection rules once this is merged and rebase the rest of prs // @Mikolaj)

Otoh, have them red is a compelling reason to work on fixing this so i am not strongly against it, thoughts?
Maybe marking it as "yellow" would be a compromise. not sure how could be done, cancelling them?

In hls we have a final post-jobs which marks the final workflow state, not sure if it could help:

https://github.com/haskell/haskell-language-server/blob/96ea854debd92f9a54e2270b9b9a080c0ce6f3d1/.github/workflows/test.yml#L239-L249

@andreabedini
Copy link
Collaborator Author

I have a minor nitpick: the experimental builds are marked as failing, so the entire build is marked that way.

Only the "branches" are marked as failing, the entire workflow is green if all the required branched pass, see https://github.com/haskell/cabal/actions/runs/1863088120

For example we could extract the ghc versions from tested-with like haskell-ci does.

This is a good idea (especially in relation to the post about dynamic workflows you posted). But, again, my intention is to try to simplify all variations rather than capturing them perfectly.


defSteps :: [String]
defSteps =
[ "print-config"
Copy link
Member

Choose a reason for hiding this comment

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

hmm steps also lives here, sad to see they go away

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

Many thanks again, really a great work

@jneira
Copy link
Member

jneira commented Feb 18, 2022

This pr cant be merged automatically, as it changes required jobs names as commented above we can change branch protection rules to match new identifiers and merge it (to check both pr jobs and updated ruels) and then aaaall prs need to be rebased to pass ci

//cc @Mikolaj

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

Looks good! I'll merge tomorrow if nobody objects. Specifically, let's squash-merge since there are a lot of fixup commits. @andreabedini if you don't want them squashed please tell!

@fgaz fgaz merged commit 65318cc into haskell:master Feb 19, 2022
@fgaz
Copy link
Member

fgaz commented Feb 19, 2022

Thanks a lot @andreabedini!

Now I just have to re-select the required jobs... please ping me or @Mikolaj if prs are stuck because of that

run: sh validate.sh -j 2 -w ghc-8.8.4 -v --lib-only -s lib-suite-extras --extra-hc /opt/ghc/7.0.4/bin/ghc-7.0.4
- name: Validate lib-suite-extras --extra-hc /opt/ghc/7.2.2/bin/ghc-7.2.2
run: sh validate.sh -j 2 -w ghc-8.8.4 -v --lib-only -s lib-suite-extras --extra-hc /opt/ghc/7.2.2/bin/ghc-7.2.2
- name: Validate lib-suite-extras --extra-hc /opt/ghc/7.4.2/bin/ghc-7.4.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see these steps in new CI setup. Is cabal-install dropping support for older GHC.

AFAICS the oldest GHC you try to run tests with is GHC-8.0

Is this correct and intentional, @fgaz, @Mikolaj ?

Copy link
Member

Choose a reason for hiding this comment

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

Afaics it is not intentional and should be restored.

Copy link
Member

@jneira jneira Feb 20, 2022

Choose a reason for hiding this comment

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

As we are deprecating build cabal with ghc < 8.0, all older version should be checked using a newer build (including 7.8 and 7.10) like < 7.6 before this pr

EDIT: the haskell setup action fall back to apt inlinux so maybe it could be used

Copy link
Member

Choose a reason for hiding this comment

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

the oldest ghc instalable with ghcup is 7.10.3, so we have to continue using apt if we want to keep older ones

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@phadej It was intentional, following #7534 but now I think I have misunderstood.

We are deprecating building with GHC < 8.0 but we still want to test cabal can use GHC 7.8 and 7.10. Is this correct?

It's a bit of headache (which GHC version we should use to compile the cabal that compiles with what GHC version?) but it can definitely be readded.

Mind adding an issue to track? Please mention me there.

Copy link
Member

@Mikolaj Mikolaj Feb 21, 2022

Choose a reason for hiding this comment

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

@andreabedini: great job! I might have mislead you re #7534. Let's clean up the fallout later on. Edit: in #8000.

@jneira
Copy link
Member

jneira commented Feb 20, 2022

Thanks a lot @andreabedini!

Now I just have to re-select the required jobs... please ping me or @Mikolaj if prs are stuck because of that

The linux and macos validate jobs has the same name and in my experience tha does not play well with branch protection rules, which needs unique jobs names

@andreabedini
Copy link
Collaborator Author

Thanks a lot everyone <3 It's been a jouney for me!

@jneira jneira mentioned this pull request Feb 21, 2022
1 task
@andreabedini andreabedini deleted the simplify-github-workflows branch February 21, 2022 09:02
@jneira jneira mentioned this pull request Mar 14, 2022
Kleidukos pushed a commit to Kleidukos/cabal that referenced this pull request Mar 30, 2022
* Avoid templating GitHub Actions workflow

GitHub Actions workflow have sufficient power to express what we need.
We don't need to maintain and additional templating solution on top.

* Add GHC 9.2, bump bounds, fix syntax

* Switch to official haskell image

* Always run cli tests for ghc 8.2 and above

* Remove step to regenerate GitHub Actions workflows

* Fix missed reference to GHC version

* Fix yaml syntax

* Fix type in the GHC version

* More CI changes

- Run bootstrap.yml on ubuntu-latest
- Use explicit matrix for linux.yml
- Drop containers in favour of haskell setup action
- Drop workaround for ancient git

* Remove unneeded package from CI setup

I belive this is only necessary to run `cabal man` which we do not in
the CI.

* Drop old GHCs from the CI

* Switch macos.yml to haskell/action/setup

Also add the same GHC versions as Linux.

* Simplify CI

- Remove cabal-plan, we actually never call it (I think)
- Remove vendored cabal-doctest
- Remove few stray allow-newer clauses no longer necessary, apparently

Originally done by @gbaz in PR haskell#7907.

* Mark GHC 9.2.1 as experimental

* Remove reference to cabal-plan from validate.sh

* setup-haskell action already runs cabal update

* Add missing build matrix in test-windows-dogfood

* Replace cabal-plan list-bin with cabal list-bin

* Enable caching in the CI

* Fix typo

* Remove continue-on-error until I figure it out

* Keep naming consistent

* Temporarily disable 8.0.2 on macos

* Add missing step id

* Tweaks

Remove workaround for nektos/act, it accidentally sneaked in.

* More tweaks

* Tweaks

* Restore cabal-plan, temporarily mark everything experimental

cabal list-bin doesn't seem to work like cabal-plan does.

* Tweaks

* Ensure cabal-plan executable gets built

* Install automake on MacOS

* Tweaks

* Tweaks

Link experimental flags to relative GitHub issues

* Fix typo
andreabedini added a commit to andreabedini/cabal that referenced this pull request May 5, 2022
* Avoid templating GitHub Actions workflow

GitHub Actions workflow have sufficient power to express what we need.
We don't need to maintain and additional templating solution on top.

* Add GHC 9.2, bump bounds, fix syntax

* Switch to official haskell image

* Always run cli tests for ghc 8.2 and above

* Remove step to regenerate GitHub Actions workflows

* Fix missed reference to GHC version

* Fix yaml syntax

* Fix type in the GHC version

* More CI changes

- Run bootstrap.yml on ubuntu-latest
- Use explicit matrix for linux.yml
- Drop containers in favour of haskell setup action
- Drop workaround for ancient git

* Remove unneeded package from CI setup

I belive this is only necessary to run `cabal man` which we do not in
the CI.

* Drop old GHCs from the CI

* Switch macos.yml to haskell/action/setup

Also add the same GHC versions as Linux.

* Simplify CI

- Remove cabal-plan, we actually never call it (I think)
- Remove vendored cabal-doctest
- Remove few stray allow-newer clauses no longer necessary, apparently

Originally done by @gbaz in PR haskell#7907.

* Mark GHC 9.2.1 as experimental

* Remove reference to cabal-plan from validate.sh

* setup-haskell action already runs cabal update

* Add missing build matrix in test-windows-dogfood

* Replace cabal-plan list-bin with cabal list-bin

* Enable caching in the CI

* Fix typo

* Remove continue-on-error until I figure it out

* Keep naming consistent

* Temporarily disable 8.0.2 on macos

* Add missing step id

* Tweaks

Remove workaround for nektos/act, it accidentally sneaked in.

* More tweaks

* Tweaks

* Restore cabal-plan, temporarily mark everything experimental

cabal list-bin doesn't seem to work like cabal-plan does.

* Tweaks

* Ensure cabal-plan executable gets built

* Install automake on MacOS

* Tweaks

* Tweaks

Link experimental flags to relative GitHub issues

* Fix typo
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.

6 participants