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

Support GHC 9.4 #8420

Merged
merged 7 commits into from
Sep 24, 2022
Merged

Support GHC 9.4 #8420

merged 7 commits into from
Sep 24, 2022

Conversation

ulysses4ever
Copy link
Collaborator

@ulysses4ever ulysses4ever commented Aug 22, 2022

Current status: slowly working through test failures... See discussion below.


Please also shortly describe how you tested your change. Bonus points for added tests!

@ulysses4ever
Copy link
Collaborator Author

Only the last commit in this PR is actually the contribution.

@ulysses4ever
Copy link
Collaborator Author

Windows is failing because cabal 3.8 is required there for GHC 9.4, but haskell/actions/setup still thinks that latest is 3.6?

@ulysses4ever
Copy link
Collaborator Author

Right... haskell/actions#113

Maybe it's about the time to move to pure ghcup workflow…

@ulysses4ever ulysses4ever force-pushed the adopt-ghc-9.4.1 branch 2 times, most recently from d6a5bbe to 0bee7c6 Compare August 23, 2022 01:03
@ulysses4ever
Copy link
Collaborator Author

Tests are not ready yet: opened haskell-hvr/windns#3

@ulysses4ever ulysses4ever force-pushed the adopt-ghc-9.4.1 branch 2 times, most recently from 2562b22 to 17059ca Compare August 23, 2022 22:39
@ulysses4ever
Copy link
Collaborator Author

Need to add TypeOperators extension to make it compile with 9.4. This was brought up earlier in #8268.

@ulysses4ever
Copy link
Collaborator Author

Hmm, an actual test failure… Backpack.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 24, 2022

Does it look like a GHC bug or a cabal bug that manifests only in the configuration we happened to set for GHC 9.4?

@Mikolaj
Copy link
Member

Mikolaj commented Aug 25, 2022

That's neither:

haddock: internal error: haddock:iface

Not very surprising. Could we mark it as expected fail for 9.4?

Edit: unless it doesn't fail locally, but I hope it's at least consistent...

@ulysses4ever
Copy link
Collaborator Author

@Mikolaj I'm sorry, I don't see "haddock: internal error" on the CI. When I click on Details for the 9.4.1 job, I see

UNEXPECTED FAIL: PackageTests/Backpack/Includes2/setup-per-component.test.hs PackageTests/Backpack/Includes2/setup-external.test.hs PackageTests/Backpack/Includes3/setup-external-ok.test.hs

and Ctrl+F doesn't help either. What am I missing?

@Mikolaj
Copy link
Member

Mikolaj commented Aug 25, 2022

Yes, C-f doesn't work (perhaps in a raw log it does). Perhaps scroll, in https://github.com/haskell/cabal/runs/7984223230?check_suite_focus=true, up to

Warning: The documentation for the following packages are not installed. No links will be generated to these packages: base-4.17.0.0, ghc-bignum-1.3, ghc-prim-0.9.0, Includes2-0.1.0.0
creating setup-per-component.dist/work/dist/doc/html/Includes2
Running: /usr/local/.ghcup/ghc/9.4.1/bin/haddock-ghc-9.4.1 '@setup-per-component.dist/work/dist/doc/html/Includes2/haddock-response35120-1.txt'
   0% (  0 /  3) in 'Mine'
  Missing documentation for:
    Module header
    Mine (mylib/Mine.hs:3)
    mine (mylib/Mine.hs:4)
haddock: internal error: haddock:iface
CallStack (from HasCallStack):
  error, called at utils/haddock/haddock-api/src/Haddock/Interface.hs:217:16 in main:Haddock.Interface


*** unexpected failure for PackageTests/Backpack/Includes2/setup-per-component.test.hs

@ulysses4ever
Copy link
Collaborator Author

ah, now I see it, thanks!

@ulysses4ever ulysses4ever force-pushed the adopt-ghc-9.4.1 branch 2 times, most recently from 4632b5f to 577e83b Compare August 26, 2022 02:39
@ulysses4ever
Copy link
Collaborator Author

Windows 🤯

@Mikolaj
Copy link
Member

Mikolaj commented Aug 26, 2022

Joy. This is something to do with foreign libraries, which has just been changed in GHC and cabal. @bgamari: does it look there is a problem in the new machinery or is the test outdated somehow? It's this one: https://github.com/haskell/cabal/tree/master/cabal-testsuite/PackageTests/ForeignLibs

@Mikolaj
Copy link
Member

Mikolaj commented Aug 26, 2022

@Mistuke: did you see anything like that in you 9.4 tests or does it ring a bell otherwise?


+ "C:\tools\ghc-9.4.1\lib\../mingw/bin\clang.exe" "-std=c11" "-Wall" "-o" "uselib" "UseLib.c" "-l" "myforeignlib" "-L" "D:\a\cabal\cabal\cabal-testsuite\PackageTests\ForeignLibs\setup.dist\usr"
ld.lld: error: undefined symbol: sayHi

>>> referenced by C:/Users/RUNNER~1/AppData/Local/Temp/UseLib-ef4d96.o:(main)

clang: error: linker command failed with exit code 1 (use -v to see invocation)


stderr:
*** Exception: Command "C:\tools\ghc-9.4.1\lib\../mingw/bin\clang.exe" "-std=c11" "-Wall" "-o" "uselib" "UseLib.c" "-l" "myforeignlib" "-L" "D:\a\cabal\cabal\cabal-testsuite\PackageTests\ForeignLibs\setup.dist\usr" failed.
Output:
ld.lld: error: undefined symbol: sayHi

The log is from https://github.com/haskell/cabal/runs/8028896326?check_suite_focus=true

The test is https://github.com/haskell/cabal/tree/master/cabal-testsuite/PackageTests/ForeignLibs

@ulysses4ever
Copy link
Collaborator Author

@Mikolaj incidentally: this is very close to what I saw on hackage-security CI when updating to GHC 9.4 but forgetting to update to cabal 3.8 at the same time. Example

@Mistuke
Copy link
Collaborator

Mistuke commented Aug 26, 2022

@Mistuke: did you see anything like that in you 9.4 tests or does it ring a bell otherwise?


+ "C:\tools\ghc-9.4.1\lib\../mingw/bin\clang.exe" "-std=c11" "-Wall" "-o" "uselib" "UseLib.c" "-l" "myforeignlib" "-L" "D:\a\cabal\cabal\cabal-testsuite\PackageTests\ForeignLibs\setup.dist\usr"
ld.lld: error: undefined symbol: sayHi

@Mikolaj in GHC 9.4 the foreign wrappers were rewritten, so might be a bug there, but the test itself is a bit suspicious, the call to sayHi isn't marked extern so it's visibility is local by default..

Does it work if you add extern here? https://github.com/haskell/cabal/blob/master/cabal-testsuite/PackageTests/ForeignLibs/UseLib.c#L7

@ulysses4ever
Copy link
Collaborator Author

@Mistuke adding extern doesn't seem to make a difference.

@Mistuke
Copy link
Collaborator

Mistuke commented Aug 27, 2022 via email

@andreasabel
Copy link
Member

I needed not-yet-merged #8398, so it may look a bit messy. Hopefully, after that one is merged, we rebase and all is good.

I hand-merged it, so maybe you can rebase this on latest master now?

@ulysses4ever
Copy link
Collaborator Author

@andreasabel

I hand-merged it, so maybe you can rebase this on latest master now?

Thanks. Done.

@Mistuke
Copy link
Collaborator

Mistuke commented Aug 31, 2022

@ulysses4ever I'm having some trouble reproducing the issue, what's the easiest way to?

@Mistuke
Copy link
Collaborator

Mistuke commented Aug 31, 2022

alternatively, is it possible to get a GHC -v3 log from the test compilation?

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Sep 1, 2022

@Mistuke I'm sorry but I don't have a Windows machine suitable for reproduction of the failure at hand. We only see the failure in CI currently. I can try to get such a machine but it may take time. If you have one, the way to reproduce it is:

❯ git clone https://github.com/ulysses4ever/cabal && cd cabal && git checkout adopt-ghc-9.4.1-win-fail
...
❯ cabal build cabal cabal-tests
... 
❯ cabal run cabal-tests -- --with-cabal="$(cabal list-bin cabal)" cabal-testsuite/PackageTests/ForeignLibs/setup.test.hs

(forward slashes should work on Windows, right? otherwise, change for backslashes in the test path...)

@ulysses4ever
Copy link
Collaborator Author

Oh my goodness... I think I solved it -- by updating cabal in the container with old GHC. Ready for review!

.github/workflows/validate.yml Show resolved Hide resolved
cabal-testsuite/cabal-testsuite.cabal Show resolved Hide resolved
@ulysses4ever ulysses4ever force-pushed the adopt-ghc-9.4.1 branch 2 times, most recently from 5570e88 to 3ceebb1 Compare September 19, 2022 20:01
@ysangkok
Copy link
Member

Why are some of the tests guarded by == 9.4.* and others by == 9.4.2? Seems weird to guess on which of these will be fixed in the next release.

@ulysses4ever
Copy link
Collaborator Author

@ysangkok no particular reason, I guess. What do you propose, 9.4.2 or 9.4.*? I can set whatever you think is better (I have no preference, personally).

@ysangkok
Copy link
Member

Probably better to use 9.4.* and then revisit when the last 9.4 release comes out.

@ulysses4ever
Copy link
Collaborator Author

@ysangkok done!

@ulysses4ever
Copy link
Collaborator Author

@ysangkok since you took a look into it, maybe you'd consider approving it.

@ulysses4ever ulysses4ever added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Sep 22, 2022
@ulysses4ever
Copy link
Collaborator Author

Thanks @Kleidukos!


I'm using merge-me instead of squash because I specifically tried to organize commits so that a) they were individually buildable and pass CI (though I haven't check :-)) and b) so that the Windows tests can re-enabled with a simple revert when #8451 is addressed.

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 24, 2022
@mergify mergify bot merged commit 410f871 into haskell:master Sep 24, 2022
@jneira jneira mentioned this pull request Sep 27, 2022
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 me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants