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

Convert validate.sh to a Haskell script #10320

Merged
merged 12 commits into from
Oct 3, 2024

Conversation

9999years
Copy link
Collaborator

@9999years 9999years commented Sep 5, 2024

Closes #10317.

A Haskell script will be easier to maintain and expand than the existing Bash script.

This also adds a --pattern PATTERN option which lets you filter tests across all test suites.

I've kept the new script's behavior as close to the old script's behavior as possible, to reduce design discussion on this PR.

TODO

  • Add cabal-validate/README.md to make it obvious where to look for logic and how to interact with the program
  • Add Haddock comments to most definitions

Future work

  • It should be possible to override the default tasty args (currently --hide-successes)
  • --list-steps only lists the steps that are enabled with the given command-line options, not all possible steps; ./validate.sh --list-steps and ./validate.sh --list-steps --run-cli-suite give different results! (This is an existing bug in the old script.)
  • Perhaps "steps" should be divided into "suites"; for example, the lib-tests step runs five different test suites, making it take much longer than necessary to reproduce test failures.
  • Commands printed to users should be shell escaped. (Exiting bug in the old script.)
  • --help output could use some love; it wasn't very clear to me what various commands do.
  • --keep-going mode to ignore failures and continue executing tests/suites.
  • The output is really verbose, because the command lines are so long.

@ulysses4ever
Copy link
Collaborator

I wonder if it can be just a cabal script... But this is not crucial at the moment.

@philderbeast
Copy link
Collaborator

I hope I'm not jumping the gun with a pull request in draft but HLint is gagged1 and could help.

We suppress a lot of HLint suggestions at the root :-(

cabal/.hlint.yaml

Lines 1 to 95 in 3a6f73e

# Warnings currently triggered by your code
- ignore: {name: "Avoid NonEmpty.unzip"} # 1 hint
- ignore: {name: "Avoid lambda"} # 46 hints
- ignore: {name: "Avoid lambda using `infix`"} # 22 hints
- ignore: {name: "Eta reduce"} # 116 hints
- ignore: {name: "Evaluate"} # 10 hints
- ignore: {name: "Functor law"} # 10 hints
- ignore: {name: "Fuse concatMap/map"} # 3 hints
- ignore: {name: "Fuse foldr/map"} # 3 hints
- ignore: {name: "Fuse mapMaybe/map"} # 1 hint
- ignore: {name: "Fuse traverse_/fmap"} # 1 hint
- ignore: {name: "Fuse traverse_/map"} # 1 hint
- ignore: {name: "Hoist not"} # 16 hints
- ignore: {name: "Missing NOINLINE pragma"} # 1 hint
- ignore: {name: "Monoid law, left identity"} # 3 hints
- ignore: {name: "Monoid law, right identity"} # 3 hints
- ignore: {name: "Move filter"} # 4 hints
- ignore: {name: "Move guards forward"} # 4 hints
- ignore: {name: "Redundant $"} # 175 hints
- ignore: {name: "Redundant $!"} # 1 hint
- ignore: {name: "Redundant <$>"} # 16 hints
- ignore: {name: "Redundant =="} # 1 hint
- ignore: {name: "Redundant bracket"} # 232 hints
- ignore: {name: "Redundant fmap"} # 1 hint
- ignore: {name: "Redundant guard"} # 2 hints
- ignore: {name: "Redundant if"} # 3 hints
- ignore: {name: "Redundant lambda"} # 19 hints
- ignore: {name: "Redundant multi-way if"} # 1 hint
- ignore: {name: "Redundant return"} # 7 hints
- ignore: {name: "Replace case with fromMaybe"} # 5 hints
- ignore: {name: "Replace case with maybe"} # 10 hints
- ignore: {name: "Unused LANGUAGE pragma"} # 167 hints
- ignore: {name: "Use $>"} # 5 hints
- ignore: {name: "Use ++"} # 4 hints
- ignore: {name: "Use :"} # 25 hints
- ignore: {name: "Use <$"} # 2 hints
- ignore: {name: "Use <$>"} # 86 hints
- ignore: {name: "Use <&>"} # 14 hints
- ignore: {name: "Use <=<"} # 5 hints
- ignore: {name: "Use =<<"} # 7 hints
- ignore: {name: "Use =="} # 3 hints
- ignore: {name: "Use >=>"} # 3 hints
- ignore: {name: "Use ?~"} # 1 hint
- ignore: {name: "Use Down"} # 3 hints
- ignore: {name: "Use Just"} # 2 hints
- ignore: {name: "Use bimap"} # 7 hints
- ignore: {name: "Use camelCase"} # 96 hints
- ignore: {name: "Use catMaybes"} # 3 hints
- ignore: {name: "Use concatMap"} # 1 hint
- ignore: {name: "Use const"} # 36 hints
- ignore: {name: "Use elem"} # 2 hints
- ignore: {name: "Use fewer imports"} # 19 hints
- ignore: {name: "Use first"} # 4 hints
- ignore: {name: "Use fmap"} # 24 hints
- ignore: {name: "Use fold"} # 1 hint
- ignore: {name: "Use for"} # 1 hint
- ignore: {name: "Use forM_"} # 1 hint
- ignore: {name: "Use fromMaybe"} # 1 hint
- ignore: {name: "Use fromRight"} # 1 hint
- ignore: {name: "Use fst"} # 1 hint
- ignore: {name: "Use if"} # 4 hints
- ignore: {name: "Use infix"} # 20 hints
- ignore: {name: "Use isAsciiLower"} # 2 hints
- ignore: {name: "Use isAsciiUpper"} # 2 hints
- ignore: {name: "Use isDigit"} # 2 hints
- ignore: {name: "Use isJust"} # 1 hint
- ignore: {name: "Use isNothing"} # 1 hint
- ignore: {name: "Use lambda-case"} # 47 hints
- ignore: {name: "Use lefts"} # 1 hint
- ignore: {name: "Use list comprehension"} # 16 hints
- ignore: {name: "Use list literal"} # 3 hints
- ignore: {name: "Use list literal pattern"} # 11 hints
- ignore: {name: "Use map once"} # 7 hints
- ignore: {name: "Use map with tuple-section"} # 3 hints
- ignore: {name: "Use mapMaybe"} # 13 hints
- ignore: {name: "Use max"} # 1 hint
- ignore: {name: "Use maybe"} # 8 hints
- ignore: {name: "Use minimumBy"} # 1 hint
- ignore: {name: "Use newtype instead of data"} # 26 hints
- ignore: {name: "Use notElem"} # 8 hints
- ignore: {name: "Use null"} # 2 hints
- ignore: {name: "Use record patterns"} # 16 hints
- ignore: {name: "Use replicateM"} # 1 hint
- ignore: {name: "Use replicateM_"} # 2 hints
- ignore: {name: "Use rights"} # 2 hints
- ignore: {name: "Use second"} # 7 hints
- ignore: {name: "Use section"} # 17 hints
- ignore: {name: "Use traverse"} # 1 hint
- ignore: {name: "Use tuple-section"} # 28 hints
- ignore: {name: "Use typeRep"} # 2 hints
- ignore: {name: "Use unless"} # 20 hints
- ignore: {name: "Use unwords"} # 8 hints
- ignore: {name: "Use void"} # 22 hints
- ignore: {name: "Use when"} # 1 hint
- ignore: {name: "Use uncurry"} # 1 hint

Side-stepping that configuration that ignores 94 suggestions lets HLint come forward with a few suggestions:

$ sed 's/^- ignore/# &/' .hlint.yaml > .hlint-open.yaml \
  && hlint cabal-validate/main/Main.hs --hint=.hlint-open.yaml
cabal-validate/main/Main.hs:125:26-52: Suggestion: Use :
Found:
  ["build"] ++ cabalArgs opts
Perhaps:
  "build" : cabalArgs opts

cabal-validate/main/Main.hs:128:25-54: Suggestion: Use :
Found:
  ["list-bin"] ++ cabalArgs opts
Perhaps:
  "list-bin" : cabalArgs opts

cabal-validate/main/Main.hs:213:12-60: Warning: Use maybe
Found:
  fromMaybe getNumCapabilities (pure <$> opts . jobs)
Perhaps:
  maybe getNumCapabilities pure (opts . jobs)

cabal-validate/main/Main.hs:303:12-43: Suggestion: Redundant bracket
Found:
  (option (maybeReader parseStep))
    (long "step"
       <>
         help "Run only a specific step (can be specified multiple times)")
Perhaps:
  option
    (maybeReader parseStep)
    (long "step"
       <>
         help "Run only a specific step (can be specified multiple times)")

cabal-validate/main/Main.hs:(373,1)-(374,66): Warning: Eta reduce
Found:
  boolOption defaultValue trueName modifiers
    = boolOption' defaultValue trueName ("no-" <> trueName) modifiers
Perhaps:
  boolOption defaultValue trueName
    = boolOption' defaultValue trueName ("no-" <> trueName)

cabal-validate/main/Main.hs:472:16-42: Suggestion: Use tuple-section
Found:
  \ exitCode -> (exitCode, "")
Perhaps:
  (, "")
Note: may require `{-# LANGUAGE TupleSections #-}` adding to the top of the file

6 hints

Footnotes

  1. The gag order is RFC: Applying HLint's #9110. We want to protect existing code from churn but this is new code.

@philderbeast
Copy link
Collaborator

I wonder if it can be just a cabal script... But this is not crucial at the moment.

@ulysses4ever, it could be but unfortunately it can't also be a script. It can't be both an installable executable and a script (but I wish it could), reported as #10324.

@9999years 9999years force-pushed the validate-haskell branch 2 times, most recently from e570576 to 29a9aa9 Compare September 6, 2024 19:47
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
@9999years 9999years force-pushed the validate-haskell branch 2 times, most recently from 94598c5 to 8155d24 Compare September 6, 2024 23:18
@ulysses4ever
Copy link
Collaborator

@9999years requested review from philderbeast and ulysses4ever

It's on my radar but I'll only get to it the next week I'm afraid

cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
cabal-validate/main/Main.hs Outdated Show resolved Hide resolved
validate.sh Show resolved Hide resolved
@9999years 9999years force-pushed the validate-haskell branch 3 times, most recently from 7331115 to c083b22 Compare September 7, 2024 01:02
@9999years 9999years marked this pull request as ready for review September 7, 2024 01:05
@9999years
Copy link
Collaborator Author

On that note, the script depends on terminal-size, which depends on ghc-prim, which "is an internal package, only for the use of GHC developers. GHC users should not use it!" Perhaps that dependency could be dropped, at the very least?

terminal-size only depends on ghc-prim when built with GHC between 7.2 and 7.6, which I suspect we do not have to worry about:

https://github.com/biegunka/terminal-size/blob/8c5765027ae6194191d6da11d69a49c37a78a73a/terminal-size.cabal#L32-L34

@9999years
Copy link
Collaborator Author

It seems that the validate script is built with the same version of GHC as the one which is being tested.

It would be better in my opinion if the validate script was decoupled from the version of GHC being tested. (This was achieved when it was a bash script)

Nothing requires this to be the case; we could easily do cabal build --with-ghc ... cabal-validate in CI or locally. The 3-line validate.sh entrypoint is simply provided as a convenience.

@9999years
Copy link
Collaborator Author

It would be bad to get into a situation where adding testing for a new compiler is blocked because one of the dependencies of the validate script hasn't been updated yet.

The new dependencies are ansi-terminal, terminal-size, and typed-process. All of the others are either bundled with GHC or used by other components in this repository already.

ansi-terminal could be removed pretty easily; we only use it to write ANSI escape sequences.

terminal-size only depends on base >=4 && <5 (except on Windows, where it depends on process and Win32, both bundled with GHC), and it's only 120 lines of code anyways, so I think that's fine.

typed-process pulls in unliftio-core (all other dependencies are bundled with GHC or included in other Cabal components already), which also seems pretty reasonable. We could remove this, but the ergonomics and maintainability of cabal-validate would suffer (IMO) and we'd have to devote a fair amount of effort to reproducing its error messages.

@9999years
Copy link
Collaborator Author

My only big concern is how many times it will have to be built from scratch and whether it will bring a noticeable slowdown in CI.

It seems like the print-config step, which builds cabal-validate, finishes in about 17 seconds in CI. (Judging by local runs, about ~1 second or less of that is running the cabal-validate script.) That seems like a reasonable cost to me, and from the fact that none of the dependencies are built in that step, it seems like the dependencies are cached as well, so we can expect to not pay that cost unless we're changing validate.sh.

@chreekat
Copy link
Collaborator

chreekat commented Oct 1, 2024

terminal-size only depends on ghc-prim when built with GHC between 7.2 and 7.6, which I suspect we do not have to worry about:

Oh, thanks for pointing that out! I only got as far as the package home page on Hackage, which lists the dependencies without mentioning the constraints.

@9999years
Copy link
Collaborator Author

@ulysses4ever @mpickering I'd appreciate if you could take a look at my responses and see if it looks good or if there's anything else we should address before merge. Thanks!

@ulysses4ever
Copy link
Collaborator

Yes, sorry, I meant to post it yesterday: I don't have any other comments and think it can be handed over to the merge bot.

@9999years 9999years added the merge me Tell Mergify Bot to merge label Oct 1, 2024
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Oct 1, 2024
@geekosaur geekosaur added merge+no rebase and removed merge me Tell Mergify Bot to merge labels Oct 3, 2024
@geekosaur
Copy link
Collaborator

Switched the merge label: mergify just balked at queueing it.

@mergify mergify bot merged commit 24a128e into haskell:master Oct 3, 2024
49 of 51 checks passed
@geekosaur geekosaur mentioned this pull request Oct 3, 2024
2 tasks
@9999years 9999years deleted the validate-haskell branch October 4, 2024 00:24
@geekosaur
Copy link
Collaborator

@mergify backport 3.12

Copy link
Contributor

mergify bot commented Oct 29, 2024

backport 3.12

✅ Backports have been created

mergify bot added a commit that referenced this pull request Oct 30, 2024
* Convert `validate.sh` to `cabal-validate`

Closes #10317.

A Haskell script will be easier to maintain and expand than the existing
Bash script.

This also adds a `--pattern PATTERN` option which lets you filter tests
across all test suites.

(cherry picked from commit 582a5c7)

# Conflicts:
#	validate.sh

* Split `cabal-validate` into modules

This disentangles the utility boilerplate from the validation logic,
making the `Main.hs` module much easier to read and modify.

(cherry picked from commit 43a3975)

* cabal-validate: Add Haddock documentation + README

(cherry picked from commit e257591)

* Remove `ansi-terminal` dependency

(cherry picked from commit 96d6ad5)

* Use `unlines` in `printConfig`

(cherry picked from commit 9f5d90f)

* optsParser -> rawOptsParser

(cherry picked from commit a10a2a3)

* withTiming: take `startTime` explicitly

(cherry picked from commit 37cfe85)

* Add `cabal-validate` to `format` job

(cherry picked from commit 92613f0)

* Build test suites explicitly

This seems to fix an error where `long-tests` isn't built?

(cherry picked from commit d208282)

* fixup! Remove `ansi-terminal` dependency

(cherry picked from commit bae200a)

* fixup! Build test suites explicitly

(cherry picked from commit 1900d5e)

* fixup! fixup! Build test suites explicitly

(cherry picked from commit 30f0faa)

* fix pointless validate conflict since it's going away

* 3.12 doesn't have Cabal-hooks

---------

Co-authored-by: Rebecca Turner <[email protected]>
Co-authored-by: brandon s allbery kf8nh <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge+no rebase ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite validate.sh in Haskell
8 participants