Re: svn commit: r1809570 - /subversion/branches/1.9.x/STATUS

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1809570 - /subversion/branches/1.9.x/STATUS

Branko Čibej
On 24.09.2017 23:23, [hidden email] wrote:

> Author: danielsh
> Date: Sun Sep 24 21:23:40 2017
> New Revision: 1809570
>
> URL: http://svn.apache.org/viewvc?rev=1809570&view=rev
> Log:
> * STATUS: Edit the 1.9.x-r1808955 entry.
>
> Modified:
>     subversion/branches/1.9.x/STATUS
>
> Modified: subversion/branches/1.9.x/STATUS
> URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/STATUS?rev=1809570&r1=1809569&r2=1809570&view=diff
> ==============================================================================
> --- subversion/branches/1.9.x/STATUS (original)
> +++ subversion/branches/1.9.x/STATUS Sun Sep 24 21:23:40 2017
> @@ -76,6 +76,8 @@ Candidate changes:
>       ^/subversion/branches/1.9.x-r1808955
>     Votes:
>       +1: brane
> +     +0: danielsh (this works on OS X and Linux but nevertheless I wonder
> +          about its portability)


It's as portable as 'httpd -V'. If someone builds with a patched httpd
that does not print a the sever version with this option, then the
script will fail.

On the other hand, if we don't backport this change, the OSX tests on
the 1.9.x branch will keep failing indefinitely because (a) 'configure'
reports the wrong version of httpd, and (b) the 1.9.x tests do not have
the concept of an httpd version whitelist.

IMO it's more important to have reliable test results on supported
branches than to support unlikely patches that people might come up with.

-- Brane
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1809570 - /subversion/branches/1.9.x/STATUS

Daniel Shahaf-2
Branko Čibej wrote on Wed, 27 Sep 2017 01:28 +0200:

> On 24.09.2017 23:23, [hidden email] wrote:
> > +++ subversion/branches/1.9.x/STATUS Sun Sep 24 21:23:40 2017
> > @@ -76,6 +76,8 @@ Candidate changes:
> >       ^/subversion/branches/1.9.x-r1808955
> >     Votes:
> >       +1: brane
> > +     +0: danielsh (this works on OS X and Linux but nevertheless I wonder
> > +          about its portability)
>
>
> It's as portable as 'httpd -V'. If someone builds with a patched httpd
> that does not print a the sever version with this option, then the
> script will fail.
>

Why would it fail?  It neither does 'set -e' nor validates the value of $HTTPD_VERSION:

> > +HTTPD_VERSION=$("$HTTPD" -V -f $HTTPD_CFG | grep '^Server version:' | sed 's|^.*/\([0-9]*\.[0-9]*\.[0-9]*\).*$|\1|')

Besides, * in sed is greedy, so an output like

    Server version: Apache/2.4.50 (Counterexample Linux/1.2.3)

would DTWT.

> On the other hand, if we don't backport this change, the OSX tests on
> the 1.9.x branch will keep failing indefinitely because (a) 'configure'
> reports the wrong version of httpd, and (b) the 1.9.x tests do not have
> the concept of an httpd version whitelist.
>
> IMO it's more important to have reliable test results on supported
> branches than to support unlikely patches that people might come up with.

The patchset may be an improvement for OS X, but I am concerned that it
might introduce a regression for other platforms; that's why I didn't vote +1
on it.

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1809570 - /subversion/branches/1.9.x/STATUS

Branko Čibej
On 28.09.2017 14:41, Daniel Shahaf wrote:

> Branko Čibej wrote on Wed, 27 Sep 2017 01:28 +0200:
>> On 24.09.2017 23:23, [hidden email] wrote:
>>> +++ subversion/branches/1.9.x/STATUS Sun Sep 24 21:23:40 2017
>>> @@ -76,6 +76,8 @@ Candidate changes:
>>>       ^/subversion/branches/1.9.x-r1808955
>>>     Votes:
>>>       +1: brane
>>> +     +0: danielsh (this works on OS X and Linux but nevertheless I wonder
>>> +          about its portability)
>>
>> It's as portable as 'httpd -V'. If someone builds with a patched httpd
>> that does not print a the sever version with this option, then the
>> script will fail.
>>
> Why would it fail?  It neither does 'set -e' nor validates the value of $HTTPD_VERSION:
>
>>> +HTTPD_VERSION=$("$HTTPD" -V -f $HTTPD_CFG | grep '^Server version:' | sed 's|^.*/\([0-9]*\.[0-9]*\.[0-9]*\).*$|\1|')

It will fail to DTRT.

> Besides, * in sed is greedy, so an output like
>
>     Server version: Apache/2.4.50 (Counterexample Linux/1.2.3)
>
> would DTWT.

Yes. It would. It's not perfect. Neither is the current situation. But
one can always avoid using 'davautocheck' on strange platforms with
strange httpd builds; 'davcheck' works just fine, it only takes a bit
more effort; probably less effort than patching and building httpd in
the first place.

>> On the other hand, if we don't backport this change, the OSX tests on
>> the 1.9.x branch will keep failing indefinitely because (a) 'configure'
>> reports the wrong version of httpd, and (b) the 1.9.x tests do not have
>> the concept of an httpd version whitelist.
>>
>> IMO it's more important to have reliable test results on supported
>> branches than to support unlikely patches that people might come up with.
> The patchset may be an improvement for OS X, but I am concerned that it
> might introduce a regression for other platforms; that's why I didn't vote +1
> on it.

So first of all, the problem is not specific to OSX. It will happen in
any environment where the available httpd headers are for a different
version than the server binaries. In the case of the particular version
of OSX running on the slave, that's 2.2.26 (headers) vs. 2.2.29
(binary), the former being in our blacklist of broken httpd versions WRT
URL quoting in mod_dav.

Here are some other possible solutions:

  * Backport the httpd version whitelist logic to 1.9.x (more complex?)
  * Use a homegrown version of httpd on the bot (not gonna happen)
  * Fix system headers on the bot (nice try)
  * Leave the OSX DAV tests broken
  * ? I'm out of ideas

The first option is the only viable one apart from what I've already
done, IMO.

-- Brane