Review request on the optional modules support

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Review request on the optional modules support

Mandy Chung
An update on the optional module support [1].

Webrev:
    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/

The note has been updated to include the open questions raised in
the discussion.
    http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html

There is an open question whether the isModulePresent method should
take a version-range argument or other alternative for an application
to support different versions of modules with incompatible change (see [2]).

I propose to get this prototype in the jigsaw repository first
and address the open question next as we would need to get the use
cases to determine what the right API for it.

Summary of Changes:
1. A new @RequireOptionalModule annotation is defined to
    indicate that the annotated type requires the optional module
    to be present when it's referenced; otherwise,
    ModuleNotPresentException would be thrown.

    See java.util.Properties.loadFromXML and other examples in the JDK.

2. Two new methods are added in the java.lang.reflect.Module class
    to test if a module has been resolved and linked with this
    module's context.

    The requireModulePresent(String mn) method checks if
    the given module is present; if not, it throws ModuleNotPresentException.

    isModulePresent(String mn) simply returns true/false
    and it can be used for the fall-back cases if an optioanl module
    is not present.

3. Class.getModule() is modified to lazily set the Module for
    a system class loaded by the VM bootstrap null loader.

    The VM loads the primordial classes directly in the VM
    and also other system classes when the caller is loaded
    by null class loader without calling the library. The
    Class's module field is null in that case.  The Loader
    class is refactored to support the lazily initialization
    of the Class's module.

4. An optional module that doesn't any local class but just
    reexports another module is not currently stored in the
    configuration.  Thus the loader can't find it even if it's
    present. jdk.jaxp is an example that reexports sun.jaxp
    as sun.jaxp exports its internal APIs and permits only
    specific module to use (jaxws) while jdk.jaxp only
    reexports the public APIs.

    I simply added the remote contexts list in the configuration as
    the fast configuration work will further change the layout.
    With another change I made in the SimpleLibrary.StoredConfiguration
    class to add indirection for the context name that reduces
    the config file size [3], the remote contexts list should have
    minimal impact to the config size.

Please also note that I added a "Guideline of Using Optional module"
section in [2] which is intended to illustrate the class loading issue
with optional module but not as a recommendation for the coding practice.
This would be another topic to discuss.

Thanks
Mandy

[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-May/001298.html
[2] http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html
[3] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-June/001349.html

Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Jesse Glick
On 08/04/2011 03:35 PM, Mandy Chung wrote:
> http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/

Parenthetical note: "jdk.jaxp" as a module name seems wrong since JAXP is not specific to the JDK (i.e. JRE + tools); would expect "jre.jaxp". Same for AWT, etc.

> A new @RequireOptionalModule annotation is defined to indicate that the annotated type

"Annotated element" I guess (includes methods); Javadoc needs to be edited.

> http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html

Open question #1 - does it not suffice that @ROM is @Documented?

Open question #3 - a Maven plugin for checking JRE compatibility does something like this: http://mojo.codehaus.org/animal-sniffer/animal-sniffer-annotations/
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Alan Bateman
Jesse Glick wrote:
> On 08/04/2011 03:35 PM, Mandy Chung wrote:
>> http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/
>
> Parenthetical note: "jdk.jaxp" as a module name seems wrong since JAXP
> is not specific to the JDK (i.e. JRE + tools); would expect
> "jre.jaxp". Same for AWT, etc.
>
I wouldn't be concerned about the module names at this point as it's way
too early in the JDK modularization effort and the module graph, names,
etc. are all likely to change.

-Alan.
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Mandy Chung
On 8/4/11 1:25 PM, Alan Bateman wrote:

> Jesse Glick wrote:
>> On 08/04/2011 03:35 PM, Mandy Chung wrote:
>>> http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/
>>
>> Parenthetical note: "jdk.jaxp" as a module name seems wrong since
>> JAXP is not specific to the JDK (i.e. JRE + tools); would expect
>> "jre.jaxp". Same for AWT, etc.
>>
> I wouldn't be concerned about the module names at this point as it's
> way too early in the JDK modularization effort and the module graph,
> names, etc. are all likely to change.

Exactly.  The module names will be changed for example how the
substitution is supported [1].  TBD.

Mandy
[1]
http://openjdk.java.net/projects/jigsaw/doc/draft-java-module-system-requirements-12#substitution
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Mandy Chung
In reply to this post by Jesse Glick
  On 08/04/11 13:18, Jesse Glick wrote:
>> A new @RequireOptionalModule annotation is defined to indicate that
>> the annotated type
>
> "Annotated element" I guess (includes methods); Javadoc needs to be
> edited.
>

Fixed.

>> http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html
>
> Open question #1 - does it not suffice that @ROM is @Documented?

I think @Document + @throws ModuleNotPresentException will suffice.
@ROM implies "@throws ModuleNotPresentException" and it's a good
practice to specify @throws in the javadoc.   I don't see a need to add
a new javadoc tag.  I included the question there to see if anyone thinks
differently.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Alan Bateman
In reply to this post by Mandy Chung
Mandy Chung wrote:
> An update on the optional module support [1].
>
> Webrev:
>    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/
>
> The note has been updated to include the open questions raised in
> the discussion.
>    http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html
I went through the webrev.

It mostly looks okay to me but I suspect that developers might initially
grapple with isModulePresent and requireModulePresent as instance
methods (in time I guess the javadoc will be fleshed out and concepts
such as context can be referenced).

I see you've changed a few of the SecurityManager methods to throw
ModuleNotPresentException but I'm not sure that is really needed. The
alternative is to just have the permission checks fail, as they do now,
when the desktop module is not present.

The updates to java.lang.reflect.Module remind me to ask whether this is
the right package for this class (java.lang, java.lang.reflect or
java.lang.module). I realize there is history here (should Class and
Package be in java.lang.reflect, etc.) but it does seem inconsistent now.

One of the questions posed is whether the compiler can use the
RequireOptionalModule annotations but I would have thought that if there
are any references to types in optional modules then it will require
that they be present at compile time. Maybe this question is only for
the case that reflection is used?

Another question is about handling different versions but that strikes
me as a bit fragile and could potentially get out of sync with the
version constraints in the module-info too.

Otherwise I think the changes are okay and I note that
RequireOptionalModule will need to have to classes element removed once
we don't require the class analyzer in the build.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Mandy Chung
On 8/8/11 8:01 AM, Alan Bateman wrote:

> Mandy Chung wrote:
>> An update on the optional module support [1].
>>
>> Webrev:
>>    
>> http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/
>>
>> The note has been updated to include the open questions raised in
>> the discussion.
>>    http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html
> I went through the webrev.
>
> It mostly looks okay to me but I suspect that developers might
> initially grapple with isModulePresent and requireModulePresent as
> instance methods (in time I guess the javadoc will be fleshed out and
> concepts such as context can be referenced).
>

I think ModuleClassLoader might actually be a more appropriate class
defining these methods.  The downside of it is that it requires to cast
the class loader returned from Class.getClassLoader() to ModuleClassLoader.

> I see you've changed a few of the SecurityManager methods to throw
> ModuleNotPresentException but I'm not sure that is really needed. The
> alternative is to just have the permission checks fail, as they do
> now, when the desktop module is not present.
>

There doesn't seem to have any reason for an application that doesn't
require desktop module to call these methods.  So the question is
whether we want to disallow the use of these methods if desktop module
is not present:
    checkAwtEventQueueAccess()
    checkTopLevelWindow()
    checkSystemClipboardAccess()

It's more of a cleanup on the API.

> The updates to java.lang.reflect.Module remind me to ask whether this
> is the right package for this class (java.lang, java.lang.reflect or
> java.lang.module). I realize there is history here (should Class and
> Package be in java.lang.reflect, etc.) but it does seem inconsistent now.
>

I too wonder about the inconsistency.  I'm making a note of it.

> One of the questions posed is whether the compiler can use the
> RequireOptionalModule annotations but I would have thought that if
> there are any references to types in optional modules then it will
> require that they be present at compile time. Maybe this question is
> only for the case that reflection is used?
>

Roger posted this question in our previous discussion.  At compile time,
the optional module must be present if there is any reference to types
in the optional module.   If the optional method is not carefully
written, at class loading time, the verifier might load types in
optional modules for verification regardless of whether the optional
module is called or not.  The question really comes to whether the
compiler can detect such potential verification error during
compilation.  I talked with Jon Gibbons another day.  The compiler can't
detect that.  Jon and Alex can chime in and explain further.

> Another question is about handling different versions but that strikes
> me as a bit fragile and could potentially get out of sync with the
> version constraints in the module-info too.
>
> Otherwise I think the changes are okay and I note that
> RequireOptionalModule will need to have to classes element removed
> once we don't require the class analyzer in the build.

I may just take it out now since the class analyzer is using the
depconfig file.

Thanks
Mandy
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Jesse Glick
On 08/08/2011 08:33 PM, Mandy Chung wrote:
>> ...should Class and Package be in java.lang.reflect...
>
> I too wonder about the inconsistency.

Do you have any ambition to make java.lang.reflect be an optional module? (This would require Object.getClass() to throw ModuleNotPresentException.) There are two reasons
this might be useful:

1. Secure containers (sandboxes) may prefer that hosted code not even run those limited reflective calls that the security manager would permit.

2. Compilers or analyzers could make more aggressive optimizations or error checks knowing that reflection is unavailable in a given codebase.

A third use case would be for a runtime subset that would permit pure functions to be written - i.e. those guaranteed by form to return consistent results - but this
would require isolating other parts of java.lang as well: Thread/ThreadGroup, System, Runtime, most Object methods, etc. It may be impossible to do this without vetting
bytecode manually.

On a related note - I have an RFE open about @SupportedAnnotationTypes in which one suggested fix is for X.class.getCanonicalName() and similar constructs to be treated
as compile-time constants. It would be odd but I suppose harmless for the definition of CTCs to refer to modules other than java.lang.

> The compiler can't detect [such potential verification errors]

Seems like this is more the domain of FindBugs.
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Alan Bateman
Jesse Glick wrote:
> On 08/08/2011 08:33 PM, Mandy Chung wrote:
>>> ...should Class and Package be in java.lang.reflect...
>>
>> I too wonder about the inconsistency.
>
> Do you have any ambition to make java.lang.reflect be an optional
> module? (This would require Object.getClass() to throw
> ModuleNotPresentException.)
No, there isn't anything in the current requirements for this. It's not
clear to me that putting reflection into its own module would even be
feasible or rather I'm not sure that anything useful could run without
java.lang.Class being present (java.lang.Class needs to loaded early in
the VM startup and it implements several types defined in
java.lang.reflect, etc.).

-Alan

Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Jesse Glick
On 08/09/2011 09:29 AM, Alan Bateman wrote:
> I'm not sure that anything useful could run without java.lang.Class being present

Certainly not. The point would merely be to forbid reflection-related APIs from being referenced from another module unless the java.lang.reflect module was explicitly
imported. But perhaps this is a deeper property of the calling code than can be captured by API signatures - the Java language and bytecode definition includes inherently
reflective operations like instanceof.
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Sean Mullan
In reply to this post by Mandy Chung
Hi Mandy,

I was just reviewing the design note (not the webrev) and I had a couple of
minor comments -

Optional Module

first sentence, suggest:

s/referred as optional module/referred to as the optional module

2. ModuleNotPresentException

"if it's absent, a ModuleNotPresentException as in the "foo" module example
given above."

I could not find the "foo" module example above. Do you mean the one at the end
of the document?

JDK Optional Dependenceies
1. APIs that throw ModuleNotPresentException

Are those really the only APIs that require optional modules? Aren't there also
internal APIs that have optional dependencies? Don't they need to be updated as
well?

General Question

I'm not sure if this question makes sense, but what happens in the future if
JAXP is required by the base module? Would it still make sense to annotate APIs
in the module that require it as an optional module?

--Sean


On 8/4/11 3:35 PM, Mandy Chung wrote:

> An update on the optional module support [1].
>
> Webrev:
> http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/
>
> The note has been updated to include the open questions raised in the
> discussion. http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html
>
> There is an open question whether the isModulePresent method should take a
> version-range argument or other alternative for an application to support
> different versions of modules with incompatible change (see [2]).
>
> I propose to get this prototype in the jigsaw repository first and address
> the open question next as we would need to get the use cases to determine
> what the right API for it.
>
> Summary of Changes: 1. A new @RequireOptionalModule annotation is defined to
> indicate that the annotated type requires the optional module to be present
> when it's referenced; otherwise, ModuleNotPresentException would be thrown.
>
> See java.util.Properties.loadFromXML and other examples in the JDK.
>
> 2. Two new methods are added in the java.lang.reflect.Module class to test if
> a module has been resolved and linked with this module's context.
>
> The requireModulePresent(String mn) method checks if the given module is
> present; if not, it throws ModuleNotPresentException.
>
> isModulePresent(String mn) simply returns true/false and it can be used for
> the fall-back cases if an optioanl module is not present.
>
> 3. Class.getModule() is modified to lazily set the Module for a system class
> loaded by the VM bootstrap null loader.
>
> The VM loads the primordial classes directly in the VM and also other system
> classes when the caller is loaded by null class loader without calling the
> library. The Class's module field is null in that case.  The Loader class is
> refactored to support the lazily initialization of the Class's module.
>
> 4. An optional module that doesn't any local class but just reexports another
> module is not currently stored in the configuration.  Thus the loader can't
> find it even if it's present. jdk.jaxp is an example that reexports sun.jaxp
> as sun.jaxp exports its internal APIs and permits only specific module to use
> (jaxws) while jdk.jaxp only reexports the public APIs.
>
> I simply added the remote contexts list in the configuration as the fast
> configuration work will further change the layout. With another change I made
> in the SimpleLibrary.StoredConfiguration class to add indirection for the
> context name that reduces the config file size [3], the remote contexts list
> should have minimal impact to the config size.
>
> Please also note that I added a "Guideline of Using Optional module" section
> in [2] which is intended to illustrate the class loading issue with optional
> module but not as a recommendation for the coding practice. This would be
> another topic to discuss.
>
> Thanks Mandy
>
> [1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-May/001298.html 
> [2] http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html [3]
> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-June/001349.html
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Mandy Chung
  On 08/09/11 10:27, Sean Mullan wrote:

> Hi Mandy,
>
> I was just reviewing the design note (not the webrev) and I had a couple of
> minor comments -
>
> Optional Module
>
> first sentence, suggest:
>
> s/referred as optional module/referred to as the optional module
>

Fixed.

> 2. ModuleNotPresentException
>
> "if it's absent, a ModuleNotPresentException as in the "foo" module example
> given above."
>
> I could not find the "foo" module example above. Do you mean the one at the end
> of the document?
>

I moved the section and the example to the end but forgot to update the
statement.  Sorry about that.  Will fix that.

> JDK Optional Dependenceies
> 1. APIs that throw ModuleNotPresentException
>
> Are those really the only APIs that require optional modules?

No. They are the ones I identified that should throw
ModuleNotPresentException when they are called but the optional module
is missing.  I also suggest that
javax.management.remote.JMXConnector.connect() and/or
RMIConnector.connect should be updated to throw
ModuleNotPresentException as described in the optional.html note.

> Aren't there also
> internal APIs that have optional dependencies? Don't they need to be updated as
> well?
>

There are APIs that have optional dependencies but they are designed to
work in the absence of the optional module as well.

There are different kinds of optional dependencies.
1. APIs that throw ModuleNotPresentException

It's the caller's responsibility to make sure that the optional module
is installed.  The APIs have to detect if the optional module exists
before referencing any type in it.

2. Optional dependencies satisfied by its input arguments

These APIs do not require the optional module to be present unless its
input arguments specify it and are constructed in in the presence of the
optional module.  For example,  java.text.Bidi references
java.awt.TextAttribute.  So the base module should have an optional
dependency on "desktop" module (it's one clean up task to fix
module-info.java on my todo list).   The desktop module is guaranteed to
be present when it hits the code path to reference the
java.awt.TextAttribute class. In this case, the application should be
configured properly.

3. APIs that fall back to the default implementation when the optional
module is absent

ResourceBundle is a good example that falls back to use the default
locale.  Security provider is another example where the default provider
and the interface are in one module whereas other provider
implementation is in another module (note that this doesn't use the
java.util.ServiceLoader mechanism). In any case, one module has an
optional dependency on another but it will work both ways - the optional
module is present or absent.

I see this possibly be adjusted to use the module-aware service loader
mechanism [1].  In any case, this doesn't throw MNPE.

> General Question
>
> I'm not sure if this question makes sense, but what happens in the future if
> JAXP is required by the base module?

Good question.  @RequireOptionalModule is intended to mark certain
functionality that requires an optional module but not the rest of the
module.  If the optional dependency becomes a hard dependency, it makes
sense to me to update the spec and implementation to remove @ROM
annotation and @throw MNPE which doesn't have compatibility issue.

> Would it still make sense to annotate APIs
> in the module that require it as an optional module?
>

@RequireOptionalModule is intended for documentation purpose and any
developer tools to help the developers  be aware that the APIs in (1)
above requires a specific module to be present.   APIs in (2) and (3)
above are of a different category that these APIs supports both cases
when the optional module is present or absent but @ROM is defined for
those who throws MNPE.  On the other hand, should we have a different
annotation?

I raised a similar question in my first post of the optional modules
support [2] what can help the implementor of these APIs to detect if all
optional dependencies are properly declared in the module-info.java.  
One potential problem I see for APIs in (2) and (3) above is that the
optional dependency is not declared in the module-info.java and I asked
if we should annotate all these APIs so that a tool can detect any
incorrect module-info.java during development.   I convinced myself that
it's not justified to annotate them for this purpose.  During
development, tests should be developed to exercise the path when the
optional module is present or absent and catch the case if the optional
module is not declared in the module-info.java.   That's my view and it
should be caught during the development and testing process.

Mandy

[1] http://openjdk.java.net/projects/jigsaw/doc/topics/services.html
[2] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-May/001298.html
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Roger Riggs
In reply to this post by Mandy Chung
ok, I'll bite...   about the compiler being able (or not) to flag
potentially
faulty references to classes in optional modules.

When the compiler is using the module info to compile a module,
it will know that another module is optional respect to the module
being compiled.  For those references, it would seem to be knowable
how the reference is being used and be able to flag the same kinds
of conditions that the verifier would.

What am I missing?

Thanks, Roger



>
>> One of the questions posed is whether the compiler can use the
>> RequireOptionalModule annotations but I would have thought that if
>> there are any references to types in optional modules then it will
>> require that they be present at compile time. Maybe this question is
>> only for the case that reflection is used?
>>
>
> Roger posted this question in our previous discussion.  At compile
> time, the optional module must be present if there is any reference to
> types in the optional module.   If the optional method is not
> carefully written, at class loading time, the verifier might load
> types in optional modules for verification regardless of whether the
> optional module is called or not.  The question really comes to
> whether the compiler can detect such potential verification error
> during compilation.  I talked with Jon Gibbons another day.  The
> compiler can't detect that.  Jon and Alex can chime in and explain
> further.

Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

mark.reinhold
In reply to this post by Mandy Chung
2011/8/4 12:35 -0700, [hidden email]:
> An update on the optional module support [1].
>
> Webrev:
>    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/

I looked at the newer version, as you suggested:

    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.01/

> The note has been updated to include the open questions raised in
> the discussion.
>    http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html
>
> ...

This mostly looks good but I have a few suggestions and questions.

Line numbers are in [brackets].

org/openjdk/jigsaw/Context.java

  [158] Why a TreeSet?  Is order important?  If not, use a HashSet.

  [166] s/reexporting/re-exporting/

org/openjdk/jigsaw/Loader.java

  [131..132] s/mname/mn/g

  [158..164] Suggest reversing the order of these two statements.  If you
  find the library first then you can pass that to the findModule method,
  avoiding a second lookup in that method.

  [182] s/kernel/boot/

  [189] Brace should be at end of previous line.

  [190] s/this class's/this/

  [196] If m != null then you can just return it here and remove the test
  on [204].

  [382..388] The isModulePresent method should be defined earlier, before
  toString() [256].

org/openjdk/jigsaw/BootLoader.java

  [55] BASE_MODULE should be defined in Platform.java; it doesn't belong
  here.

  [58] If you're going to provide a factory method to enforce a singleton
  instance [118] then declare the constructor to be private.

  [94..115] I don't understand why this override is necessary.  The boot
  and base modules will be in the same context, so the definition in the
  Loader superclass should work correctly.

  [131] Brace should be at end of previous line.

org/openjdk/jigsaw/Linker.java

  [246] This comment should now mention that the supplier-name maps are
  being updated too.

java/lang/reflect/Module.java

  I think the isModulePresent and requireModulePresent methods really
  belong in java.lang.module.ModuleClassLoader.  These methods query
  a module context, not a module, and a ModuleClassLoader holds the
  run-time representation of a module context.

  This makes the usage idioms a little clunkier since now you have to
  cast the result of Class.getClassLoader():

      ((ModuleClassLoader)this.class.getClassLoader()).isModulePresent(mn)

  but that could be ameliorated by defining a convenience method in
  java.lang.Class which does the cast for you (and maybe returns the boot
  module loader rather than null when in legacy mode, as we've discussed
  previously).

java/lang/module/RequireOptionalModule.java

  Shouldn't this be named "RequiresOptionalModule", to match the fact
  that we use "requires" rather than "require" in module declarations?

  [47] If you rename this to "value" then annotations of the form

      @RequireOptionalModule(modules={"jdk.jaxp"})

  can be shortened to

      @RequireOptionalModule("jdk.jaxp")

- Mark
Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Mandy Chung
On 9/7/11 2:58 PM, [hidden email] wrote:
> This mostly looks good but I have a few suggestions and questions.
Thanks for the review.  I'll make the change per your suggestions.
My comments inlined below.

> Line numbers are in [brackets].
>
> org/openjdk/jigsaw/Context.java
>
>    [158] Why a TreeSet?  Is order important?  If not, use a HashSet.
Order is not important and will change it to HashSet.


>
>
> org/openjdk/jigsaw/BootLoader.java
>
>
>    [94..115] I don't understand why this override is necessary.  The boot
>    and base modules will be in the same context, so the definition in the
>    Loader superclass should work correctly.

The base and boot modules were made to be in different contexts to
implement the exports work [1]. In the prototype I built for exports,
if they are in the same context, the internal types exported from the
boot module will be visible to the applications requiring jdk.base.
As we have to explore further what the best model for exports,
I should keep them in the same context in this patch.

I do think this override is not necessary if they are in the same
context.  But I'm afraid split package could be one potential issue.
I'll make the change and double check to confirm.

One issue I ran into is that:

java.util.XMLUtils is currently in sun.jaxp module due to
its dependency on jaxp API.  java.util package is split
between jdk.boot and sun.jaxp and thus they are in the same
context.  In this case, jdk.base can't require optional jdk.jaxp.
Otherwise it will fail to install with:

    org.openjdk.jigsaw.ConfigurationException: Package javax.crypto.interfaces
    defined in +jdk.base+jdk.boot+sun.charsets+sun.jaxp but exported by
    supplier +jdk.jaxp

This is not a good example since java.util.XMLUtils should be renamed
to another package to avoid the split package.  It's on the todo list
and I'll do the rename in this fix and see if there is any other issue.

>
> java/lang/reflect/Module.java
>
>    I think the isModulePresent and requireModulePresent methods really
>    belong in java.lang.module.ModuleClassLoader.  These methods query
>    a module context, not a module, and a ModuleClassLoader holds the
>    run-time representation of a module context.
>
>    This makes the usage idioms a little clunkier since now you have to
>    cast the result of Class.getClassLoader():
>
>        ((ModuleClassLoader)this.class.getClassLoader()).isModulePresent(mn)
>
>    but that could be ameliorated by defining a convenience method in
>    java.lang.Class which does the cast for you (and maybe returns the boot
>    module loader rather than null when in legacy mode, as we've discussed
>    previously).

I also considered defining these methods in ModuleClassLoader
and add a convenience method in java.lang.Class that would simplify
the call to these methods after posting the webrev.  I thought I added
that question to in optional.html to look for feedback but apparently
it's still in my private copy :(

That's good and I'll make the change.

>
> java/lang/module/RequireOptionalModule.java
>
>    Shouldn't this be named "RequiresOptionalModule", to match the fact
>    that we use "requires" rather than "require" in module declarations?

I thought the new proposed grammar [2] is "require" rather than "requires".
The current prototype is yet to be updated to implement the new proposal
and thus we still use "requires".


>    [47] If you rename this to "value" then annotations of the form
>
>        @RequireOptionalModule(modules={"jdk.jaxp"})
>
>    can be shortened to
>
>        @RequireOptionalModule("jdk.jaxp")

That looks more concise.

Thanks
Mandy

[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-June/001333.html
[2] http://openjdk.java.net/projects/jigsaw/doc/topics/grammar.html


Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Mandy Chung
I modified the code per Mark's review comments.

Updated webrev at:
   http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.02/

* Move isModulePresent and requireModulePresent methods
   to ModuleClassLoader.  Also add the convenience methods
   in java.lang.Class.

* Class.getClassLoader for system classes returns the boot
   loader in module mode and returns null in legacy mode.

* Removed BootLoader.isModulePresent override.  The base and boot
   modules are in the same context and such override is not needed.

Mandy

Reply | Threaded
Open this post in threaded view
|

Re: Review request on the optional modules support

Mandy Chung
  On 09/15/11 08:17, Mandy Chung wrote:
>
> * Class.getClassLoader for system classes returns the boot
>   loader in module mode and returns null in legacy mode.
>

I would have to back out this Class.getClassLoader change because this
requires
more analysis and modification to the implementation.  I'll look into
this together
with the legacy mode support.

With more testing and investigation, there are places  in the JDK that
rely on null class loader for its implementation details.  Also there is
bootstrapping issue that we have to determine when the boot loader
can be initialized.

Final webrev:
    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.03/

Mandy