RFR: Module constraints on targets

[mark.reinhold at oracle.com]

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.

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.

  (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.

  [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].

  [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].

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.

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

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

ModuleFile

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

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].)

RepositoryCatalog

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

RemoteRepository

  [340] s/findlocalModuleIds/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.

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.

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.

- Mark