Felix,
On 15 Jan 2015, at 07:15, FELIX YANG <felix.y...@oracle.com> wrote:
> Chris,
> excuse me, I'm not a reviewer but have some questions.
All comments and questions are welcome.
> at line 209, classes is actually trying to resolve a path like
> "c:\jdk1.9.0\modules", which never exists in JDK directory. It should be
> "c:\jdk1.9.0\lib\modules”.
The 'jdk/modules' directory will only exist when running with an exploded
build. It is useful to be able to run tests against both exploded builds and
images.
> 207 if (home.resolve("lib").toFile().exists()) {
> 208 // either an exploded build or an image
The check for 'lib' is not strictly necessary, I will remove it, as it is
confusing.
> 209 File classes = home.resolve("modules").toFile();
>
> 210 if (classes.isDirectory()) {
> 211 return Stream.of(classes.toPath());
> 212 } else {
> 213 return JrtPaths();
> 214 }
>
> The test works because it always go to JrtPaths method. The jimages have been
> mounted at initializing of Jrt file system.
When running with a binary image, then yes it will always use the JRT code path.
> There may be two issues here:
> 1. the naming
> JrtPaths doesn't follow common rules. Is it more suitable with j
> rtPaths()?
I’ll make this change before pushing.
> 2. Even when changing classes to "lib/modules", it is still probably unable
> to process class files. I tried last code and failed.
>
> File classes = home.resolve("lib/modules").toFile();
‘lib/modules’ is not a path that is search for classes by the runtime. Previous
comments should have already covered the requirement for ‘modules’
Thanks for looking at this felix.,
-Chris.
> Because, com.sun.tools.jdeps.ClassFileReader hasn't been upgraded to
> correctly handle Jrt image files.
>
> So this logic is either unnecessary (as stated above, the jimages have been
> mounted at initializing of Jrt file system) or unable to work now.
>
> Thanks,
>
> -Felix
> On 1/15/2015 12:31 AM, Chris Hegarty wrote:
>> On 14/01/15 16:18, Mandy Chung wrote:
>>> On 1/14/2015 8:04 AM, Chris Hegarty wrote:
>>>>
>>>> http://cr.openjdk.java.net/~chegar/8061297/webrev.01/webrev/
>>>
>>> Looks okay to me.
>>>
>>> In CallerSensitiveFinder.java line 57, 80, 137 - not sure if you were
>>> thinking to pass "pool" to the CallerSensitiveFinder constructor. Looks
>>> like some editing you meant to cleanup.
>>>
>>> nit: line 76 a space between "classes" and "=="
>>> line 207 can be removed.
>>
>> D'oh! Fixed both of these. Webrev updated in place.
>>
>> -Chris.
>>
>>> Mandy
>>>
>>>
>
