Merge the late additions to ghc-8.10 into ghc-9.0 #1296

Kleidukos · 2021-01-21T14:22:38Z

Last merge request to synchronise ghc-9.0 and ghc-8.10, ghc-9.0 is now the main branch.

int-index · 2021-01-24T15:37:04Z

.github/workflows/ci.yml

-    - uses: actions/cache@v1
+    - name: Freeze
+      run: |
+        cabal freeze --allow-newer


It’s probably a better idea to update cabal.project accordingly so that local builds work too, see #1305

int-index · 2021-01-24T15:38:48Z

.github/workflows/ci.yml

      run: |
-        cabal freeze
+        echo "$HOME/.ghcup/bin" >> $GITHUB_PATH
+        sudo apt-get install libtinfo-dev


Why is this needed? In the build log I see:

libtinfo-dev is already the newest version (6.1-1ubuntu1.18.04). libtinfo-dev set to manually installed.

int-index · 2021-01-24T15:39:42Z

.github/workflows/ci.yml


    steps:
    - uses: actions/checkout@v2
-      if: github.event.action == 'opened' || github.event.action == 'synchronize' || github.event.ref == 'refs/heads/ghc-8.10'
+      if: github.event.action == 'opened' || github.event.action == 'synchronize' || github.event.ref == 'refs/heads/ghc-9.0'


What would go wrong without this line (i.e. if it is removed entirely)? cc @chshersh

Nothing should go wrong. This is just an optimization. According to docs, possible events that trigger GitHub Actions include a lot of things, and by default CI is running a lot of times for different type events:

https://docs.github.com/en/developers/webhooks-and-events/github-event-types

My understanding of this, is this is configured that way mostly because GitHub Actions can do a lot of things, not just build the code, but also review, add labels, write messages, etc. However, I usually only want to build on code changes for pull requests and commits to the main branches, and don't want to spend GitHub Actions resources on other events.

OK, but doesn’t this if only apply to the checkout step? The other steps would run but without cloning the sources, and then fail

Good point. Feel free to remove this line. I guess, nothing should become broken, and the config can be simplified.

I reread the documentation again, and it seems that the correct way to filter by events is something like this:

https://github.com/Exheredrey/Exheredrey/blob/5a11c102af246b144ced9674dfb28ef235fd8d9f/.github/workflows/clang.yml#L5-L7

int-index · 2021-01-24T15:41:22Z

.github/workflows/ci.yml

        ghc:
-          - "8.10.1"
-          - "8.10.2"
+          - "9.0.0.20201227"


Since we have a separate branch for every release, I don’t think there’s much point to specifying the ghc version in the matrix – could be moved directly into the Setup Haskell step

int-index · 2021-01-24T15:42:25Z

.github/workflows/ci.yml


    - name: Build
      run: |
-        cabal configure --enable-tests --enable-benchmarks --test-show-details=direct
-        cabal build all
+        cabal configure --enable-tests --enable-benchmarks --test-show-details=direct --allow-newer -w ghc-9.0


Likewise, instead of --allow-newer and -w ghc-9.0 here, it’d be more convenient for building locally if this was specified in cabal.project (#1305)

Okay great :)

int-index · 2021-01-24T15:42:49Z

haddock-api/haddock-api.cabal

@@ -189,6 +189,7 @@ test-suite spec
               , containers
               , deepseq
               , directory
+               , exceptions


The previous lack of CI shows! 😄

You don't say…
I'm really playing cat and mouse here :P

int-index · 2021-01-24T15:44:57Z

hoogle-test/src/Bug873/Bug873.hs

@@ -1,5 +0,0 @@
-module Bug873 (($), ($$)) where


Is this test useless or something? Why is it removed? (I don’t object, just curious)

Undecipherable failure, it's planned to reinstate it but we're short on contributors to properly take care of that.

Looking into it

By “undecipherable failure” you probably mean this:

Failed to run Haddock on test package 'Bug873' hoogle-test: hoogle-test/out/Bug873: removeDirectoryRecursive:getSymbolicLinkStatus: does not exist (No such file or directory)

Turns out this error message is produced by the use of removeDirectoryRecursive in haddock-test/src/Test/Haddock.hs, in the runHaddock function:

unless succeeded $ removeDirectoryRecursive (outDir cfgDirConfig tpkg)

This code is weird. It runs when the test failed (unless succeeded) and assumes that the outDir exists (removeDirectoryRecursive expects the directory to exist). However, it is precisely when the test fails that we don’t get an output directory. So we would get this undecipherable failure for any other failing test, not Bug873 specifically. The test driver needs fixing.

By commenting out this line I got a more helpful error message:

Failed to run Haddock on test package 'Bug873' Haddock output is at '/dev/null'. This file can be set with `--haddock-stdout`.

OK, so haddock fails on this file, but its output is hidden. I haven’t figured out how to use the --haddock-stdout option though, so I hacked it around by specifying let haddockStdOut = stdout in runHaddock and got this output:

haddock: While creating Haddock interface for Bug873: While creating Haddock interface for Bug873: get Wrap: Bad h CallStack (from HasCallStack): error, called at src/Haddock/InterfaceFile.hs:729:12 in haddock-api-2.24.0-inplace:Haddock.InterfaceFile

Now we’re talking! This error is apparently produced by this code in haddock-api/src/Haddock/InterfaceFile.hs:

instance Binary n => Binary (Wrap n) where put_ bh (Unadorned n) = do putByte bh 0 put_ bh n put_ bh (Parenthesized n) = do putByte bh 1 put_ bh n put_ bh (Backticked n) = do putByte bh 2 put_ bh n get bh = do h <- getByte bh case h of 0 -> do name <- get bh return (Unadorned name) 1 -> do name <- get bh return (Parenthesized name) 2 -> do name <- get bh return (Backticked name) _ -> error "get Wrap: Bad h"

This is as far as I got. And since you mention binary serialization issues in #1296 (comment), I suspect it might be related.

OK, so haddock fails on this file, but its output is hidden. I haven’t figured out how to use the --haddock-stdout option though

I think you can pass it with --test-options from Cabal, or else if you cabal run the test suite, you can pass it to --run-options. The removeDirectoryRecursive is an issue I remember coming up against too (and I think I similarly would just change the code.

The binary serialization issue (for which I got a ping, but can't find the original comment), is probably due to the fact that the version of GHC used for CI comes with .haddock interface files built using the version of Haddock that was in the submodule at the time. However, this PR introduces breaking changes to that binary format. In order for this test to pass, you end up in a chicken and egg problem: you need GHC to bump the Haddock submodule so that the generated base.haddock matches the binary format that this PR has, but you can only bump the GHC submodule once this PR is merged.

The circular dependency problem is part of why at some point I embedded (most of) Haddock's test suite inside GHC's testsuite (at least there we would catch regressions). Over time, I've come more and more to believe that we should just try to move Haddock into GHC's tree (perhaps with the one exception being the haddock-library library, which is designed to build on older GHC versions, eg. so it can be used in Pandoc). Of course, the downside is that this would create lots of developer friction (in order to contribute to Haddock, you'd need to go through the whole GHC merge/build process).

@harpocrates Hi! Thanks for chiming in, and sorry for the ghost ping, I had a bad case of "hands doing weird stuff with the GitHub interface".

alexbiehl · 2021-02-05T14:02:45Z

There we go, it’s green!

Kleidukos · 2021-02-05T14:03:50Z

yep', sounds like it. :)

Merge the late additions to ghc-8.10 into ghc-9.0

Kleidukos mentioned this pull request Jan 24, 2021

WIP: Simplify CI and update for 9.0 #1304

Closed

int-index reviewed Jan 24, 2021

View reviewed changes

Kleidukos mentioned this pull request Jan 24, 2021

Binary serialisation/deserialisation changes for interface files with GHC 9.0? #1307

Closed

Kleidukos and others added 2 commits February 5, 2021 14:53

Move CI to ghc-9.0

664122f

Update cabal.project and README build instructions

fcf9a04

Kleidukos merged commit 17acefb into haskell:ghc-9.0 Feb 5, 2021

garetxe mentioned this pull request Feb 6, 2021

Add support for labeled module references #1315

Closed

Kleidukos mentioned this pull request Feb 18, 2021

The CI badge in README still points to Travis #1300

Closed

hubot pushed a commit to ghc/ghc that referenced this pull request May 17, 2024

Merge pull request haskell/haddock#1296 from Kleidukos/ghc-9.0

31bd292

Merge the late additions to ghc-8.10 into ghc-9.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge the late additions to ghc-8.10 into ghc-9.0 #1296

Merge the late additions to ghc-8.10 into ghc-9.0 #1296

Kleidukos commented Jan 21, 2021

int-index Jan 24, 2021 •

edited

Loading

int-index Jan 24, 2021

int-index Jan 24, 2021

chshersh Jan 26, 2021

int-index Jan 26, 2021

chshersh Jan 27, 2021

int-index Jan 24, 2021

int-index Jan 24, 2021

Kleidukos Jan 24, 2021

int-index Jan 24, 2021

Kleidukos Jan 24, 2021

int-index Jan 24, 2021

Kleidukos Jan 24, 2021

int-index Jan 24, 2021

int-index Jan 24, 2021 •

edited

Loading

harpocrates Jan 24, 2021

Kleidukos Jan 24, 2021

alexbiehl commented Feb 5, 2021

Kleidukos commented Feb 5, 2021

Merge the late additions to ghc-8.10 into ghc-9.0 #1296

Merge the late additions to ghc-8.10 into ghc-9.0 #1296

Conversation

Kleidukos commented Jan 21, 2021

int-index Jan 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

int-index Jan 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbiehl commented Feb 5, 2021

Kleidukos commented Feb 5, 2021

int-index Jan 24, 2021 •

edited

Loading

int-index Jan 24, 2021 •

edited

Loading