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

Improve Github Actions. #5687

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Improve Github Actions. #5687

merged 1 commit into from
Apr 4, 2024

Conversation

erikd
Copy link
Contributor

@erikd erikd commented Feb 25, 2024

Description

Improve Github Actions.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@erikd erikd marked this pull request as draft February 25, 2024 21:30
@erikd erikd force-pushed the erikd/gha-tweaking branch from db1ece2 to 57c277c Compare February 26, 2024 00:57
@erikd erikd self-assigned this Feb 27, 2024
@erikd erikd force-pushed the erikd/gha-tweaking branch 5 times, most recently from 0bf6d53 to 53ca490 Compare March 1, 2024 06:47
@erikd erikd marked this pull request as ready for review March 1, 2024 06:47
@erikd erikd requested review from a team as code owners March 1, 2024 06:47
@erikd erikd changed the title WIP: Improve Github Actions. Improve Github Actions. Mar 1, 2024
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

Nice work!

My main concern is about haskell/cabal#9587 workaround - see my comments.

I have also a questions about caching mechanism. I have a few doubts about the cache availability https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache.

  1. The docs explain that the cache is only available to child branches. That means that if we don't trigger this build on master, the feature branches won't have access to the cache - correct? If that's true, we should also add the trigger on pushes to master.
  2. Any dependency change (e.g. bump for one of our CHaP packages) will cause cache miss for cabal store, which means rebuilding of a whole dependency tree, including hackage deps. I'm wondering if it would make sense to split the cache into two: cabal store, and stuff in dist-newstyle. It would speed-up workflows when bumping dependencies. How much time does a build take when you just bump a dependency?

# The following seems like a duplicate but it is due to a bug in cabal which can sometimes
# cause build intermittent build failures on Windows, so we run the build twice.
#
# https://github.com/haskell/cabal/issues/9587
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing this step? The bug doesn't seem fixed? The original discussion https://github.com/IntersectMBO/cardano-node/pull/5625/files#r1442647736

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How easy was that issue to reproduce, because I have not seen it for ages.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say one in 20 builds? It was happening sometimes.

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 would have done at least 20 build of this an not yet seen it.

Do you want it back in?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try without it and see if it happens again. Maybe it was a side effect from cabal-cache.

.github/workflows/haskell.yml Show resolved Hide resolved
.github/workflows/haskell.yml Outdated Show resolved Hide resolved
.github/workflows/haskell.yml Outdated Show resolved Hide resolved
@erikd erikd marked this pull request as draft March 8, 2024 08:39
@erikd erikd force-pushed the erikd/gha-tweaking branch 3 times, most recently from 52f94f7 to 75d8f44 Compare April 2, 2024 02:55
@erikd erikd marked this pull request as ready for review April 2, 2024 03:49
.github/workflows/haskell.yml Outdated Show resolved Hide resolved
.github/workflows/haskell.yml Outdated Show resolved Hide resolved
@erikd erikd force-pushed the erikd/gha-tweaking branch 4 times, most recently from 331dc90 to 5262ff8 Compare April 3, 2024 09:07
  * Make it more like `cardano-base`.
  * Drop the ad-hoc caching in favour of Cabal caching.
@erikd erikd force-pushed the erikd/gha-tweaking branch from 5262ff8 to 23280e0 Compare April 3, 2024 09:19
run: |
# The tests call out to msys2 commands. We generally do not want to mix toolchains, so
# we are very deliberate about only adding msys64 to the path where absolutely necessary.
${{ (runner.os == 'Windows' && '$env:PATH=("C:\msys64\mingw64\bin;{0}" -f $env:PATH)') || '' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

@erikd erikd force-pushed the erikd/gha-tweaking branch from de7d684 to 23280e0 Compare April 4, 2024 08:25
@erikd erikd added this pull request to the merge queue Apr 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 4, 2024
@smelc smelc added this pull request to the merge queue Apr 4, 2024
@disassembler disassembler removed this pull request from the merge queue due to a manual request Apr 4, 2024
@disassembler disassembler merged commit 9a9b499 into master Apr 4, 2024
40 of 41 checks passed
@disassembler disassembler deleted the erikd/gha-tweaking branch April 4, 2024 17:07
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