Pass *-options to GHC when compiling ordinary Haskell sources#10969
Pass *-options to GHC when compiling ordinary Haskell sources#10969zlonast wants to merge 3 commits into
Conversation
1bd501e to
c3e68be
Compare
|
I probably need to think about tests, maybe something started working besides |
|
Ok, got it, we just need to separate the flags |
a439429 to
1b8e5bb
Compare
|
@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
left a comment
There was a problem hiding this comment.
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).
geekosaur
left a comment
There was a problem hiding this comment.
Same concerns as @ulysses4ever raised.
Yes, it looks like so. I'd say the problem is rather that IMHO, it's wrong that there are separate EDIT: There might be situations where EDIT: So AFAICT, this is not the correct fix, not what I'd imagine seeing. |
0b74992 to
37417aa
Compare
geekosaur
left a comment
There was a problem hiding this comment.
There are a lot of unresolved comments still, that will need to be resolved to merge.
|
FWIW I'm not worried about rebases, except insofar as they may help with development; since PRs are being merged all the time, it's usually not worth the resulting constant rebases when Mergify will do it itself. |
|
@zlonast please, attend to the inline comments when you get a chance |
|
I've closed a few threads because I don't think they're that important, but feel free to reopen them.
|
|
Perhaps we should add more tests, but I don't have the energy for that right now.
|
|
There are some linking issues with older versions. I don't know what to say about that.
|
a7f3e55 to
a8f1b13
Compare
|
And also we have problems with normalizer. #11187 (comment)
|
|
I see the diff is quite large and there is scope creep (possible interactions, hypothetical scenarios, related buglets), so I'm worried for the kind PR author. Could we all try to help somehow? Maybe let's try to answer some questions @zlonast poses? Could we somehow split responsibility for some subtasks or open new tickets for them and leave them undone for now? It would be really great to merge this PR while there still the momentum. |
|
The only unresolved thread that I was able to find in this (rather lengthy) discussion is this: #10969 (comment) started by @mpickering, so, maybe Matt could try to push it forward (or backward :-)) |
|
Sadly, @mpickering may not be able to contribute to this PR any more. @zlonast: how can we help to get it through? |
|
@Mikolaj Hi! As the author, I think it works and we can be merge it 😄 But perhaps you want to pay attention to this:
|
a8f1b13 to
6b7957c
Compare
|
@zlonast thank you for your work here and for the summary in partiular. Quetsions:
This looks rather serious. Can you elaborate with example scenarios? E.g. will I not be able to build any package with c-sources after this change if I need to use 9.4? FTR, we are supposed to maintain ccompatibility with any GHC released last 5 years. GHC 9.4.8 was released 10 November 2023, so there's still 2.5 years of it staying in this support window.
Do you think you could add some? |
6b7957c to
e522b12
Compare
|
This must happen before this pull request is merged.
issue #11712 pr fix #11713
tests #11711 |
e522b12 to
afa91a2
Compare
|
|
afa91a2 to
858d618
Compare
858d618 to
7de85b7
Compare
|
I found the problem and left a link in this #10969 (comment). I've only left two things in that pr.
I suggest checking the compilation of the entire |
|
Deprecation notice: This pull request comes from a fork and was rebased using |
✅ Branch has been successfully rebased |
Refactor componentGhcOptions for pass *-options to all invoking GHC. During the refactoring process we needed to add componentGhcOptions to all Haskell sources. It was also worth add linkGhcOptions to linkLibrary same as componentGhcOptions to linkExecutable, linkFLib. Add test for PackageTests/FFI/ForeignOptsCapi to pass cc-options flags to *.h. Fixes haskell#9801 haskell#4435 Co-authored-by: Mikolaj Konarski <mikolaj.konarski@gmail.com>
Co-authored-by: Yuriy Syrovetskiy <git-orbi@cblp.su>
|
To summarize the current state of this PR:
Regarding the tests, the two added test cases directly address what was requested in the linked issues (#9801, #4435):
@Mikolaj @ulysses4ever I believe this is now ready. |
| ( \verbosity lbi bi clbi odir filename -> | ||
| (Internal.sourcesGhcOptions verbosity lbi bi clbi odir filename) | ||
| { -- C++ compiler options: GHC >= 8.10 requires -optcxx, older requires -optc | ||
| -- we want to be able to support cxx-options and cc-options separately | ||
| -- https://gitlab.haskell.org/ghc/ghc/-/issues/16477 | ||
| -- see example in cabal-testsuite/PackageTests/FFI/ForeignOptsCxx | ||
| ghcOptCcOptions = | ||
| Internal.ghcOptionsSince | ||
| (mkVersion [8, 10]) | ||
| (compiler lbi) | ||
| (Internal.optimizationCFlags lbi ++ ccOptions bi) | ||
| , -- Use -pgmc to ensure that Cabal always passes cc-options, ld-options to GHC (#4435, #9801) | ||
| -- We can only do this on GHC >= 9.4, as we need https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6949 | ||
| -- Without that GHC MR, this change would cause GHC to never pass -no-pie when linking, | ||
| -- which can cause breakage depending on the C toolchain use. Otherwise, | ||
| -- we pass the flag only to source files #11712 | ||
| -- see example in cabal-testsuite/PackageTests/FFI/ForeignOptsPgmc | ||
| ghcOptCcProgram = maybeToFlag $ programPath <$> lookupProgram gccProgram (withPrograms lbi) | ||
| } | ||
| ) |
There was a problem hiding this comment.
Is this an absolute duplicate of the hunk above? Can we factor it out? At the very least, I don't think we should duplicate comments that long. You could create a "Note" and reference it. (We don't use notes as extensively as GHC, but there are some cases where we do.)
There was a problem hiding this comment.
Then I propose what is in the last commit
| (Internal.sourcesGhcOptions (verbosityLevel verbosity) lbi bnfo clbi odir filename) | ||
| { -- C++ compiler options: GHC >= 8.10 requires -optcxx, older requires -optc | ||
| -- we want to be able to support cxx-options and cc-options separately | ||
| -- https://gitlab.haskell.org/ghc/ghc/-/issues/16477 | ||
| -- see example in cabal-testsuite/PackageTests/FFI/ForeignOptsCxx | ||
| ghcOptCcOptions = | ||
| Internal.ghcOptionsSince | ||
| (mkVersion [8, 10]) | ||
| (compiler lbi) | ||
| (Internal.optimizationCFlags lbi ++ ccOptions bnfo) | ||
| , -- Use -pgmc to ensure that Cabal always passes cc-options, ld-options to GHC (#4435, #9801) | ||
| -- We can only do this on GHC >= 9.4, as we need https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6949 | ||
| -- Without that GHC MR, this change would cause GHC to never pass -no-pie when linking, | ||
| -- which can cause breakage depending on the C toolchain use. Otherwise, | ||
| -- we pass the flag only to source files #11712 | ||
| -- see example in cabal-testsuite/PackageTests/FFI/ForeignOptsPgmc | ||
| ghcOptCcProgram = maybeToFlag $ programPath <$> lookupProgram gccProgram (withPrograms lbi) | ||
| } |
| (Internal.sourcesGhcOptions (verbosityLevel verbosity) lbi bnfo clbi tmpDir filename) | ||
| { -- C++ compiler options: GHC >= 8.10 requires -optcxx, older requires -optc | ||
| -- we want to be able to support cxx-options and cc-options separately | ||
| -- https://gitlab.haskell.org/ghc/ghc/-/issues/16477 | ||
| -- see example in cabal-testsuite/PackageTests/FFI/ForeignOptsC | ||
| ghcOptCxxOptions = | ||
| Internal.ghcOptionsSince | ||
| (mkVersion [8, 10]) | ||
| (compiler lbi) | ||
| (Internal.optimizationCFlags lbi ++ cxxOptions bnfo) | ||
| , -- Use -pgmc to ensure that Cabal always passes cc-options, ld-options to GHC (#4435, #9801) | ||
| -- We can only do this on GHC >= 9.4, as we need https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6949 | ||
| -- Without that GHC MR, this change would cause GHC to never pass -no-pie when linking, | ||
| -- which can cause breakage depending on the C toolchain use. Otherwise, | ||
| -- we pass the flag only to source files #11712 | ||
| -- see example in cabal-testsuite/PackageTests/FFI/ForeignOptsPgmc | ||
| ghcOptCcProgram = maybeToFlag $ programPath <$> lookupProgram gccProgram (withPrograms lbi) | ||
| } |
| Before `ghc` 8.10, `cc-options` and `cxx-options` were passed via one flag `-optc`, | ||
| then in 8.10 they started to split into `-optc` and `-optcxx`, cabal picked up | ||
| this change and started to give flags separately not only for new versions, | ||
| but also for old ones. That is, now for 8.8 we send `-optc` flags separately for | ||
| different sources. |
There was a problem hiding this comment.
| Before `ghc` 8.10, `cc-options` and `cxx-options` were passed via one flag `-optc`, | |
| then in 8.10 they started to split into `-optc` and `-optcxx`, cabal picked up | |
| this change and started to give flags separately not only for new versions, | |
| but also for old ones. That is, now for 8.8 we send `-optc` flags separately for | |
| different sources. | |
| Before GHC 8.10, Cabal passed its `cc-options` and `cxx-options` via one GHC flag `-optc`. | |
| GHC 8.10 introduced `-optcxx`, and Cabal picked up this change: `cc-options` and `cxx-options` are now mapped to `-optc` and `-optcxx` respectively. | |
| But Cabal also changed its behaviour for older GHCs (8.8 and older): Cabal sends `cc-options` and `cxx-options` through separate instances of `-optc`. |
Does this sound right? I think it's a bit more clear.
There was a problem hiding this comment.
Actually, changelog may be not the best place to put historical notes. Can you move it to commit message or/and comments?
For Changelog, it would be relevant to comment on compatibility with GHCs pre 9.4. Does this patch change anything for thise? I still don't quite understand those comments about 9.4 in the sources.
There was a problem hiding this comment.
Yes, you're right, I just redid the comment.
-- We pass -pgmc to ensure that GHC respects cc-options and ld-options (#4435, #9801).
-- However, we deliberately restrict this flag ONLY to C and C++ source files.
-- We do not pass it globally (e.g., during linking or ordinary Haskell compilation)
-- for two reasons:
-- 1. Status quo: This preserves Cabal's historical behavior.
-- 2. GHC < 9.4 bug (https://gitlab.haskell.org/ghc/ghc/-/issues/15319): Before GHC 9.4,
-- passing a custom C compiler via -pgmc caused GHC to automatically disable the
-- -no-pie flag (which is critical during linking on auto-hardened toolchains).
-- Passing -pgmc globally would therefore break linking on older GHCs.
-- https://gitlab.haskell.org/ghc/ghc/-/merge_requests/6949 fixed this by moving
-- the -no-pie check to -pgml, but scoping -pgmc exclusively to C/C++ compilation
-- protects older GHC versions from this link-time breakage.Before GHC 8.10, Cabal passed both `cc-options` and `cxx-options` via a single GHC flag, `-optc`. When GHC 8.10 introduced `-optcxx`, Cabal picked up this change: `cc-options` and `cxx-options` are now mapped to `-optc` and `-optcxx`, respectively. However, Cabal also changed its behavior for older GHC versions (8.8 and below): it now sends `cc-options` and `cxx-options` through separate instances of `-optc`. Co-authored-by: Artem Pelenitsyn <a.pelenitsyn@gmail.com>
Will close issues #9801 #4435
Main idea #9801 (comment)
cc-options,cxx-options,jspp-options,asm-options,cmm-options,ld-options,cpp-options,should be always passed when invoking GHC, similarly as
ghc-optionsshould be always used wheninvoking
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.
cc-optionsandcxx-optionsstill need to be separated (C/C++) for versions below 8.10.https://gitlab.haskell.org/ghc/ghc/-/issues/16477
Template Α: This PR modifies behaviour or interface
Include the following checklist in your PR:
significance: significantin the changelog file.Depends-on: #11711
Depends-on: #11713