Skip to content

Conversation

@Mikah-Kainen
Copy link
Collaborator

This PR updates cabal.haskell-ci to run CI on pushes to main and PRs that target main.

I wasn't able to find a way to configure haskell-ci to generate .yml files that run CI on all PRs, but if we ever have a PR targeting a branch, we can modify the .yml files locally for testing.

The changes to haskell-ci.yml came from running haskell-ci regenerate. I used a newer version of haskell-ci, which introduced some additional changes unrelated to this PR.

Fixes #297

I could not figure out how to get haskell-ci to generate .yml files
that run CI on all PRs, but the main cases should be covered by
pushes to main and PRs that target main.

If we ever have a PR targeting another branch, we can modify the
.yml files locally for testing.
@ulysses4ever
Copy link
Collaborator

Thanks! It'd be great to update the other workflow files (in the .github/workflows directory) in the same manner. Can you do it?

branches: main should do the trick well enough. Yes, I just noticed today too that haskell-CI doesn't alow exactly what I asked for:

on:
  push:
    branches:
      - main
  pull_request:

instead, with branches: main, it'll produce

on:
  push:
    branches:
      - main
  pull_request:
    branches:
      - main

but it's worth noting that the meaning of branches is fairly different in these. For pull requests it means the target branch, not the base branch, so it's what we want most of the time.


The CI failure is due to the update of haskell-CI (and to be clear, you were perfectly right to use the latest version of haskell-CI from GitHub). The solution to one part (an example is here) of the issue is simply

error-unused-packages: False

in cabal.haskell-ci.

The second part (e.g. here) doesn't have as easy a solution: you have to fix the missing-fields initialization, as there's no option to disable this recently added check. The good news is, I'm fixing it in my PR. But there's no reason to wait for it --- you can just copy the corresponding lines:

https://github.com/gibbon-compiler/gibbon/pull/294/files#diff-f5fb504c9220f8a4fdc2dbfc1c9eca543aafd4f3bed62e1e7968a9c6ba9307d8L937

@Mikah-Kainen
Copy link
Collaborator Author

Mikah-Kainen commented Sep 25, 2025

Thanks for the detailed feedback! I think I solved the first issue by removing the unused dependencies, but if it turns out we need them for something after all, I will go with your solution of adding

error-unused-packages: False

to cabal.haskell-ci. In general, I think the extra checks are good, so where possible, I will try to keep them enabled and resolve the warnings that trigger them.

I'll tackle the missing-fields initialization tomorrow. It seems like there might be some other problems from Defined but not used, This binding shadows the existing binding and Pattern match(es) are non-exhaustive, which I will also try to work through.

Please let me know if you'd prefer another approach, since I'm happy to adjust!

@Mikah-Kainen Mikah-Kainen marked this pull request as draft September 25, 2025 04:19
@ulysses4ever
Copy link
Collaborator

Sorry, I should have explained more. We do need the lower bound on hashable otherwise some tests become non-deterministic (see #292 (comment) and #295). And there's no good reason to specify this bound without getting the warning (see haskell/cabal#11053).

Defined but not used, This binding shadows the existing binding and Pattern match(es) are non-exhaustive

the first two are mere warnings, so they should not affect the build. The last one (Pattern match(es) are non-exhaustive) is a warning promoted to error recently (haskell-CI/haskell-ci#790). It'll have to be fixed. Note that I made a pass over warnings in a separate PR #294 (although my PR addresses only part of them). You risk to duplicate that work, so maybe we should merge that first.

I'm sorry it's taking more effort than anticipated: haskell-CI's update policies are hectic. I didn't realise how much trickier it will be after the update...

@Mikah-Kainen
Copy link
Collaborator Author

Makes sense, thank you! I’ll take a look at your PR and add back the hashable dependency. Do we also need uuid?

@ulysses4ever
Copy link
Collaborator

uuid can be removed yes, that's a good point

@ulysses4ever
Copy link
Collaborator

now, after -Werror is merged, may be a good time to resume work here?.. I suggest starting with a rebase.

I am not sure about this change, since it the guard hints that it
might break the build on lower versions of GHC. I'm pushing to test
this in CI.
@ulysses4ever
Copy link
Collaborator

Oh my, I didn't realize incomplete-uni-patterns isn't the same as incomplete-patterns (see e.g. https://ghc.gitlab.haskell.org/ghc/doc/users_guide/using-warnings.html#ghc-flag-Wincomplete-uni-patterns), so #294 doesn't help with these. If this is taking too much time to fix properly, you should feel free to disable this warning in the haskell-ci config (or, better yet, per module like here: https://github.com/gibbon-compiler/gibbon/pull/294/files#diff-89481f41b007bf12646a24a3a1411a4c10341e5bf532f018a385785245e6c557R1) leaving it as future work.

@Mikah-Kainen
Copy link
Collaborator Author

There are quite a few places to change, but I have been working through them and hope to get all the way through. If I don't completely finish it, then I will push what I have and disable the warning in the haskell-ci config. Thank you for the links!

A previous commit about removing non-exhaustive pattern matches had
a couple issues, which this commit resolves. The offending commit is
cfa28ce.
Some files still contain incomplete uni-patterns, which trigger
warnings that are treated as errors due to `-Werror`.
This commit adds `{-# OPTIONS_GHC -Wno-incomplete-uni-patterns #-}`
to the top of those files, allowing Gibbon to compile.

This is a temporary measure while we work on removing all
incomplete uni-patterns.
@Mikah-Kainen
Copy link
Collaborator Author

I've removed a fair amount of the non-exhaustive uni-patterns and added {-# OPTIONS_GHC -Wno-incomplete-uni-patterns #-} to the rest! Once CI is green, I'll mark this PR ready for review.

@ulysses4ever
Copy link
Collaborator

     The following packages were specified via -package or -package-id flags,
    but were not needed for compilation:
      - gibbon-0.3 (exposed by flag -package-id gibbon-0.3-inplace)
      - mtl-2.2.2 (exposed by flag -package-id mtl-2.2.2)
      - transformers-0.5.6.2 (exposed by flag -package-id transformers-0.5.6.2)

the latter two could probably be removed. The former looks funny but I guess it's the integration tests that use gibbon executable and not the library. so it really should be build-tool-depends: gibbon instead of build-depends: gibbon.

@ulysses4ever
Copy link
Collaborator

wow, the HoistBoundsCheck test is failing as well??

@Mikah-Kainen
Copy link
Collaborator Author

Sorry, I haven't had a chance to look at this the last few days, but I will see if I can fix a few more of the errors now

@ulysses4ever
Copy link
Collaborator

now the test suite: it also has unused packages:

Building test suite 'test-gibbon' for gibbon-0.3...

<no location info>: error: [-Wunused-packages, -Werror=unused-packages]
    The following packages were specified via -package or -package-id flags,
    but were not needed for compilation:
      - bytestring-0.11.5.1 (exposed by flag -package-id bytestring-0.11.5.1)
      - directory-1.3.7.1 (exposed by flag -package-id directory-1.3.7.1)
      - filepath-1.4.2.2 (exposed by flag -package-id filepath-1.4.2.2)
      - pretty-1.1.3.6 (exposed by flag -package-id pretty-1.1.3.6)
      - process-1.6.17.0 (exposed by flag -package-id process-1.6.17.0)
      - srcloc-0.6.0.1 (exposed by flag -package-id srcloc-0.6.0.1-6a5ed5348e24b559ae8413eb8c5a6b68f6e9dbf5d7a38338069c327b446afd8b)
      - text-2.0.2 (exposed by flag -package-id text-2.0.2)
      - time-1.12.2 (exposed by flag -package-id time-1.12.2)

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.

CI runs twice per update of PR

3 participants