RFR: 8172309: classpath wildcards code does not support --class-path

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

RFR: 8172309: classpath wildcards code does not support --class-path

Henry Jen
Hi,

Please review the webrev[1], the fix is to ensure —class-path and —class-path= is processed correctly to expand wildcard. Changes are made in jdk repo. However, test case to verify the bug fix is in langtool repo.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
[2] https://bugs.openjdk.java.net/browse/JDK-8172309
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8172309: classpath wildcards code does not support --class-path

Kumar Srinivasan

Hi Henry,

Looks ok to me. Thanks for making this change.

Kumar

> Hi,
>
> Please review the webrev[1], the fix is to ensure —class-path and —class-path= is processed correctly to expand wildcard. Changes are made in jdk repo. However, test case to verify the bug fix is in langtool repo.
>
> Cheers,
> Henry
>
> [1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
> [2] https://bugs.openjdk.java.net/browse/JDK-8172309

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8172309: classpath wildcards code does not support --class-path

Jonathan Gibbons
Henry,

You've changed old test cases into new test cases, thereby eliminating
the old cases, which is not so good.

You should be adding new test cases, but changing old ones.

-- Jon


On 01/26/2017 02:31 PM, Kumar Srinivasan wrote:

>
> Hi Henry,
>
> Looks ok to me. Thanks for making this change.
>
> Kumar
>
>> Hi,
>>
>> Please review the webrev[1], the fix is to ensure —class-path and
>> —class-path= is processed correctly to expand wildcard. Changes are
>> made in jdk repo. However, test case to verify the bug fix is in
>> langtool repo.
>>
>> Cheers,
>> Henry
>>
>> [1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
>> [2] https://bugs.openjdk.java.net/browse/JDK-8172309
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8172309: classpath wildcards code does not support --class-path

Henry Jen
> On Jan 26, 2017, at 4:09 PM, Jonathan Gibbons <[hidden email]> wrote:
>
> Henry,
>
> You've changed old test cases into new test cases, thereby eliminating the old cases, which is not so good.
>
> You should be adding new test cases, but changing old ones.
>

I am not sure, I believe all wild-card cases still tested, just that we have different variety of calling -cp. Of course, it would be nice to test all cases with all 4 flavors of -cp, but I don’t think that’s necessary as I think the test coverage is the same.

Anyway, I could be wrong. If you feel strong about this, I can redo it. But that may takes more time to digest what the test cases are really for.

Cheers,
Henry


> -- Jon
>
>
> On 01/26/2017 02:31 PM, Kumar Srinivasan wrote:
>>
>> Hi Henry,
>>
>> Looks ok to me. Thanks for making this change.
>>
>> Kumar
>>
>>> Hi,
>>>
>>> Please review the webrev[1], the fix is to ensure —class-path and —class-path= is processed correctly to expand wildcard. Changes are made in jdk repo. However, test case to verify the bug fix is in langtool repo.
>>>
>>> Cheers,
>>> Henry
>>>
>>> [1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8172309
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8172309: classpath wildcards code does not support --class-path

Henry Jen
How about following patch instead for langtools repo?

diff -r ef142ac9824e test/tools/javac/Paths/wcMineField.sh
--- a/test/tools/javac/Paths/wcMineField.sh     Thu Jan 26 16:53:56 2017 -0800
+++ b/test/tools/javac/Paths/wcMineField.sh     Fri Jan 27 13:10:12 2017 -0800
@@ -26,7 +26,7 @@
 #
 # @test
 # @summary Test classpath wildcards for javac and java -classpath option.
-# @bug 6268383
+# @bug 6268383 8172309
 # @run shell/timeout=600 wcMineField.sh

 # To run this test manually, simply do ./wcMineField.sh
@@ -186,6 +186,8 @@
 Failure "$javac" ${TESTTOOLVMOPTS} -classpath "GooJar/*${PS}." Main1.java
 Success "$javac" ${TESTTOOLVMOPTS} -cp "GooJar/SubDir/*" Main1.java
 Success "$javac" ${TESTTOOLVMOPTS} -classpath "GooJar/SubDir/*" Main1.java
+Success "$javac" ${TESTTOOLVMOPTS} --class-path "GooJar/SubDir/*" Main1.java
+Success "$javac" ${TESTTOOLVMOPTS} --class-path="GooJar/SubDir/*" Main1.java
 #Same with launcher. Should not load jar in subdirectories unless specified
 Failure "$java" ${TESTVMOPTS}  -classpath "GooJar/*${PS}." Main1
 Success "$java" ${TESTVMOPTS}  -classpath "GooJar/SubDir/*${PS}." Main1

Cheers,
Henry


> On Jan 26, 2017, at 6:01 PM, Henry Jen <[hidden email]> wrote:
>
>> On Jan 26, 2017, at 4:09 PM, Jonathan Gibbons <[hidden email]> wrote:
>>
>> Henry,
>>
>> You've changed old test cases into new test cases, thereby eliminating the old cases, which is not so good.
>>
>> You should be adding new test cases, but changing old ones.
>>
>
> I am not sure, I believe all wild-card cases still tested, just that we have different variety of calling -cp. Of course, it would be nice to test all cases with all 4 flavors of -cp, but I don’t think that’s necessary as I think the test coverage is the same.
>
> Anyway, I could be wrong. If you feel strong about this, I can redo it. But that may takes more time to digest what the test cases are really for.
>
> Cheers,
> Henry
>
>
>> -- Jon
>>
>>
>> On 01/26/2017 02:31 PM, Kumar Srinivasan wrote:
>>>
>>> Hi Henry,
>>>
>>> Looks ok to me. Thanks for making this change.
>>>
>>> Kumar
>>>
>>>> Hi,
>>>>
>>>> Please review the webrev[1], the fix is to ensure —class-path and —class-path= is processed correctly to expand wildcard. Changes are made in jdk repo. However, test case to verify the bug fix is in langtool repo.
>>>>
>>>> Cheers,
>>>> Henry
>>>>
>>>> [1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8172309
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8172309: classpath wildcards code does not support --class-path

Jonathan Gibbons
OK

-- Jon

On 01/27/2017 01:12 PM, Henry Jen wrote:

> How about following patch instead for langtools repo?
>
> diff -r ef142ac9824e test/tools/javac/Paths/wcMineField.sh
> --- a/test/tools/javac/Paths/wcMineField.sh     Thu Jan 26 16:53:56 2017 -0800
> +++ b/test/tools/javac/Paths/wcMineField.sh     Fri Jan 27 13:10:12 2017 -0800
> @@ -26,7 +26,7 @@
>   #
>   # @test
>   # @summary Test classpath wildcards for javac and java -classpath option.
> -# @bug 6268383
> +# @bug 6268383 8172309
>   # @run shell/timeout=600 wcMineField.sh
>
>   # To run this test manually, simply do ./wcMineField.sh
> @@ -186,6 +186,8 @@
>   Failure "$javac" ${TESTTOOLVMOPTS} -classpath "GooJar/*${PS}." Main1.java
>   Success "$javac" ${TESTTOOLVMOPTS} -cp "GooJar/SubDir/*" Main1.java
>   Success "$javac" ${TESTTOOLVMOPTS} -classpath "GooJar/SubDir/*" Main1.java
> +Success "$javac" ${TESTTOOLVMOPTS} --class-path "GooJar/SubDir/*" Main1.java
> +Success "$javac" ${TESTTOOLVMOPTS} --class-path="GooJar/SubDir/*" Main1.java
>   #Same with launcher. Should not load jar in subdirectories unless specified
>   Failure "$java" ${TESTVMOPTS}  -classpath "GooJar/*${PS}." Main1
>   Success "$java" ${TESTVMOPTS}  -classpath "GooJar/SubDir/*${PS}." Main1
>
> Cheers,
> Henry
>
>
>> On Jan 26, 2017, at 6:01 PM, Henry Jen <[hidden email]> wrote:
>>
>>> On Jan 26, 2017, at 4:09 PM, Jonathan Gibbons <[hidden email]> wrote:
>>>
>>> Henry,
>>>
>>> You've changed old test cases into new test cases, thereby eliminating the old cases, which is not so good.
>>>
>>> You should be adding new test cases, but changing old ones.
>>>
>> I am not sure, I believe all wild-card cases still tested, just that we have different variety of calling -cp. Of course, it would be nice to test all cases with all 4 flavors of -cp, but I don’t think that’s necessary as I think the test coverage is the same.
>>
>> Anyway, I could be wrong. If you feel strong about this, I can redo it. But that may takes more time to digest what the test cases are really for.
>>
>> Cheers,
>> Henry
>>
>>
>>> -- Jon
>>>
>>>
>>> On 01/26/2017 02:31 PM, Kumar Srinivasan wrote:
>>>> Hi Henry,
>>>>
>>>> Looks ok to me. Thanks for making this change.
>>>>
>>>> Kumar
>>>>
>>>>> Hi,
>>>>>
>>>>> Please review the webrev[1], the fix is to ensure —class-path and —class-path= is processed correctly to expand wildcard. Changes are made in jdk repo. However, test case to verify the bug fix is in langtool repo.
>>>>>
>>>>> Cheers,
>>>>> Henry
>>>>>
>>>>> [1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
>>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8172309

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8172309: classpath wildcards code does not support --class-path

Martin Buchholz-3
In reply to this post by Henry Jen
I would try to avoid having the wildcard code know about the various ways of specifying classpath.  We now have 3 different classpath flags - can they be canonicalized into one form before wildcard handling?

On Wed, Jan 25, 2017 at 1:23 PM, Henry Jen <[hidden email]> wrote:
Hi,

Please review the webrev[1], the fix is to ensure —class-path and —class-path= is processed correctly to expand wildcard. Changes are made in jdk repo. However, test case to verify the bug fix is in langtool repo.

Cheers,
Henry

[1] http://cr.openjdk.java.net/~henryjen/jdk9/8172309/0/
[2] https://bugs.openjdk.java.net/browse/JDK-8172309

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8172309: classpath wildcards code does not support --class-path

Martin Buchholz-3
In reply to this post by Henry Jen
Can we have tests in sets of threes, checking -cp, -classpath, --class-path (and perhaps also CLASSPATH environment)

--- a/test/tools/launcher/ClassPathWildCard.sh
+++ b/test/tools/launcher/ClassPathWildCard.sh
@@ -125,7 +125,7 @@
   CheckFail TestA
 
   rm -f TestB${OUTEXT}
-  $JAVA${variant} -classpath JarDir/"*"$NOOP TestB || exit 1
+  $JAVA${variant} -cp JarDir/"*"$NOOP TestB || exit 1
   CheckFail TestB
 
 
@@ -134,11 +134,11 @@
   cp TestD/*.class JarDir
 
   rm -f TestC${OUTEXT}
-  $JAVA${variant} -classpath JarDir${PATHSEP}JarDir/"*"$NOOP TestC || exit 1
+  $JAVA${variant} --class-path JarDir${PATHSEP}JarDir/"*"$NOOP TestC || exit 1
   CheckFail TestC
 
   rm -f TestD${OUTEXT}
-  $JAVA${variant} -classpath JarDir${PATHSEP}JarDir/"*"$NOOP TestD || exit 1
+  $JAVA${variant} --class-path=JarDir${PATHSEP}JarDir/"*"$NOOP TestD || exit 1
   CheckFail TestD

Loading...