Review Request: JDK-8171418: Remove jdeps internal --include-system-modules option

[Daniel Fuchs]

On 19/12/16 19:03, Mandy Chung wrote:
>
>> On Dec 19, 2016, at 10:26 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> On 19/12/16 18:06, Mandy Chung wrote:
>>> Updated webrev:
>>>
>>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171418/webrev.01
>>
>> Looks good to me.
>> A related question is whether the tokens ALL-SYSTEM, ALL-DEFAULT,
>> ALL-MODULE-PATH, should appear in the jdeps help message?
>>
>
> I’ll leave it as is.  Clean it up when we do another pass on the help message.

OK. Looks good then!

-- daniel

>
> Mandy
>
>> best regards,
>>
>> -- daniel
>>
>>>
>>>> On Dec 19, 2016, at 9:54 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>>>
>>>> Hi Mandy,
>>>>
>>>> DepsAnalyzer.java:
>>>>
>>>> 232         Collection<Module> modules = configuration.getModules().values();
>>>>
>>>> I don't see where modules is used in this method.
>>>> Should this line be removed?
>>>>
>>>
>>> Removed. Lance also caught that.
>>>
>>>>
>>>> 163         if (filter.requiresFilter().isEmpty()) {
>>>> 164             return archives.stream()
>>>> 165                 .filter(this::include)
>>>> ...
>>>> 168         } else {
>>>> 169             // use the archives that have dependences and not specified in --require
>>>> 170             return archives.stream()
>>>> 171                 .filter(source -> !filter.requiresFilter().contains(source.getName()))
>>>> 172                 .filter(this::include)
>>>> ...
>>>>
>>>> and later:
>>>>
>>>> 266     public boolean include(Archive source) {
>>>> 267         Module module = source.getModule();
>>>> 268         // skip system module by default
>>>> 269         return  !module.isSystem()
>>>> 270                     || configuration.rootModules().contains(source)
>>>> 271                     || filter.requiresFilter().contains(module.name());
>>>> 272     }
>>>>
>>>>
>>>> If I look at how JdepsFilter::includes was implemented, and given that
>>>> filter.requiresFilter().contains(module.name()) will always be false
>>>> if we come from line 165, or from line 172 then I would suggest to
>>>> remove line 271: this would ensure that the call at line 95 does
>>>> the same than before.
>>>>
>>>
>>> That’s a good suggestion.
>>>
>>> Mandy
>>>
>>>>
>>>> On 19/12/16 05:42, Mandy Chung wrote:
>>>>> Webrev at:
>>>>>  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8171418/webrev.00
>>>>>
>>>>> --include-system-modules option was a temporary option added in an early stage of developement.  Time to remove it as a clean up.
>>>>>
>>>>> Mandy
>>>>>
>>>>
>>>
>>
>