RFR: JDK-8189955 Configuration validation is broken for some types of paths

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8189955 Configuration validation is broken for some types of paths

Magnus Ihse Bursie
We are validating that the topdir we're using when running make is the
same as when we were running configure.

For some circumstances, most typically when using a subdirectory in the
user's home directory (e.g. ~/jdk10) on Cygwin, this check fails.

Bug: https://bugs.openjdk.java.net/browse/JDK-8189955
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8189955-configuration-validation-is-broken-for-some-paths/webrev.01

/Magnus

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189955 Configuration validation is broken for some types of paths

Tim Bell
Magnus:

> We are validating that the topdir we're using when running make is the
> same as when we were running configure.
>
> For some circumstances, most typically when using a subdirectory in the
> user's home directory (e.g. ~/jdk10) on Cygwin, this check fails.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189955
> WebRev:
> http://cr.openjdk.java.net/~ihse/JDK-8189955-configuration-validation-is-broken-for-some-paths/webrev.01

Looks good.

Tim

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189955 Configuration validation is broken for some types of paths

Erik Joelsson
In reply to this post by Magnus Ihse Bursie
I see why this huge check becomes necessary for a valid comparison, but
it's quite a bit of extra complexity added as well. I was worried about
the extra overhead on an already slow platform so I took the time to
measure. While it's certainly measurable it's hardly noticeable. On a
fast machine, the difference is around 50-100ms on repeated make
invocations (baseline 1.85s). On my slow laptop it's 300-400ms, but the
baseline is already over 7 seconds so as said, hardly noticeable.

That said, line 392 is easily expressible using the make filter function
instead of going to the shell. All the other shell calls seem to be
necessary however.

Just thinking some more. Would it be possible to do something like "cd
DIR && pwd" for both dirs and just compare the output of that instead?
If that works it would seem like way fewer shell calls and simpler logic.

/Erik


On 2017-10-25 17:03, Magnus Ihse Bursie wrote:

> We are validating that the topdir we're using when running make is the
> same as when we were running configure.
>
> For some circumstances, most typically when using a subdirectory in
> the user's home directory (e.g. ~/jdk10) on Cygwin, this check fails.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189955
> WebRev:
> http://cr.openjdk.java.net/~ihse/JDK-8189955-configuration-validation-is-broken-for-some-paths/webrev.01
>
> /Magnus
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189955 Configuration validation is broken for some types of paths

Magnus Ihse Bursie

On 2017-10-26 10:09, Erik Joelsson wrote:
> I see why this huge check becomes necessary for a valid comparison,
> but it's quite a bit of extra complexity added as well. I was worried
> about the extra overhead on an already slow platform so I took the
> time to measure. While it's certainly measurable it's hardly
> noticeable. On a fast machine, the difference is around 50-100ms on
> repeated make invocations (baseline 1.85s). On my slow laptop it's
> 300-400ms, but the baseline is already over 7 seconds so as said,
> hardly noticeable.
The alternative is to drop the check entirely. I did some source control
archeology now, to figure out the origin of this check. It has grown a
bit over the years to handle more complex cases, but the original test
was introduced by me in JDK-8076060, which was the big rewrite that
created Init.gmk and the current "bootstrapping" process of the build.

I can't, at least now, figure out what was the driving need behind that
check. It might been that I had collected some complaints about odd
broken setups that "the build system should check for that!!! i've
wasted hours!!!" but never got a separate bug. Or I might have
encountered issues myself while developing the patch. Or I just thought
it was prudent to check.

In any case, I think it's fully viable by now to remove the check in its
entirety. Do you agree?

>
> That said, line 392 is easily expressible using the make filter
> function instead of going to the shell. All the other shell calls seem
> to be necessary however.
>
> Just thinking some more. Would it be possible to do something like "cd
> DIR && pwd" for both dirs and just compare the output of that instead?
> If that works it would seem like way fewer shell calls and simpler logic.
Unfortunately, that will not present a canonical representation of the
directory. In cygwin, both /cygdrive/c/cygwin/home/user and /home/user
is the same directory, but cygwin will not rewrite the path of any of
them using that formula. :-( So for better or for worse, I think the
method that has hardened over the years in our configure script is a
battle-proven way to create a canonical path representation on all
platforms.

/Magnus

>
> /Erik
>
>
> On 2017-10-25 17:03, Magnus Ihse Bursie wrote:
>> We are validating that the topdir we're using when running make is
>> the same as when we were running configure.
>>
>> For some circumstances, most typically when using a subdirectory in
>> the user's home directory (e.g. ~/jdk10) on Cygwin, this check fails.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189955
>> WebRev:
>> http://cr.openjdk.java.net/~ihse/JDK-8189955-configuration-validation-is-broken-for-some-paths/webrev.01
>>
>> /Magnus
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189955 Configuration validation is broken for some types of paths

Erik Joelsson
Hello,

On 2017-10-26 10:56, Magnus Ihse Bursie wrote:

>
> On 2017-10-26 10:09, Erik Joelsson wrote:
>> I see why this huge check becomes necessary for a valid comparison,
>> but it's quite a bit of extra complexity added as well. I was worried
>> about the extra overhead on an already slow platform so I took the
>> time to measure. While it's certainly measurable it's hardly
>> noticeable. On a fast machine, the difference is around 50-100ms on
>> repeated make invocations (baseline 1.85s). On my slow laptop it's
>> 300-400ms, but the baseline is already over 7 seconds so as said,
>> hardly noticeable.
> The alternative is to drop the check entirely. I did some source
> control archeology now, to figure out the origin of this check. It has
> grown a bit over the years to handle more complex cases, but the
> original test was introduced by me in JDK-8076060, which was the big
> rewrite that created Init.gmk and the current "bootstrapping" process
> of the build.
>
> I can't, at least now, figure out what was the driving need behind
> that check. It might been that I had collected some complaints about
> odd broken setups that "the build system should check for that!!! i've
> wasted hours!!!" but never got a separate bug. Or I might have
> encountered issues myself while developing the patch. Or I just
> thought it was prudent to check.
>
> In any case, I think it's fully viable by now to remove the check in
> its entirety. Do you agree?
>
Looking back there doesn't seem to be a corresponding check previous to
JDK-8076060. The original comment "Sanity check the spec file, so it
matches this source code" indicates a fear of someone trying to use a
spec file from a different source tree. That would certainly have weird
effects, but I don't think the build should be held responsible. The
previous check was pretty cheap so could be argued worth it. This check
has on the other hand caused quite a bit of problems for us and given
the increased complexity necessary for keeping it in there, I would
agree with your conclusion. It's better to just drop it.

>> That said, line 392 is easily expressible using the make filter
>> function instead of going to the shell. All the other shell calls
>> seem to be necessary however.
>>
>> Just thinking some more. Would it be possible to do something like
>> "cd DIR && pwd" for both dirs and just compare the output of that
>> instead? If that works it would seem like way fewer shell calls and
>> simpler logic.
> Unfortunately, that will not present a canonical representation of the
> directory. In cygwin, both /cygdrive/c/cygwin/home/user and /home/user
> is the same directory, but cygwin will not rewrite the path of any of
> them using that formula. :-( So for better or for worse, I think the
> method that has hardened over the years in our configure script is a
> battle-proven way to create a canonical path representation on all
> platforms.
>
Ah, of course, that's the tricky situation to overcome.

/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189955 Configuration validation is broken for some types of paths

Magnus Ihse Bursie
On 2017-10-26 11:09, Erik Joelsson wrote:
> This check has on the other hand caused quite a bit of problems for us
> and given the increased complexity necessary for keeping it in there,
> I would agree with your conclusion. It's better to just drop it.
Updated webrev, which removes the check:

WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8189955-configuration-validation-is-broken-for-some-paths/webrev.02

/Magnus
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189955 Configuration validation is broken for some types of paths

Erik Joelsson
Looks good.

/Erik


On 2017-10-26 12:03, Magnus Ihse Bursie wrote:

> On 2017-10-26 11:09, Erik Joelsson wrote:
>> This check has on the other hand caused quite a bit of problems for
>> us and given the increased complexity necessary for keeping it in
>> there, I would agree with your conclusion. It's better to just drop it.
> Updated webrev, which removes the check:
>
> WebRev:
> http://cr.openjdk.java.net/~ihse/JDK-8189955-configuration-validation-is-broken-for-some-paths/webrev.02
>
> /Magnus

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8189955 Configuration validation is broken for some types of paths

Martin Buchholz-3
In reply to this post by Magnus Ihse Bursie
Not a review, but ... a note that some of us have to live on slow
filesystems with deeply nested paths where canonicalizing paths becomes a
bottleneck.

On Wed, Oct 25, 2017 at 8:03 AM, Magnus Ihse Bursie <
[hidden email]> wrote:

> We are validating that the topdir we're using when running make is the
> same as when we were running configure.
>
> For some circumstances, most typically when using a subdirectory in the
> user's home directory (e.g. ~/jdk10) on Cygwin, this check fails.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189955
> WebRev: http://cr.openjdk.java.net/~ihse/JDK-8189955-configuration-
> validation-is-broken-for-some-paths/webrev.01
>
> /Magnus
>
>