You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by Stefan Bodewig <bo...@apache.org> on 2018/02/13 17:50:00 UTC
Need a Second Pair of Eyes
Hi all
I'm currently reviewing the big change that introduced the regression in
1.10.2 as we may have overlooked more than this issue. It is a *big*
change and so it is taking time.
While looking at the hunk starting at
https://github.com/apache/ant/commit/b7d1e9bde44cb8e5233d6e70bb96e14cbb2f3e2d#diff-3cabe19d89e908d993d999100d888b6eL256
I think the buildup of the path is now backwards.
The original code would add all elements of dependClasspath to the path
that are not members of destPath to the new Path p. Unless I am mistaken
the Difference created in the new code will in addition retain all
elements of destPath that are not in dependClasspath. So we get all
elements that are in exactly one of the two - which is more than we
would have with the old code unless destPath is a subset of
dependClasspath.
Does anybody else read it the same way? Or can tell me I'm wrong?
Unfortunately our test coverage for the depend task is so close to
non-existent that we can't tell from the tests whether we've broken
anything.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Need a Second Pair of Eyes
Posted by Jaikiran Pai <ja...@gmail.com>.
I forgot to add that, as you note, as a result of the change, the new
created path will retain all elements from destPath that aren't in
dependClassPath, which is unlike the behaviour before the change.
-Jaikiran
On 14/02/18 9:59 AM, Jaikiran Pai wrote:
> Hi Stefan,
>
> You are right. The change introduces a different behaviour than what
> was there before. Just to be extra sure that we indeed are reading it
> right, I added a (dummy) test case in my personal repo, which compares
> the previous logic and the new logic and it does shows that the change
> indeed introduces a different behaviour.
>
> [1]
> https://github.com/jaikiran/ant/commit/f57b9d5fbca6e9648695bc9d37a27000c0b4aff2#diff-585ed59cf64ef6930e5148094adc322aR21
>
> -Jaikiran
>
>
> On 13/02/18 11:20 PM, Stefan Bodewig wrote:
>> Hi all
>>
>> I'm currently reviewing the big change that introduced the regression in
>> 1.10.2 as we may have overlooked more than this issue. It is a *big*
>> change and so it is taking time.
>>
>> While looking at the hunk starting at
>>
>> https://github.com/apache/ant/commit/b7d1e9bde44cb8e5233d6e70bb96e14cbb2f3e2d#diff-3cabe19d89e908d993d999100d888b6eL256
>>
>>
>> I think the buildup of the path is now backwards.
>>
>> The original code would add all elements of dependClasspath to the path
>> that are not members of destPath to the new Path p. Unless I am mistaken
>> the Difference created in the new code will in addition retain all
>> elements of destPath that are not in dependClasspath. So we get all
>> elements that are in exactly one of the two - which is more than we
>> would have with the old code unless destPath is a subset of
>> dependClasspath.
>>
>> Does anybody else read it the same way? Or can tell me I'm wrong?
>>
>> Unfortunately our test coverage for the depend task is so close to
>> non-existent that we can't tell from the tests whether we've broken
>> anything.
>>
>> Stefan
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
>> For additional commands, e-mail: dev-help@ant.apache.org
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Need a Second Pair of Eyes
Posted by Stefan Bodewig <bo...@apache.org>.
On 2018-02-14, Stefan Bodewig wrote:
> On 2018-02-14, Jaikiran Pai wrote:
>> You are right. The change introduces a different behaviour than what
>> was there before. Just to be extra sure that we indeed are reading it
>> right, I added a (dummy) test case in my personal repo, which compares
>> the previous logic and the new logic and it does shows that the change
>> indeed introduces a different behaviour.
>> [1]
>> https://github.com/jaikiran/ant/commit/f57b9d5fbca6e9648695bc9d37a27000c0b4aff2#diff-585ed59cf64ef6930e5148094adc322aR21
> Many thanks, Jaikiran, so I'm going to revert that hunk.
I've used a third approach in commit 1fbf712 and checked it in a way
similar to your test.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Need a Second Pair of Eyes
Posted by Stefan Bodewig <bo...@apache.org>.
On 2018-02-14, Jaikiran Pai wrote:
> You are right. The change introduces a different behaviour than what
> was there before. Just to be extra sure that we indeed are reading it
> right, I added a (dummy) test case in my personal repo, which compares
> the previous logic and the new logic and it does shows that the change
> indeed introduces a different behaviour.
> [1]
> https://github.com/jaikiran/ant/commit/f57b9d5fbca6e9648695bc9d37a27000c0b4aff2#diff-585ed59cf64ef6930e5148094adc322aR21
Many thanks, Jaikiran, so I'm going to revert that hunk.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Need a Second Pair of Eyes
Posted by Jaikiran Pai <ja...@gmail.com>.
Hi Stefan,
You are right. The change introduces a different behaviour than what was
there before. Just to be extra sure that we indeed are reading it right,
I added a (dummy) test case in my personal repo, which compares the
previous logic and the new logic and it does shows that the change
indeed introduces a different behaviour.
[1]
https://github.com/jaikiran/ant/commit/f57b9d5fbca6e9648695bc9d37a27000c0b4aff2#diff-585ed59cf64ef6930e5148094adc322aR21
-Jaikiran
On 13/02/18 11:20 PM, Stefan Bodewig wrote:
> Hi all
>
> I'm currently reviewing the big change that introduced the regression in
> 1.10.2 as we may have overlooked more than this issue. It is a *big*
> change and so it is taking time.
>
> While looking at the hunk starting at
>
> https://github.com/apache/ant/commit/b7d1e9bde44cb8e5233d6e70bb96e14cbb2f3e2d#diff-3cabe19d89e908d993d999100d888b6eL256
>
> I think the buildup of the path is now backwards.
>
> The original code would add all elements of dependClasspath to the path
> that are not members of destPath to the new Path p. Unless I am mistaken
> the Difference created in the new code will in addition retain all
> elements of destPath that are not in dependClasspath. So we get all
> elements that are in exactly one of the two - which is more than we
> would have with the old code unless destPath is a subset of
> dependClasspath.
>
> Does anybody else read it the same way? Or can tell me I'm wrong?
>
> Unfortunately our test coverage for the depend task is so close to
> non-existent that we can't tell from the tests whether we've broken
> anything.
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org