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

Tue Jul 21 13:59:04 UTC 2020

Ben Franksen <ben.franksen at online.de> added the comment:

I could swear I did reply to the review but now I can see it neither in
my email nor on the tracker nor in my drafts folder. Strange.

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

I asked you about that one before making the change ;-)

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

Addressed by

patch d559b09f9516251c3ad5bf9cb331f65767bab152
Author: Ben Franksen <ben.franksen at online.de>
Date:   Sun Jul 19 16:34:54 CEST 2020
  * remove an unneeded LANGUAGE CPP pragma

and

patch 368451e02778df65951b7f8a024c2564e71426f7
Author: Ben Franksen <ben.franksen at online.de>
Date:   Sun Jul 19 16:35:54 CEST 2020
  * remove all MonadFail instances in darcs lib

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

Yes, I released 2.14.3 and 2.14.4 with it. Took quite some fiddling
until it worked to my satisfaction, but i think it's okay now.

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

I felt this was implied, but feel free to re-word to make it more explicit.

>>   * clean up Darcs.Test.Patch.Check
>
> 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)

Addressed by

patch cbf8512f7107db7f82579b448ccd7d7825e3ddd3
Author: Ben Franksen <ben.franksen at online.de>
Date:   Sun Jul 19 15:35:17 CEST 2020
  * improve docs for PatchCheck

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

It is not ideal, which is why I tried to get rid of shelly in favour of
turtle. Unfortunately that failed to work on Windows.

The obvious "solution" is to add every source file from the bundled
shelly to our darcs.cabal. This would be a very ugly.

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

The general idea is that /all/ exceptions are user errors, except
ErrorCall. If an exception, for instance an IOError, should not lead to
failure of the command, then it must be caught.

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

See below.

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

The long comments for these two patches are pretty confusing. Sorry
about that. Please ignore them.

The idea here was to make the minimally invasive changes that kept the
behavior the same it was before, across different compiler/base-library
versions. The instance MonadFail IO throws a UserError and i believe
most monad transformers just lifted the fail method. The monads in this
case are Apply and TreeMonad, for which instances in Darcs proper (i.e.
outside of the test harness) are usually based on IO. The replacement
'throw . userError' is the closest approximation to the IO monad's
'fail' that I could think of.

Note that UserError is an IOException, which means that if code at lower
levels (like apply) throws a UserError, we can (and in some places do)
catch it, along with any other IOExceptions. If we do not catch them, it
is is a normal user error.

Throwing UserErrors in the instances of Apply and TreeMonad is not
ideal. This should be reviewed but that is pretty much out of scope for
this patch bundle.

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

I'd say we postpone that decision. Feel free to remind me when we have
done the release. I am personally not inconvenienced by this. I
occasionally run a loop like

for g in /opt/ghc/8.10.1/bin/ghc /opt/ghc/8.2.2/bin/ghc
/opt/ghc/8.4.4/bin/ghc /opt/ghc/8.6.5/bin/ghc /opt/ghc/8.8.2/bin/ghc; do
cabal build --disable-optimisation --enable-tests -fwarn-as-error
--with-compiler=$g || break
done

and less often also build with optimisations and run the test suite. I
haven't yet encountered any breakage with the newer compilers (after
applying this bundle, I am still using ghc-8.2.2 by default).

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