Requires optional permits <was> Re: Service provider module resolution
David Holmes
david.holmes at oracle.com
Tue Jul 10 18:53:00 PDT 2012
On 11/07/2012 2:02 AM, Paul Sandoz wrote:
> 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.
Are you only referring to an optional dependency on a module which in
turns has a permits clause? I would not in general expect to have to
resolve an optional dependency at compile time.
David
------
> 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
>>>
>>
>
More information about the jigsaw-dev
mailing list