On 8/7/2016 21:16, Ivan Zhakov wrote: > On 7 August 2016 at 21:27, Stefan Sperling <s...@elego.de> wrote: >> On Sun, Aug 07, 2016 at 08:23:58PM +0200, Stefan Sperling wrote: >>> On Sun, Aug 07, 2016 at 02:08:35PM +0200, Stefan wrote: >>>> Hi, >>>> >>>> the attached patch is the outcome of talking with jcorval on IRC about a >>>> test suite issue on Windows (release builds) in SVN 1.8. It resolves the >>>> fact that the tests will be interrupted on Windows by a Windows popup >>>> upon an abort()-call. >>>> >>>> Atm this is triggered for the 1.8 test suite for the move-test (no 8) >>>> which is marked as XFail and triggers an SVN_ERR_ASSERT() and therefore >>>> breaks fully automated tests. >>> I'm not a windows person but this looks reasonable to me. +1 >> Forgot to mention: If this is a 1.8-only fix, I'd suggest you create >> a branch of 1.8.x and commit your patch there, and then nominate your >> branch in the 1.8.x/STATUS file (you can add my +1 to the nomination). > I'm not sure that we should add handling of new environment variable > in patch release. > > I can understand the concern. Please bare in mind however that the default behavior (aka: if the environment variable is not set) would stay unchanged and the effect of the environment variable is quite limited (it will only disable the Watson crash dumps in release builds - obviously only applies on Windows builds). It's by default only activated for the test suites and therefore the risks involved is IMO close to non existent.
Weighting the pros (fixing a test interruption of the test suite for everybody testing/building the SVN package) with the cons (the risk of s/o unintentionally already having set an environment variable with the same name and on the other side introducing a new test-suite related feature in a patch release which would be not available in versions <= 1.8.16), I do have a tendency to still propose that patch. (Note: I would suggest otherwise, if it wasn't related to a fix for the testsuite, since then it could hardly be seen as a bugfix). I thought about alternatives, but rejected them due to different reasons: Alternative 1: Do not use the environment variable and rather make it implicit behavior to disable the Watson crash reports. Rejected, since that would be a behavior change of SVN. Downstream releases of SVN might rely on current crashdump integration and would be broken. Alternative 2 (untested): For the test environment set the corresponding Windows registry entry [1] prior to running the SVN tests so to disable sending the crashreports and/or bring up the popup to attach a debugger. Rejected, since that would be an additional barrier to run the testsuite especially for people building SVN themselves. IMO there should be as few hurdles as possible to build and test SVN, so to not risk stopping people from giving SVN a try. Alternative 3: Use a config setting instead of an environment variable. Rejected, since there we could not disable the crashreports before the config file was processed. Alternative 4: Use a command line parameter to disable crashreports. Rejected, since it would only work for command line tests but not for testing the C API. Maybe your call, Ivan, would be to use alternative 2 for SVN <= 1.9 and introduce the environment variable related fix in trunk only? [1] https://blogs.msdn.microsoft.com/alejacma/2011/02/18/how-to-disable-the-pop-up-that-windows-shows-when-an-app-crashes/ Regards, Stefan
smime.p7s
Description: S/MIME Cryptographic Signature
