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

Ganesh Sittampalam ganesh at earth.li
Sat Jul 18 15:52:28 UTC 2020

>   * loosen upper bound for haskeline to <0.9


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


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


>   * 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.

> patch 347aeb4b5c1eccfe00956ac318a2123bca9ef9ca
> Author: Ben Franksen <ben.franksen at online.de>
> Date:   Sun Apr 26 20:26:07 CEST 2020
>   * 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.
> patch f85f4c41eeb116483655b6cd6c337845e63226ce
> Author: Ben Franksen <ben.franksen at online.de>
> Date:   Mon Apr 27 15:54:37 CEST 2020
>   * resolve conflicts after 347aeb4b5c1eccfe00956ac318a2123bca9ef9ca


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.


>   * 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


>   * 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. In the long run we
> should consider switching to turtle, which is actually maintained by the
> author (a well known and quite diligent member of the community).

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


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


>   * 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.



>   * 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.


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


>   * 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?

More information about the darcs-devel mailing list