native cmds and libs in the module library

[Mandy Chung]

Thanks, Chris. This looks much better.

Modules.gmk
   L141: I would suggest to replace "$$mlib/.." with
   "$(MODULE_IMAGES_DIR)/$$image/bin" to make the path explicit and
   similarly for "$$mlib/../../bin".  Perhaps define a new variable for
   the root "$(MODULE_IMAGES_DIR)/$$image" would help.

   L191: since the jmod files have been created, do we really need
   L195-197?

SimpleLibrary.java
   L228-232: should they be final fields?

ModuleFile.java
   L295: extra blank line.

Mandy

On 1/20/2012 9:56 AM, Chris Hegarty wrote:
> Thanks Mandy,
>
> You are correct. I was storing absolute paths.
>
> Updated webrev:
>   http://cr.openjdk.java.net/~chegar/jigsaw/libbin_webrev.04/webrev/
>
>  - Paths to 'jmod create -L lib -natlib <dir> ...' are resolved
>    relative to the current working dir, as you would expect.
>  - natlib, natcmd, config, paths are stored relative to the module
>    library root, in the libraries metadata.
>  - The per module 'files' contains paths relative to the modules
>    install directory.
>
>  - I made most of the specific code suggestions you said, with the
>    exception that natlibs, natcmds, and configs can still be null
>    in SimpleLibrary. I use this to differentiate between a passed
>    path and a per module path.
>
> -Chris.
>
> On 01/18/12 07:03 AM, Mandy Chung wrote:
>> On 1/17/2012 6:28 AM, Chris Hegarty wrote:
>>
>>> Here is an updated webrev based on the comments and feedback I received
>>> on this.
>>>
>>> - An additional option is added for supporting config files
>> If a relative path is specified as an argument to --natlib, --natbin,
>> or --config option, what is it relative to and what gets recorded
>> in the module library header? I believe the implementation currently
>> uses cwd (i.e. the directory where jmod install is invoked) which
>> means that we can't simply copy the module library and the natlibs
>> to a different location to use?? See my comment for Modules.gmk.
>>
>>
>>> http://cr.openjdk.java.net/~chegar/jigsaw/libbin_webrev.03/webrev/
>> Modules.gmk
>> L141: --natlib $mlib/.. --natcmd $mlib/../../bin --config $$mlib/..
>>
>> $mlib is an absolute path. Since these alt paths are recorded
>> in the module library header, if we zip up the jdk-module-image and
>> unzip it at a different location, these paths need fixup in order
>> to install new modules in it. It seems that we should keep
>> these paths relative to the system module library when creating
>> the jdk modules image.
>>
>> Similarly, the realpath of the files are stored in "files"
>> which would need fixup too which is not ideal.
>>
>> SimpleLibrary.java
>> L346-348: I wonder if it's cleaner to initialize these instance
>> variables to the default path, i.e. new File(root, "bin") if the
>> given argument is null so that the null check is not needed in
>> various place.
>>
>> L301: I think this warning should be output in the Librarian
>> implementation rather than in the SimpleLibrary. Instead
>> it should throw an exception if the natlibs is not writeable
>> and when it writes a native library.
>>
>> L1289: it's a good question when we should check the extension.
>> It depends if the specification requires the extension of a
>> module file be a specific value. I think we can leave as it
>> is and get back to it later.
>>
>> ModuleFile.java
>> L199-201: may be cleaner if these variables are initialized
>> to the default path e.g. File(destination, "lib") and the
>> non-null check in the getDirOfSection method will not be needed.
>>
>> L581-585: minor: since they are interested in 3 section types,
>> would it be better to handle each section type in this postExtract
>> method rather than checking the section type in both
>> markNativeCodeExecutable and trackFiles methods. Probably the
>> markNativeCodeExecutable method can be merged with this postExtract
>> method and remove L501-504 as well.
>>
>> Librarian.java
>> L55, 151: this comment line can be removed.
>>
>> hello-native.sh
>> It might be good to verify if the path of the native library
>> is expected.
>>
>>
>>> Issues I intend to address separately:
>>>
>>> - Separate out native libs and config files during the build, so they
>>> can be put into the right sections of the jmod file
>>> - Investigate the potential for global configuration files versus
>>> non-editable per-module config files.
>>
>> That's fine with me.
>>
>> Mandy
>>
>>>
>>> Thanks,
>>> -Chris.