Services/configuration/context webrevs

[Paul Sandoz]

On Jul 5, 2012, at 5:13 PM, Mandy Chung wrote:
>>> 
>> Well spotted. Hmm... need to put my thinking cap on and investigate!
>> 
>> A service provider module choice from the perspective of the resolver is the same as a root module choice. I don't currently see how the above case is different from:
>> 
>>   modulex at 1.0  {
>>     permits y;
>>   }
>> 
>>   moduley at 1.0  {
>>     requires x;
>>   }
>> 
>>   modulez at 1.0  {
>>     requires optional x;
>>   }
>> 
>> when the order of resolution is z, y, then x. I have attached a simple test case that resolves using the above modules.
> 
> The attachment is missing.  

That attachment got stripped by the mail server :-(  i will resend to you privately.


> The resulting solution would be z at 1.0 only which is correct.  

No, because module y is a required root module; x y and z will be compiled at the same time by javac (would be clearer if the attachment was present!).

For me the solution is:

     [exec] | Configured for [x at 1.0, y at 1.0, z at 1.0]

If I rename module z to y and module y to z the resolution will fail:

     [exec] error: Cannot resolve module dependencies using Jigsaw module resolver
     [exec]   [x@=1.0, y@=1.0, z@=1.0]: Cannot resolve


> In the example I showed above, the service provider module P requires a module X at 1.0 that doesn't satisfy the constraints in the previously resolved modules.

I think the issue is more general. A service provider dependency is equivalent to an optional root dependency.


>  I would consider that module P will not be included as its dependency X at 1.0 fails to be resolved.
> 

OK.


>> So i think we have a general bug and require either:
>> 
>> 1) a validation step on the set of resolved modules; or
>> 
>> 2) the compile and install time linkers do the right thing.
>> 
>> Which one we choose depends on whether we consider the above a valid solution or not. Can x be present but the linker knows that z should not link to x? i.e. as if the "resolve optional x" gets erased by the resolver by adapting the module info of z. It seems reasonable.
> 
> In your example, there is no service provider module in the picture.  The current implementation should resolve z with no x module in the solution.   Just to make sure this is the behavior you're thinking about - if x is required by a service provider module p being resolved in phase 1 due to  (z but not x is one of the resolved module in phase 0),  would the service provider module requiring x be included in the solution?

Yes i am beginning to think so, x would be included in the configuration but z would not be linked to x. To me that makes sense because of the "requires optional x" in z. 

Why should, for a failed resolution of x in z, the optionality of x in z affect other modules that can resolve a dependency on x?

I realize that option 1) above does not make sense given the optionality.


>>> 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.
>> Can we reach agreement on a code convention for Jigsaw that is supported by the NetBeans formatting options? then i can just hit Ctrl+Shift+F :-) i fully admit i am lazy!
> 
> One inconsistency I know of is the for-each statement "for (T a: iterable)" vs "for (T a : iterable)" whether a space is needed after ":".   Other than that the coding convention for jigsaw is fairly consistently applied in the org.openjdk.jigsaw source e.g. 4-space indentation, and line breaks.  Is there any other inconsistency you observe?
> 

It is not so much the existing inconsistency, i don't know to what extent the code is or is not consistent, but the effort required to not be inconsistent and the effort required to review code and point out the inconsistencies. It's something a tool like NBs can do. We just need agreement on the style and i can come up with a NBs formatting configuration.


>> I don't have any strong preference on the style, I just want to be consistent and do it quickly to avoid us spending unnecessary cycles on these aspects.
>> 
>> 
>>> L458: typo 'dependencey'
>>> L533: if serviceInterface was resolved in previous phases, it can skip and process the next service interface.
>>> 
>> I was wondering about this. A service provider dependency that failed to resolve in phase n might be resolved later in a phase>  n. So i left that as is for now. Also I did not bother to check if a service provider dependency was already resolved since it is done in the resolve method and i wanted the tracing to show what was going on.
> 
> Is it possible to have a service provider dependency that failed to resolve in phase n but successfully resolved later in phase > n? During resolution, the module libraries should be locked and there should be no change to the list of available modules.

Good point, i am struggling to come up with an example. 


>  In any case, I think it's fine not to check that and the resolver/library would do better caching and avoid reading the same set of module-info multiple times.

> 
>> 
>>> 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.
>> Yes, i want to flesh out the _Configurator related tests to support services, that should make redundant some of the shell-script-based tests, those tests can then focus on the use of ServiceLoader and we can probably consolidate.
>> 
>> I have a preference to do that in another webrev, since it is gonna result in some shuffling around is that OK?
>> 
>> Also i have a preference to do that after the refactoring changes (which somewhat cleans  up that test code), but realize that could hold things up.
> 
> I see and you have several changes lined up.  Would you consider to include the tests update in part of the refactoring webrev?   Test cases are sometimes helpful for code review.
> 

Yes. To keep things easier for everyone else it is probably best if keep work on the tip and pull it up stream as required.

Paul.