ghc-options appears to remove duplicates

[hvr]

Summary. Cabal incorrectly deduplicates ghc-options which it passes to GHC.

Steps to reproduce. Create a Cabal package, and put ghc-options: -trust base -trust containers in it. Attempt to build it and GHC will fail with something like:

target `base' is not a module name or a source file

How to fix. The fix is pretty simple: you will need to disable deduplication of command line arguments in ghc-options. This behavior was introduced in ba4a6d5 so you will need partially undo this patch.

The crux of the matter is the changes to the field types in Cabal/Distribution/Simple/Program/GHC.hs; by changing a field from [String] to NubListR String, we change the behavior of mappend (which is how we combine flags together) from non-deduplicating to deduplicating.

Which fields should we revert? At a minimum, ghcOptExtra and ghcOptExtraDefault, but there may be others. You will need to determine what these fields mean, and see if deduplicating is a good idea or not.

You will need to add a test. Read the README in https://github.com/haskell/cabal/tree/master/cabal-testsuite for guidance. The test for this ticket should be pretty standard; you'll be able to find plenty of similar examples.

What you will learn. You'll learn about how Cabal computes the options it eventually passes to GHC during a build. You'll get familiar with the build environment and writing tests for Cabal.

---

Old summary. I just ran into a problem where where cabal new-build messes up

ghc-options:        -trust base -trust dependent-sum

By not passing both -trust tokens to GHC (to be more specific, only the last occurrence of -trust reaches GHC's CLI, i.e. base -trust dependent-sum is passed to GHC), which then causes GHC to fail with the following cryptic message:

target `base' is not a module name or a source file

[DanielG]

Looks like ghcOptExtra :: NubListR String since 1.22, ba4a6d5 seems to have introduced that.

[hvr]

@DanielG thanks for investigating! This looks quite nasty... I'm surprised it didn't cause more problems already...

[23Skidoo]

Can this be worked around with ghc-options: "-trust base" "-trust foo"?

[hvr]

@23Skidoo I think that works... but unfortunately it doesn't help with already released .cabal files on Hackage where the revision-validation would refuse such changes currently... :-)

Do we actually agree that this a bug that needs fixing? :-)

[23Skidoo]

Well, NubList-ification was done for a reason. Do you suggest simply reverting ba4a6d5?

[hvr]

@23Skidoo well, while I appreciate the reasoning behind why it was added (i.e. #2110), I don't think it's the right/principled thing to blindly de-duplicate flags w/o understanding their semantic/grammar/syntax (in the future, some flags may actually become ordering-sensitive (or just think of +RTS/-RTS as an extreme example), so dropping occurences would result in a semantic change). We have actual evidence this broke existing .cabal files on Hackage. And I don't think it's obvious to users that flags get automatically deduplicated.

So I think we should at best de-duplicate flags whose structure we are very confident to understand (e.g. I think we could safely whitelist -X*/-W*/-f(no-)warn-* flags and a couple of other popular flags for deduplication), i.e. use a pessimistic approach. (And speaking of pessimistic approach, I still think cabal should warn when it encounters a new major GHC version it doesn't know yet - we already had such cases in the past where a new GHC version turned out to be incompatible with existing cabal versions)

[23Skidoo]

@hvr Maybe instead of reverting that patch we can just treat flags followed by positional arguments (e.g. -foo bar) as single entities for the purposes of deduplication?

(And speaking of pessimistic approach, I still think cabal should warn when it encounters a new major GHC version it doesn't know yet - we already had such cases in the past where a new GHC version turned out to be incompatible with existing cabal versions)

+1.

[hvr]

That sounds like a good heuristic that would definitely help with the specific issue that gave rise to #4449 (so it would definitely be an improvement over the current situation).

However, it doesn't help with CLI flags that have cumulative effect, or that don't follow Last-monoid (ok, not really, but you know what I mean :-) ) semantics, or groupings of flags such as +RTS/-RTS

[ezyang]

I read the old ticket. The nub listification was done for a specific reason, but that reason ONLY applies to ghcOptExtensionMap, and NOT the other fields (which were also nubified.) There is no reason to nub ghcOptions, and we should look over the other fields and make sure they haven't been inappropriately nubbed.