3 patches for repository http://darcs.net/screened:

patch 7f44cc4a6b7586acb9c9396a4c0620fa7f7486df
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Aug 18 10:48:41 CEST 2019
  * extend test script for pristine conversion

patch 05c6dd13cb73266898e2fed05deafc1bbb4588e4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 15 19:34:38 CEST 2019
  * tests/conflict-fight-failure.sh: allow 10% overshoot margin
  
  This makes the test more robust against small fluctuations in timing.

patch 90e0577336e80e2bc7656c1d8c27343d40b4f9bc
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 17 18:43:43 CEST 2019
  * harness: create an environment file and source it from tests/lib
  
  This fixes a long-time problem with the test harness: when you run the tests
  in a specific directory, then cd there and try to run one of the scripts
  manually, you didn't have the same environment as the original test run. So
  you had to set PATH, EMAIL, TESTDATA, etc manually on the command line. We
  now create a file named env that gets sourced by lib in addition to setting
  the environment before we run the test.

Attachments

• extend-test-script-for-pristine-conversion.dpatch
• patch-preview.txt
• unnamed

The environment file idea is a good plan, reproducing
failures has always been annoying.

Sadly it doesn't work perfectly on Windows, the comments
in this patch should be self-explanatory. So I've
disabled it there only, but I can still source env manually
if I want to.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 23aed63a617491027d2ef76015e344f39d8591d6
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Aug 25 14:32:43 BST 2019
  * harness: windows fixes for env file
  
  This corrects issues on Windows introduced by
  "harness: create an environment file and source it
  from tests/lib"

Attachments

• harness_-windows-fixes-for-env-file.dpatch
• patch-preview.txt
• unnamed

The other two patches are fine too. Could you review my followup?

> patch 23aed63a617491027d2ef76015e344f39d8591d6
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Sun Aug 25 14:32:43 BST 2019
>   * harness: windows fixes for env file
>   
>   This corrects issues on Windows introduced by
>   "harness: create an environment file and source it
>   from tests/lib"

I think your changes are mostly okay. Sorry for making life more
complicated for you, I didn't expect this to cause difficulties on
Windows. Your comments in the code are appreciated; it took me a while
to understand the problem but I think I do now.

Some remarks follow.

Have you considered

http://hackage.haskell.org/package/filepath-1.4.2.1/docs/System-FilePath-Posix.html#g:2

? Perhaps this could be used to avoid some of the #ifdefs and extra
definitions for Windows.

Regarding the TODO comment in harness/test.hs:

     -- TODO: on Windows ./lib deliberately doesn't source ./env and
instead relies
     -- on this environment, because the paths we write out to env are
in Windows format
     -- (e.g. c:\msys64\home\ganesh) rather than the msys format
(/home/ganesh) which
     -- is expected in msys shell scripts.
     -- While in practice this seems to mostly work, it does break the
'harness.sh'
     -- sanity test and could also cause other subtle effects.

Since this is the test harness and not code for darcs proper, couldn't
we just fix/adapt harness.sh and deal with any remaining "subtle
effects" whenever we notice them?

      -- .... Failing that we could do the translation here and then do some
     -- checks in lib/env that the inherited env matches the script's env.

I may be wrong but I think such a check would defeat the purpose of
being able to run the tests without the harness.

> Have you considered
> 
> http://hackage.haskell.org/package/filepath-1.4.2.1/docs/System-FilePath-Posix.html#g:2
> 
> ? Perhaps this could be used to avoid some of the #ifdefs and extra
> definitions for Windows.

I hadn't thought of that. It looks like a good idea.

> Since this is the test harness and not code for darcs proper, couldn't
> we just fix/adapt harness.sh and deal with any remaining "subtle
> effects" whenever we notice them?

The thing is that the subtle effects would happen exactly when a Windows
developer (i.e. me :-) ) is trying to understand some bad behaviour
anyway. I've been in that kind of position in the last couple of days.

First there was the NUL problem which was indirectly caused by me in
that I wrote the QuantifiedConstraints patch and therefore switched to
GHC 8.6, but I first thought it was something you'd changed. Then a
couple of different problems with env (path separator and then the
actual behaviour of PATH) but of course I first thought it was my GHC
8.6 fix. Right now half the network tests are broken which turned out to
be because openssh started segfaulting after I upgraded msys. Still
stuck on that one.

All the problems are orthogonal, but it takes work to discover that so
I'd rather just eliminate one variable. I can always source/enable env
manually when I want to, or if that becomes too painful maybe I'll
change my mind about the balance of risk. I have at least taught myself
to first check whether a problem can be reproduced on reviewed before
jumping to conclusions :-)


>       -- .... Failing that we could do the translation here and then do some
>      -- checks in lib/env that the inherited env matches the script's env.
> 
> I may be wrong but I think such a check would defeat the purpose of
> being able to run the tests without the harness.

I meant when running in the test harness. Actually perhaps the checks
would better belong in a separate test that we'd never want to run for
any other reason.

I'll update that comment and look at using splitSearchPath, once I've
fixed my working environment.

>> Since this is the test harness and not code for darcs proper, couldn't
>> we just fix/adapt harness.sh and deal with any remaining "subtle
>> effects" whenever we notice them?
> 
> All the problems are orthogonal, but it takes work to discover that so
> I'd rather just eliminate one variable.

Okay, this is absolutely fine with me.

>>       -- .... Failing that we could do the translation here and then do some
>>      -- checks in lib/env that the inherited env matches the script's env.
>>
>> I may be wrong but I think such a check would defeat the purpose of
>> being able to run the tests without the harness.
> 
> I meant when running in the test harness. Actually perhaps the checks
> would better belong in a separate test that we'd never want to run for
> any other reason.
> 
> I'll update that comment and look at using splitSearchPath, once I've
> fixed my working environment.

Good! And thanks for trying to fix darcs on Windows! I would really like
us to get out the next release with a working Windows binary that is
easy to install and has been well-tested.

I did a bit more research and discovered there is
an easy way to translate the paths for Windows.

I couldn't see an elegant way to handle all the
different cases directly (direct env injection/bash
script on Linux/bash script on Windows) so ended up
introducing a new datatype to make everything
explicit.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 95ae5700feba1c19016b9711167ef3400c50fe88
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Aug 26 21:18:43 BST 2019
  * harness: properly translate env script on Windows
  
  We have Windows paths in Haskell but we need
  Unix style paths in the env script

Attachments

• harness_-properly-translate-env-script-on-windows.dpatch
• patch-preview.txt
• unnamed

> I did a bit more research and discovered there is
> an easy way to translate the paths for Windows.
> 
> I couldn't see an elegant way to handle all the
> different cases directly (direct env injection/bash
> script on Linux/bash script on Windows) so ended up
> introducing a new datatype to make everything
> explicit.
> 
> 1 patch for repository darcs-unstable@darcs.net:screened:
> 
> patch 95ae5700feba1c19016b9711167ef3400c50fe88
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Mon Aug 26 21:18:43 BST 2019
>   * harness: properly translate env script on Windows
>   
>   We have Windows paths in Haskell but we need
>   Unix style paths in the env script

Beautiful! The data type is absolutely the right thing to do here.

At this point we may want to move all the code that concerns (only)
shell tests from test.hs to a its own module Darcs.Test.Shell.

Anyway, all accepted (including mine).