pretty-print run targets on failure

[mgsium]

Resolves #8189; run targets are pretty-printed when failing due to multiple targets. Duplicates are also removed.

i.e.

$ cabal run all -- --help
Resolving dependencies...
Error: cabal: The run command is for running a single executable at once. The
target 'all' refers to all the packages in the project which includes
- executables: cabal, cabal-tests, hackage-benchmark and setup
- test-suites:
| * cabal-benchmarks
| * check-tests
| * custom-setup-tests
| * hackage-tests
| * integration-tests2
| * long-tests
| * mem-use-tests
| * no-thunks-test
| * parser-tests
| * rpmvercmp
| * unit-tests

---

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary. (N/A)

[Mikolaj]

Splendid. And you got the right CI breakage: on golden test of the old kind of output. [edit: which means you probably don't need to write separate tests]

[ulysses4ever]

I’m worried about introduction of partial field accessors. The proper solution would be to factor out the other four fields into a separate data type (NonLibComponentName or some such), which could have a total accessor. Didn’t check how much churn that would involve.

[ulysses4ever]

If throw in some pattern synonyms on the top, the churn could actually be none.

[mgsium]

Perhaps we can forego the field accessors entirely, given a function like

extract :: ComponentName -> Either LibraryName UnqualComponentName
extract (CLibName   s) = Left s
extract (CFLibName  s) = Right s
extract (CExeName   s) = Right s
extract (CTestName  s) = Right s
extract (CBenchName s) = Right s

that way we can avoid redefining types while keeping componentNameRaw and componentNameString simple.

[andreasabel]

I like @ulysses4ever 's proposal. One can define pattern synonyms for CFLibName etc. so that the change to the codebase remains small.

[mgsium]

Could either of you show me what you have in mind? Perhaps my lack of understanding but I can't see the change to the codebase being smaller.

[ulysses4ever]

Either-based idea is not bad too, but here's an example of what I meant (can compile and run to get "x")

{-# LANGUAGE PatternSynonyms #-}

type LibraryName = String
type UnqualComponentName = String

data NotLibComponentName
        = CNLFLibName  UnqualComponentName
        | CNLExeName   UnqualComponentName
        | CNLTestName  UnqualComponentName
        | CNLBenchName UnqualComponentName
data ComponentName
        = CLibName   LibraryName
        | CNotLibName NotLibComponentName
pattern CFLibName n = CNotLibName (CNLFLibName n)
// ... 3 more of these

main = print n
  where
    c = CFLibName "x" -- using bidirectional pattern to construct
    (CFLibName n) = c -- using bidirectional pattern to destruct

[ulysses4ever]

(And then change NotLibComponentName to have a field accessor, and possibly some convenience function for ComponentName)

[Mikolaj]

Something got stuck after recent CI improvements, so let me restart CI (and rebase just in case, @mgsium, I hope you don't mind?).

[Mikolaj]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[Mikolaj]

LGTM (but @ulysses4ever has a point)

[mgsium]

Looks like {-# COMPLETE ... #-} is giving an unrecognized pragma warning on ghc version 8.0.2. Afaict from the docs, the COMPLETE pragma was introduced sometime after 8.0.2.

Suggestions as to how to proceed?

[Mikolaj]

GHC 8.0.2 has just fallen out of our support window, but I struggle to comprehend the consequences. Probably that means we no longer want to extend our custom prelude nor add CPP for the sake of 8.0.2. If you can make it work, fine, if not, we drop GHC 8.0.2 from CI.

But IANAL, so let's wait until somebody confirms my understanding. :)

[ulysses4ever]

It’s a warning, right? Maybe just leave it be?

[ulysses4ever]

Oh, I missed -Werror (I was sure that I saw warnings in cabal codebase, but I guess not?)

Another idea is to stitch {-# OPTIONS_GHC -Wno-unrecognised-pragmas -Wno-incomplete-patterns #-} in the file. Not sure how good of an idea it is.

[ulysses4ever]

@mgsium do you have any trouble with finishing it off? Please, let us know. I'm looking forward for this change!

[mgsium]

Apologies for the delay, I'll get this done

[mgsium]

On second thoughts, do we need -Wno-unrecognised-pragmas? It fails regardless on 8.0.2 and shouldn't give the warning on later versions.

Edit: Ignore that, I lost track of the reason for the change. Looking at the most recent CI, we'll also need {-# OPTIONS_GHC -Wno-incomplete-patterns #-} in any file where we use the patterns. Is it worth doing this to support 8.0.2 or should it be dropped?

[Mikolaj]

Looking at the most recent CI, we'll also need {-# OPTIONS_GHC -Wno-incomplete-patterns #-} in any file where we use the patterns. Is it worth doing this to support 8.0.2 or should it be dropped?

Please feel free to drop support for 8.0.2 in all parts of CI where you need to (including all of CI). This PR is a good place for that, because it contains the reason for removing 8.0.2 and the discussion of it.

[ulysses4ever]

https://github.com/haskell/cabal/pull/8234/files#diff-1247d017c37ce35fb4af8dbb4a07fe846918264cd555bb37bb4fc57d8d7fa8f0R4

Do we still need the options?

[ulysses4ever]

Another thing: if you could i-rebase in the way that there are two commits: removing 8.0.2 support and implementing the feature, that may be a cleaner history overall (as opposed to squashing all of that in one commit).

[mgsium]

Do we still need the options?

Nope we shouldn't, I'll remove and test before rebasing.

[ulysses4ever]

This looks good to go. Who should apply the label? If the author doesn't hold the appropriate role for that, should we fix it too?

[Mikolaj]

Oh, yes, let's get this merged. I forgot how the new system of permissions works. @mgsium: I've given you Triage rights to the repo. Are you now able to set the "merge_me" label? That should start the merge process, which ends in 2 days if there are no more changes to the PR.

[Mikolaj]

@mgsium: hah, you've got yelled at due to whitespace. See https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#whitespace-conventions

[Mikolaj]

Perfect. Now mergify will sit on it for 2 days waiting for further comments, if any.

[ulysses4ever]

Should this be backported?

[fgaz]

It isn't a bugfix so there's no need to backport it

[ulysses4ever]

Fair enough, thanks!

[kokobd]

@mergify backport 3.8

[mergify]

backport 3.8

✅ Backports have been created

• #8471 pretty-print run targets on failure (backport #8234) has been created for branch 3.8