Requires optional permits <was> Re: Service provider module resolution

[Paul Sandoz]

Hi,

Based off the previous webrev is the following one that fixes requires optional with permits:

  http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/requires-optional-permits-webrev/

The two following tests produce the same configuration result where x is not linked to z.

 693         new Test("permits-requires-optional-first", true, "z at 1") {
 694             void init(MockLibrary mlib) {
 695                 mlib.add(module("z at 1")
 696                         .requiresOptional("x")
 697                         .requires("y"))
 698                     .add(module("y at 1").requires("x"))
 699                     .add(module("x at 1").permits("y"));
 700             }
 701             void ref(ConfigurationBuilder cfbd) {
 702                 cfbd.add(context("z at 1").remote("+y"))
 703                     .add(context("y at 1").remote("+x"))
 704                     .add(context("x at 1"));
 705             }
 706         };
 707 
 708         new Test("permits-requires-optional-second", true, "z at 1") {
 709             void init(MockLibrary mlib) {
 710                 mlib.add(module("z at 1")
 711                         .requires("y")
 712                         .requiresOptional("x"))
 713                     .add(module("y at 1").requires("x"))
 714                     .add(module("x at 1").permits("y"));
 715             }
 716             void ref(ConfigurationBuilder cfbd) {
 717                 cfbd.add(context("z at 1").remote("+y"))
 718                     .add(context("y at 1").remote("+x"))
 719                     .add(context("x at 1"));
 720             }
 721         };

--

IIUC from reading the Jigsaw Big Picture document an optional dependency must resolve at compile time, if such a dependency is not resolvable due to permits or otherwise no module being present then resolution should fail. 

Currently this is not the case. Even the most simple "requires optional foo" where module "foo" does not exist will not resolute in resolution failure at compilation.

This implies:

1) Compile-time resolution should fail e.g. with some post-validation step in PathLinker; and

2) Install-time resolution should pass with correct linking (as implemented in the webrev).

Thoughts?

Paul.

On Jul 9, 2012, at 5:52 PM, Paul Sandoz wrote:

> Hi,
> 
> The following webrev fixes the nits. Many service-based tests are added to _Configurator.java, which makes most of the service-based *.sh tests redundant so those redundant ones removed. I added a runtime service-based test [*] that ensures correct behavior for all three ServiceLoader methods.
> 
>  http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.3/
> 
> --
> 
> I also added tests that show the permits behavior is general and not specific to services:
> 
> 693         /*
> 694          * The following two tests re-produce an issue with permits
> 695          * and requires optional. Depending on the order of resolution
> 696          * a module, x, may be linked to a module, z, that is not
> 697          * permitted to do so.
> 698          * 
> 699          * "requires optional" should not cause a resolution failure instead
> 700          * linking should not be performed for a "requires optional x" where
> 701          * x does not permit the requiring module.
> 702          */
> 703         
> 704         new Test("permits-requires-optional-permissive-linking", true, "z at 1") {
> 705             void init(MockLibrary mlib) {
> 706                 mlib.add(module("z at 1")
> 707                         .requiresOptional("x")
> 708                         .requires("y"))
> 709                     .add(module("y at 1").requires("x"))
> 710                     .add(module("x at 1").permits("y"));
> 711             }
> 712             void ref(ConfigurationBuilder cfbd) {
> 713                 cfbd.add(context("z at 1").remote("+x", "+y"))
> 714                     .add(context("y at 1").remote("+x"))
> 715                     .add(context("x at 1"));
> 716                 // The result should be
> 717 //                cfbd.add(context("z at 1").remote(""+y"))
> 718 //                    .add(context("y at 1").remote("+x"))
> 719 //                    .add(context("x at 1"));
> 720             }
> 721         };
> 722 
> 723         new Test("permits-requires-optional-failure", false, "z at 1") {
> 724             void init(MockLibrary mlib) {
> 725                 mlib.add(module("z at 1")
> 726                         .requires("y")
> 727                         .requiresOptional("x"))
> 728                     .add(module("y at 1").requires("x"))
> 729                     .add(module("x at 1").permits("y"));
> 730             }
> 731             void ref(ConfigurationBuilder cfbd) {
> 732                 cfbd.add(context("z at 1").remote("+x", "+y"))
> 733                     .add(context("y at 1").remote("+x"))
> 734                     .add(context("x at 1"));
> 735                 // The result should be
> 736 //                cfbd.add(context("z at 1").remote(""+y"))
> 737 //                    .add(context("y at 1").remote("+x"))
> 738 //                    .add(context("x at 1"));
> 739             }
> 
> I will work on fixing that this week.
> 
> Paul.
> 
> [*] I held back on the temptation for now to convert to testng! jtreg is rather smart but IMHO we really need tests we can edit, debug and run within the IDE.
> 
> On Jul 5, 2012, at 5:55 AM, Mandy Chung wrote:
> 
>> On 7/1/2012 2:34 AM, Paul Sandoz wrote:
>>> Hi,
>>> 
>>> I thought it would help to consolidate the service/configuration changes into one email.
>> 
>> Yes, that helps.  Thanks.
>> 
>>> Service provider module resolution
>>> -----------------------------------------------
>>> 
>>> http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/
>>> 
>>> - "permits" clauses do not affect service provider module resolution
>>> 
>>> - service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.
>>> 
>>> I understand that on and off list there is reasonable agreement to go forward with this latter approach.
>> 
>> Doing the service provider dependency resolution in phases would make it a little easier to trace and diagnose.  Each phase may add new modules to the graph (service provider module and its dependencies) and looks like it requires to re-validate if a module is required optionally but failed to be chosen in phase 0. For example,
>> 
>> M
>>  requires optional X;
>>  requires service S;
>> 
>> P
>>  requires X @ 1.0
>>  provides service S with S1;
>> 
>> X @ 1.0
>>  permits P
>> 
>> In this case, phase 0 will chose M only.  Phase 1 choses P and X at 1.0 and the solution includes M, P, and X at 1.0.  However, X at 1.0doesn't permit M but it's linked with M in your current implementation.
>> Nits w.r.t. the change:
>> Resolver.javaL221: toString() call is not needed
>> L446: best to follow current convention to put the first parameter
>>     next to the signature L445 and line break at the second parameter.
>> L450: 4-space indent would be good.
>> L458: typo 'dependencey'
>> L533: if serviceInterface was resolved in previous phases, it can skip and process the next service interface.
>> 
>> many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
>> and permits is ignored.
>> 
>> I think it would be useful to add a few test cases that service provider module pulls in new modules and how this fix changes the solution - e.g. a case when a provider module is not included due to a module selected in phase 0 doesn't satisfy its dependency.
>> 
>> Mandy
>> 
>