[darcs-devel] [patch2017] support ghc-8.8 and ghc-8.10

Ganesh Sittampalam bugs at darcs.net
Sat Jul 18 16:03:09 UTC 2020


Ganesh Sittampalam <ganesh at earth.li> added the comment:

Sorry for the duplicate, it's been so long since I reviewed I had
forgotten I need to send email reviews to bugs at . I'm resending so it
appears on the bug tracker but feel free to reply to either one.

This should be the same as the message already sent direct to
darcs-devel modulo some reformatting/snipping where quoting seems to
have gone wrong.

>   * loosen upper bound for haskeline to <0.9

OK.

>   * loosen upper bound for shelly to <1.10
>   * resolve conflicts after 5d63e665634c964d7b23dcf9c28efc72d6a6f947

OK.

>   * darcs.cabal: clean up extra-source-files
>   * resolve conflicts after 2dbe7ff7042a2d909640f468c2d1dd39a00ca38b

OK.

>   * fix license in darcs.cabal

OK (had to stop and check that we have always been GPL-2.0-or-later, I
guess these things are easy to forget!)

>   * for base >=4.13 define fail in MonadFail instances
>   * resolve conflicts after 447f754fd0613f221cdf6174e67a2e020d71bdb6

OK. Though :-( to CPP, of course it's pretty much unavoidable for the
range of GHC versions wanted.

>   * add some elementary build instructions to the README.md

OK (I learnt a few things from that!)

>   * adapt release/release.sh for use with modern cabal

OK - I assume you will have battle-tested this.

>   * mitigate issue2643 with a better error message
>   
>   If the patch index is corrupt, output the name of the currupt file
>   and suggest its removal.
> 
>   * resolve conflicts after 82fe27cc6295a2e55936b3b1ff10fcb4966e1e5e

Looks good. The user message might be clearer still if it said that it
is always safe to remove the index but it's no big deal.

>   * clean up Darcs.Test.Patch.Check
>   
>   There was a comment in the code about the strangeness of how in this 
>   test module returning a Boolean was used to communicate a failure,
>   and how this should be replaced with a more conventional way of
>   error handling.
>   This is now done, using the MaybeT monad transformer.
> 
>   * resolve conflicts after 347aeb4b5c1eccfe00956ac318a2123bca9ef9ca

OK.

One (optional) comment:

[in Darcs.Test.Patch.Check]
> -- | PatchCheck is a state monad with a simulated repository state

> newtype PatchCheck a = PatchCheck { runPatchCheck :: MaybeT (State
>                                          ValidState) a }

It would help a bit to indicate in the haddock what the MaybeT
signifies, although it is visible by reading down just a little bit to
the throw/catch code. (Just = things are ok, Nothing = check failed)

>   * add changelog entry for release 2.14.3

OK - I shall assume it's accurate!

>   * darcs.cabal: remove redundant version constraints
>   * remove a redundant import from Setup.hs

Nice cleanups.

>   * no longer export fromJust from Darcs.Prelude [amended
>     from branch-2.14]
>   
>   While used in many places, fromJust is a partial function and we
>   should not encourage its unfettered use in Darcs.

OK.

>   * remove redundant imports
>   
>   This mostly concerns (<>) imported from Darcs.Util.Printer and (<$>) 
>   from Control.Applicative. These are now consistently exported from
>   Darcs.Prelude.

>   * resolve conflicts after 80223edc7108090f9c3231e36d4bdb94e1f52f6a

OK.


>   * import shelly-1.7.1 as a new subdirectory

[quoting from main body of submission:]
> This worked out remarkably well. However, it currently works only
> when building from a darcs clone, since the files in the shelly sub
> directory are not included in the cabal source dist.

So that means someone downloading from hackage and running the tests
will get an automatic failure? This doesn't seem ideal in case people
would want to do some kind of CI over all of hackage or something like
that. This does already happen though I suspect it wouldn't include
darcs' tests anyway.


>   * unwind tests: use condition compilation for MonadFail instances 
>     and imports

OK

>   * avoid use of fail in Darcs.Patch.Depends.unwrapOneTagged

OK

>   * more consequently throw MatchFailure exception
>   
>   This avoids two uses of fail in Darcs.Patch.Match in the context of
>   a monad tranformer that is not necessarily IO based.

OK (so I guess MatchFailure is an exception that signifies a user error
rather than an internal error)

>   * Darcs.Util.Prompt: add liftIO when calling fail 

OK. So now we don't care about the current Monad being MonadFail, just
that it is MonadIO.

>   * replace fail with error in Darcs.Repository.Diff
>   
>   This is the /only/ case where the "quick fix" of replacing fail with
>   error is semantically valid. Here we clearly have a partial pattern
>   match and it would be a bug if we hit that case.

OK.


>   * replace fail with throw.userError when applying a patch
>   
>   There are several uses of apply (and its variants) where we catch
>   IOExceptions e.g. maybeApplyToTree. This makes sense: if we apply in 
>   the context of IO (or a transformer over IO), then there may occur
>   any kind of IOException, not only the ones explicitly raised from
>   our code, and we want to catch them all.

So we're saying that if this code is called in a context that catches
IOExceptions, then these cases are normal/accepted. If it's not called
in a such a context then hitting them is an internal error in darcs
(like calling error - of course the Haskell name 'userError' is
confusing here)?

>   * replace fail with throw.userError in Tree monads and index 
>     matching
>   
>   The situation here is different from that of patch application.
>   Here we want the exception to percolate all the way up to the
>   top level handler.
>   In other words, these are regular failures similar to IO exceptions.

And these ones are always internal errors or at least unexpected
situations that are not errors in what the user requested?

>   * replace runhaskell with runghc in tests/renames.sh
>   
>   It seems runghc is better supported by ghc packagers than 
>   runhaskell.
>   This is not a complete solution, though. If cabal is used with
>   --with-compiler=/path/to/ghc, there is no guarantee that we have a 
>   runghc or a runhaskell in the PATH.

OK.

>   * darcs.cabal: remove containers from setup-depends

OK.

>   * raise upper bounds for base and Cabal dependencies
>   
>   This allows to build with ghc-8.8 and ghc-8.10.

OK. Though I am concerned about trying to support this many GHCs (8.2 to
8.10 inclusive) on an ongoing basis. Can we bump the minimum on HEAD as
soon as we release 2.16?

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch2017>
__________________________________


More information about the darcs-devel mailing list