Re: [PATCH] Resolve testsuite interruption in SVN 1.8

On 8/8/2016 12:22 PM, Ivan Zhakov wrote:
> On 7 August 2016 at 23:14, Stefan <luke1410_at_posteo.de> wrote:
>> On 8/7/2016 21:16, Ivan Zhakov wrote:
>>> On 7 August 2016 at 21:27, Stefan Sperling <stsp_at_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.
>>
> Could you please first clarify: does this problem exist in trunk? I'm
> asking because subject is somewhat confusing because it only mention
> "SVN 1.8". If yes, we should discuss solution for trunk first. And
> then discuss backporting committed solution to active stable branches
> separately.
To clarify: I'd define the actual design problem as follows:
For release builds on Windows the test suite lacks the capability of
automatically handling abort()-calls in such a way that it won't require
manual interventions.
This problem exists on trunk.

> Regarding the patch itself (assuming it for trunk): why you have
> decided to check for SVN_CMDLINE_DISABLE_WATSON_ON_ABORT environment
> variable only if SVN_CMDLINE_USE_DIALOG_FOR_ABORT is not set? It
> creates very hard to understand dependency on other environment
> variables that controls crashhandler/abort() behavior. Because
> currently suggested behavior would be:
> if (!getenv("SVN_CMDLINE_DISABLE_CRASH_HANDLER"))
> {
> if (!getenv("SVN_CMDLINE_USE_DIALOG_FOR_ABORT"))
> {
> if (getenv("SVN_CMDLINE_DISABLE_WATSON_ON_ABORT"))
> {
> _set_abort_behavior(0, _CALL_REPORTFAULT);
> }
> }
> }
>
I'll split that up into separate questions:
1. SVN_CMDLINE_DISABLE_WATSON_ON_ABORT is only specified, unless the SVN
crash handler is disabled, because I assumed that the design decision
was that the SVN_CMDLINE_DISABLE_CRASH_HANDLER environment variable
should control whether the OS-default crash handling behavior should be
used, if specified (aka: do not mess around with system defaults, if
SVN_CMDLINE_DISABLE_CRASH_HANDLER is specified).

2. Not specifying SVN_CMDLINE_USE_DIALOG_FOR_ABORT was chosen as a
prerequisite for the SVN_CMDLINE_DISABLE_WATSON_ON_ABORT design, because
I assumed the design of SVN_CMDLINE_USE_DIALOG_FOR_ABORT was to ensure
that the OS-default dialog is to be used for abort-calls (and disabling
the Watson crash dumps would change that default handling).

I'm certainly not against having SVN_CMDLINE_DISABLE_WATSON_ON_ABORT be
evaluated completely independent from the other two environment
variables in order to make its definition easier to comprehend and leave
it with the user to ensure it's used reasonably in combination with the
other two environment variables.

> Btw what are the problems with approach to disable watson reports on
> abort(), except backporting? I mean we already override Windows Error
> Reporting by installing our own SEH exception handler, so it looks
> natural to disable abort() reporting also.
>
I agree with you on this one. I wasn't aware that SVN does already alter
the default exception handling on Windows (and therefore effectively
disable the Watson crash dump reports in case of an unhanled exception).
So disabling the Watson crash dumps on abort calls does indeed sound
like increasing the system/design consistency in SVN.
You got my non-binding +1 on that proposal (which obviously would be a
replacement for my proposed patch).

> Also your patch is missing documentation changes in notes/knobs.
>
Thanks for pointing that out. Wasn't aware of this document before.
Unless the tendency is to go with your suggestion (see above), I'll
provide an updated patch adding the missing documentation.

-- 
Regards,
Stefan Hett