Cabal doesn't pass *-options to GHC when compiling ordinary Haskell sources

[zlonast]

Will close issues #9801 #4435
Main idea #9801 (comment)

*-options should be always passed when invoking GHC, similarly as ghc-options should be always used when invoking ghc - regardless of what is the intention of a particular GHC-call. GHC might use or not use the options, Cabal cannot know and should not guess.

---

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
  • Is the change significant? If so, remember to add significance: significant in the changelog file.
• The documentation has been updated, if necessary.
• Manual QA notes have been included.
• Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

[zlonast]

I probably need to think about tests, maybe something started working besides CApiFFI 🤔

[zlonast]

Ok, this only works for things newer than 8.8
gcc: fatal error: cannot specify ‘-o’ with ‘-c’, ‘-S’ or ‘-E’ with multiple files

Ok, got it, we just need to separate the flags -optc and -optcxx
Pass -optcxx for GHC >= 8.10 #7072

[zlonast]

@ulysses4ever @phadej It's not very easy to add flags to sources files due to backward compatibility, so for now I suggest considering the pull request as is ¯\_(ツ)_/¯ (Anyway, this solved the problem with macros in header files)

[ulysses4ever]

Great start, thank you! Below are some inline questions.

One general question: did you check that this doesn't lead to duplicated options when cabal calls GHC? I assume other *-options are passed today sometimes. So, sometimes we'll get those options passed as today and through your change too. Is that right? It seems undesirable.

It's not very easy to add flags to sources files due to backward compatibility

Can you be more specific? Do you mean it's hard to test on older GHCs? This shouldn't be a problem because the tests can run for specific versions of GHC (e.g. starting from 8.10).

[ulysses4ever]

Can you specify exactly which *-options are meant currently? (ASM, LD, etc...) And explain what's the difference with how there handled now: I assume they're passed only under specific circumstances now. Which ones?

[zlonast]

They were only partially in componentGhcOptions, after my changes, some cross-interaction became possible. This is currently only available for .c and .h or .cpp/.cxx/.c++ and .h. I think so.

[ulysses4ever]

But the patch does something to other *-options too. Is it possible to test this change w.r.t. other *-options?

[zlonast]

I need to think about it, but I don't know yet. Maybe I need to read other issues to see if anything worked.

[geekosaur]

Same concerns as @ulysses4ever raised.

[phadej]

did you check that this doesn't lead to duplicated options when cabal calls GHC?

Yes, it looks like so.

I'd say the problem is rather that GhcOptions is not used uniformly. There is e.g. componentCcGhcOptions which does set ghcOptCcOptions. Why these don't show up when compiling files CApiFFI headers?

IMHO, it's wrong that there are separate componentAsmGhcOptions, componentJsGhcOptions, componentGhcOptions. There should be just one componentGhcOptions with all the options, and then it's up to GHC which ones are relevant, and which aren't.

EDIT: There might be situations where ghcOptMode or something like that is different, and input files "obviously". But the bulk of options should be the same. Starting with verbosity. I'd imagine there is only single line in the codebase which does ghcOptVerbosity = toFlag (min verbosity normal). Currently there are six copies of that. Cannot be right. I suspect there are many subtle inconsistencies hiding because of that.

EDIT: So AFAICT, this is not the correct fix, not what I'd imagine seeing.

[zlonast]

Basically I just merged most of the options into one base one and added the separation in 8.10.
Now buildHaskellModules, buildAllExtraSources, linkOrLoadComponent have the same base buildOpts.

[zlonast]

I'm not sure about this change, even though it seems logical.

[zlonast]

Just add "-optc-O1","-optcxx-O1","-opta-O1"

[zlonast]

One general question: did you check that this doesn't lead to duplicated options when cabal calls GHC? I assume other *-options are passed today sometimes. So, sometimes we'll get those options passed as today and through your change too. Is that right? It seems undesirable.

@ulysses4ever Yes, you're right, now I've fixed it.

Can you be more specific? Do you mean it's hard to test on older GHCs? This shouldn't be a problem because the tests can run for specific versions of GHC (e.g. starting from 8.10).

@phadej correctly noted that compilerCompatVersion should have been added.

I'd say the problem is rather that GhcOptions is not used uniformly. There is e.g. componentCcGhcOptions which does set ghcOptCcOptions. Why these don't show up when compiling files CApiFFI headers?

Because they were only transferred to buildAllExtraSources files.

IMHO, it's wrong that there are separate componentAsmGhcOptions, componentJsGhcOptions, componentGhcOptions. There should be just one componentGhcOptions with all the options, and then it's up to GHC which ones are relevant, and which aren't.

I think you're right, now I did it this way.

EDIT: There might be situations where ghcOptMode or something like that is different, and input files "obviously". But the bulk of options should be the same.

Yes, I separated them into sourcesGhcOptions and componentGhcOptions.

Currently there are six copies of that. Cannot be right. I suspect there are many subtle inconsistencies hiding because of that.

Looks like I'll have to think about how to break the cabal :)

EDIT: So AFAICT, this is not the correct fix, not what I'd imagine seeing.

I hope you like the new version better.

[phadej]

why this (duplicated snippet) is not in sourcesGhcOptions . It has access to lbi?

[zlonast]

It does not completely match, there are different defaultGhcOptCcOptions and defaultGhcOptCxxOptions.
You are right, ghcOptCcOptions and ghcOptCxxOptions can be moved to sourcesGhcOptions.
On the other hand, ghcOptCcProgram should be left here, since not everywhere (it seems to me) there may be access to gccProgram.

[zlonast]

I didn't quite understand about the access of lbi...

[phadej]

here may be access to gccProgram.

That's a bug on its own:

• If --with-gcc is given, it should always be passed to GHC (so GHC always consistently uses the same GCC).

[phadej]

there are different defaultGhcOptCcOptions and defaultGhcOptCxxOptions

What are the differences? Why. I don't find that intuitive. If there is a reason, we should have a comment about that. But in that case we should work on removing that reason. I only see a downsides of using subtly different calls to GHC when compiling C or C++ files.

[zlonast]

if we do such refactoring in renderGhcOptions from

            , -- C++ compiler options: GHC >= 8.10 requires -optcxx, older requires -optc
          let cxxflag = case compilerCompatVersion GHC comp of
                Just v | v >= mkVersion [8, 10] -> "-optcxx"
                _ -> "-optc"
           in [cxxflag ++ opt | opt <- ghcOptCxxOptions opts]

to

        , ["-optcxx" ++ opt | opt <- ghcOptCxxOptions opts]

and we will also make options in accordance with their appearance

          -- C++ compiler options: GHC >= 8.10 requires -optcxx, older requires -optc
        , ghcOptCxxOptions = case compilerCompatVersion GHC (compiler lbi) of
            Just v
              | v >= mkVersion [8, 10] -> optimizationCFlags lbi ++ cxxOptions bi
              | otherwise -> []
            Nothing -> []
        , ghcOptCcOptions = case compilerCompatVersion GHC (compiler lbi) of
            Just v
              | v >= mkVersion [8, 10] -> optimizationCFlags lbi ++ ccOptions bi
              | otherwise -> optimizationCFlags lbi ++ cxxOptions bi ++ ccOptions bi
            Nothing -> []

we will fail on foreign-opts-cxx.cabal test as it is tested on versions older than 8.10

cabal-version:       2.2
name:                foreign-opts-cxx
version:             0.1
build-type:          Simple

executable foreign-opts-cxx-exe
  main-is:             Main.hs
  build-depends:       base
  default-language:    Haskell2010
  include-dirs:        cxxbits
  extra-libraries:     stdc++
  cxx-sources:         cxxbits/cxxlib.cpp
  -- The following options are shared in all ForeignOpts cases:
  cc-options:          -D__TESTOPT_C__=11
  cxx-options:         -D__TESTOPT_CXX__=22

#ifdef __TESTOPT_C__
#error "Got unexpected __TESTOPT_C__ from cc-options"
#endif

to avoid these subtleties we can skipUnlessGhcVersion ">= 8.10" ???

[phadej]

No. Am I explaining myself unclearly. Let's try again.

it looks like that

• You set C options and ghcOptCcProgram only in buildCSources
• You set C++ options and ghcOptCxxProgram only in buildCxxSources

That cannot be right. You should set these options always.

My comments weren't related to >=8.10 check.

[zlonast]

No, I always set after 8.10 and try to separate them in 8.8 for buildCSources and buildCxxSources, because otherwise the foreign-opts-cxx.cabal test will fail. In order for foreign-opts-cxx.cabal to pass on 8.8 they need to be separated or use skipUnlessGhcVersion ">= 8.10".

[phadej]

Ok, I see. The GHC-8.8 doesn't know about C++ compiler, so we have to deal with it separately, and in an ugly way.

Could you write a note, referencing the test. By looking at the code only, it looks wrong or at least suspicious.

[phadej]

In the retrospective, cabal-install could refused to support an accidental "but it did work before with older GHC, cc-options were passed to C compiler" when proper cxx-options were added. Those accidents just make code base very spagetti.