RFR: Module constraints on targets

[Chris Hegarty]

Mark,

Updated webrev, and comments inline.

    http://cr.openjdk.java.net/~chegar/jigsaw/constraints.01/webrev/

On 10/19/2012 07:31 PM, mark.reinhold at oracle.com wrote:
> 2012/10/11 9:37 -0700, chris.hegarty at oracle.com:
>> ...
>>
>> Webrev:
>>    http://cr.openjdk.java.net/~chegar/jigsaw/constraints.00/webrev/
>
> Overall this looks pretty good; detailed comments below.
>
> After this change goes in we need to think further about repository
> layout and module-file naming conventions.  The filtering mechanism
> you've put into the repository code is fine, but we need a story for
> how to create a repository that contains multiple instances of the
> same module built for different os/arch combinations.

Right, and if os/arch are to be part of this convention, then maybe this 
is something that jpkg could do. It will now know if a module-file is 
platform dependent during creation. Anyway, your right further thought 
is required.

> ModuleArchitecture.java
>
>    I think one reason this class is hard to name is that it's serving two
>    purposes.  It can be a concrete os/arch pair or it can be a pattern
>    against which a concrete pair can be matched.  I suggest splitting this
>    into two classes, say Host and HostPattern.  The first is just a simple
>    wrapper around os and arch strings, and has a static method to return a
>    Host object describing the os/arch on which we're running.  The second
>    has the same instance methods as the first -- but it's not a subtype --
>    and also includes a boolean matches(Host h) method.
>
>    Then a Library can have a Host host() method, and a ModuleFileHeader
>    can have a HostPattern getHostPattern() method.
>
>    The string representations of these classes should be unambiguous, and
>    always in lower case.  E.g., "linux-x86", "linux-any", or "any".  As I
>    suggested previously, patterns like "any-x86" should be disallowed, at
>    least until someone comes up with a compelling use case.

I have taken this suggestion and added Host and HostPattern, and 
enforced this sensible restriction within jmod and jpkg.

>    (At some point we need to rationalize all our cpu-architecture names,
>     but that's a different topic.)
>
> SimpleLibrary
>
>    A Library will always have a Host, so wherever you're passing around
>    separate os/arch string pairs I'd pass a Host object instead.

Got it.

>    [204-211] There's no need to distinguish null from the empty string,
>    since neither can occur, so you can just use writeUTF(String) here, and
>    readUTF() when reading [234-238].

Yes. Still stored as separate os/arch strings so as not to have to parse 
a host string representation.

>    [1260] Throw a ConfigurationException for now, not an IOException.  We
>    should consider later whether to have a separate exception type for
>    this kind of failure.  Likewise on [1567].

Thank you. Changed to throw ConfigurationException, for now.

> Librarian
>
>    [74-75] If the os isn't specified on the command line then the host os
>    should be used, and likewise for arch.  The right place to arrange for
>    that is probably in the Host.create method.

Good idea. Changed.

>    [137-141] You can just out.format("host %s%n", lib.host()) here.

Done.

>    [357] When creating a new library, if it has a parent then we should
>    ensure that the parent's os/arch is identical.

Ah yes, good catch. latest webrev implements this.

> ModuleFile
>
>    [662-702] Having a builder for ModuleFileHeader seems like overkill,
>    but maybe that's just me.

Yes, I flip flopped on this myself. For now, the builder has been removed.

> ModuleFileWriter
>
>    [323-339] How much slower is this going to be than generating the hash
>    directly, as we do now?  Why is using a non-validating module-file
>    parser here a better approach?  (If you retained the current code then
>    you wouldn't need the dynamic dispatching in ModuleFile.readFileHash
>    [573-578].)

I did not do any perf measurements on this.

The main issue is that the file header is now variable length. I have 
reverted the changes in this area ( dynamic dispatching in 
ModuleFile.readFileHash too ), and you can see that the writer can 
workaround this issue once it can locate the hash bytes for exclusion. 
Should be fine, and the most optimal.

> RepositoryCatalog
>
>    [129] Shouldn't this be using .matches rather than .equals ?

D'oh! Yes, you are of course correct. Fixed.

> RemoteRepository
>
>    [340] s/findlocalModuleIds/findModuleIds/

Changed to findModuleIds.

>    [351-357] Why a separate method here?  I don't see it used anywhere
>    else.  I'd just fold line 356 into the find(Local)ModuleIds method.

Yes, Done

> Packager
>
>    If native commands or libraries are specified on the command line but
>    os or arch aren't given then we should default them to the values for
>    the local system.

Good idea. Implemented in the latest webrev.

>
> hello-repo.sh
>
>    This test is meant to be very basic, like the other hello tests.
>    You've added essentially two new tests at the end; I'd break these out
>    into separate files.

Moved into new repofilter.sh

-Chris.

>
> - Mark
>