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

Add a workflow to dry run cabal-install #10740

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

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Jan 12, 2025

Fixes #10738. Adds a dry run check of cabal install cabal-install and then fixes the casing of extra-source-files in Cabal-hooks.

Here's a failing run, https://github.com/haskell/cabal/actions/runs/12736390756/job/35496224194.

Excluding .cabal files that are test packages, only 4 of our packages use extra-source-files:

$ find . -type f -name '*.cabal' | grep -v -E 'dist- \
  |cabal-testsuite/PackageTests|Cabal-tests/tests' | xargs grep -P -A 1 'extra-source-files'
./cabal-benchmarks/cabal-benchmarks.cabal:extra-source-files: README.md
./cabal-benchmarks/cabal-benchmarks.cabal-
--
./cabal-testsuite/cabal-testsuite.cabal:extra-source-files:
./cabal-testsuite/cabal-testsuite.cabal-  README.md
--
./solver-benchmarks/solver-benchmarks.cabal:extra-source-files:
./solver-benchmarks/solver-benchmarks.cabal-  README.md
--
./Cabal-hooks/Cabal-hooks.cabal:extra-source-files:
./Cabal-hooks/Cabal-hooks.cabal-  readme.md changelog.md

  • 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).

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.

We already have something very similar in purpose: https://github.com/haskell/cabal/blob/master/.github/workflows/check-sdist.yml Could you instead look why that one didn't help and possibly improve it? Maybe, ask the author @geekosaur if necessary. Otherwise, I'm worried about proliferation of small workflows: it's quickly becoming unmanageable!

@philderbeast
Copy link
Collaborator Author

philderbeast commented Jan 12, 2025

@ulysses4ever on the description of #10738, I mentioned why the existing workflow doesn't catch this problem.

We have .github/workflows/check-sdist.yml but this does less, creating a cabal sdist cabal-install and then installing that.

Also the existing workflow runs on a matrix whereas this one runs only on the latest GHC and I think that would be sufficient to catch these kinds of errors.

ghc:
["9.12.1", "9.10.1", "9.8.1", "9.6.1"]

I've requested a review from @geekosaur, knowing that he's been involved in maintaining the GitHub workflows.

@geekosaur
Copy link
Collaborator

Could you instead look why that one didn't help and possibly improve it?

It didn't help because it only runs on released versions, not master because it can never trigger the specific problem check-sdist is looking for (inability to build with the Cabal bundled with a released ghc). A general dry run makes sense.

As to "out of hand", we have a lot of moving parts; we should not be surprised that a lot of things need to be checked. And combining them to shrink the checks list causes other problems, especially if the checks aren't very related. check-sdist could probably be changed to do this by skipping the actual build if the "do we match Cabal major version?" check fails instead of not running at all, but would need a dry-run build added before that check. It's not clear if this makes logical sense to someone trying to diagnose CI, though.

@ulysses4ever
Copy link
Collaborator

We have .github/workflows/check-sdist.yml but this does less, creating a cabal sdist cabal-install and then installing that.

Great, let’s make it do more! How about that?

the existing workflow runs on a matrix whereas this one runs only on the latest GHC and I think that would be sufficient to catch these kinds of errors.

Fair, but let’s just add the new stuff under an if conditional.

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

Also see my mainline comment about check-sdist; that may make sense to merge with this, but it'll need a bit of an overhaul including removing the docs hack as mentioned in the first comment.

.github/workflows/check-install.yml Show resolved Hide resolved
.github/workflows/check-install.yml Show resolved Hide resolved
@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Jan 12, 2025

it only runs on released versions, not master

Why does it say on:push:branches:master then? It'd be good to have a documentation as to what exactly it does...

@geekosaur
Copy link
Collaborator

geekosaur commented Jan 12, 2025

Leftover from debugging (I removed similar things from the release branch but apparently missed that one on master).

@philderbeast philderbeast force-pushed the add/dry-run-cabal-install branch 2 times, most recently from 0aa7d62 to 293e517 Compare January 20, 2025 11:47
@Bodigrim
Copy link
Collaborator

Bodigrim commented Feb 1, 2025

Could the actual fix for #10738 please be merged soon? And then you can spend eternity discussing CI jobs.

@philderbeast
Copy link
Collaborator Author

philderbeast commented Feb 1, 2025

I'd like to see this merged as-is, if possible. Mentioned GitHub workflow script changes, such as merging workflows, could be done separately afterwards, couldn't they?

Without the fix, any contributor that wants to install cabal-install built from source is going to be met with:

$ cabal install cabal-install:exe:cabal --dry-run
...
Error: [Cabal-6661]
filepath wildcard 'readme.md' does not match any files.

For that reason, I've added a high priority label.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 1, 2025

@geekosaur, @ulysses4ever: are you fine with removing your change requests and moving these considerations to future work? If you'd like to open a ticket with these, that may be a good idea, too.

@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Feb 1, 2025

I'm fine with removing mine. I think Brandon's request is more important, and that I'm not sure. But he can decide it, I guess.

@ulysses4ever ulysses4ever dismissed their stale review February 1, 2025 15:08

future work

@ulysses4ever
Copy link
Collaborator

It's really bad that this PR lumps together two absolutely unrelated things: the casing fix, which is indeed critical, and the new CI job, which is the main point of contention in our comments. I thing it'd be good to be principled here, @Mikolaj, and request the author to resubmit as two separate PRs. But I won't die over it.

@philderbeast
Copy link
Collaborator Author

philderbeast commented Feb 1, 2025

It's really bad that this PR lumps together two absolutely unrelated things

I did that because it was a cheap way to ensure this doesn't happen again. Happy to split if need be.

@geekosaur
Copy link
Collaborator

I should note that this still won't detect the README issue because of the docs exclusion, but as this is being pushed along and nobody apparently cares that it's not doing anything, I'm just going to get out of the way. (This is part of why I'm distancing myself from the project.)

@philderbeast
Copy link
Collaborator Author

nobody apparently cares that it's not doing anything

I do and have moved the Cabal-hooks.cabal fix to #10776.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Feb 1, 2025

At the risk of derailing even further, I'm surprised that the main CI pipeline does not go through cabal sdist or cabal install, but instead apparently (?) simply builds from the repo. Is there a good reason for this? Going through cabal sdist is the default practice as enshrined in Haskell CI, here is an example of boilerplate involved.

Adapting Haskell CI boilerplate could take quite a long time, and just firing cabal install cabal-install is the next best thing.

I don't quite understand why there is a carefully crafted list of file exclusions when build is not triggered - is saving CI resources really that critical? On the other hand, even if this job is not fired on the very first change to README.md, surely it will be triggered soon enough by other PRs.

(End of unsolicited advices ;)

@Mikolaj
Copy link
Member

Mikolaj commented Feb 1, 2025

@geekosaur, @ulysses4ever: if I'm not asking for too much, could you bring back your change requests now? :)

And, in general, let's discuss. I think what @Bodigrim says is very on point.

.github/workflows/check-install.yml Outdated Show resolved Hide resolved
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.

LGTM, thanks!

We should, perhaps, continue discussing issues raised by Bodigrim above. Maybe open a ticket?

@philderbeast philderbeast force-pushed the add/dry-run-cabal-install branch from 61a5238 to 6e9b141 Compare February 1, 2025 22:35
- Push to any branch
- Only check with the latest GHC version
- Shorten name of job
- Don't skip if run on master
- Don't ignore **/README.md files
- Ignore known test paths
- Don't ignore change logs
@philderbeast
Copy link
Collaborator Author

philderbeast commented Feb 1, 2025

Maybe open a ticket?

@ulysses4ever, I've opened "Mix include and exclude paths in workflows", #10777.

@ulysses4ever
Copy link
Collaborator

I've opened "Mix include and exclude paths in workflows"

That's not what I meant but thank you! I meant a ticket out of Bodigrim's comment: we might need a simple sdist workflow. Apparently, the existing one is very special.

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.

Casing of extra-source-files in Cabal-hooks
5 participants