[rfc][icedtea-web] add classapth resolving to rust lunchers (and thus make them work)

[Jiri Vanek]

On 1/8/19 1:58 AM, Alex Kashchenko wrote:
> Hi Jiri,
> 
> Sorry for the delay, see notes inline:
> 

No problem. Thank yo very much. the review is surprisingly kind. I'm wondering if it was so good
code, or so bad :)

> On 12/18/18 2:53 PM, Jiri Vanek wrote:
>>
>> [...]
>>
>>>>> configure:
>>>>> - introduced configure switch --with-itw-libs which determines if you build portable or
>>>>> distribution
>>>>> result. The goal is to achieve, that you can coexists  system itw, and in parallel custom portable
>>>>> instance by default, without fear of interference. Still the libraries order can be changed in
>>>>> runtime pretty much.
> 
> Is it possible to add auto-detection here to default to BUNDLED on windows? It would be great if
> "bash configure" without args continue working on windows.

done
> 
> Also currently check_config_files_paths test is failing on windows, I suggest adding
> #[cfg(not(windows))] to it.

done

I wonted to replace os:Linux::new(verbose) by new method, create_current_os(verbose)->Os, which
would made the tes valid, and would actually enforce to implement basic Windows struct. but forget
about memory issues which will rise, and thus enforcing bigger  changes to code.
Thus this will be done as separate chnageset, end the cfg(not(windows) will then be removed.
In addition I split the test to two, as windows will likely not provide legacy dirs.
> 
> 
>>>>> * rust
>>>>> - inlcuded laoding of ITW_LIBS in compile time, propagating all jars so they can be reusable. Ketp
>>>>> xboothclassapth propagated to, but is use donly in debug output.
>>>>> - new module jars_helper is here to try to search for resources based  on ITW_LIBS and locations
>>>>> where deps are (we currently have windows and linux dirs named differently). About the members and
>>>>> order of LOCAL_PATH, I have many concerns. The current one is most free one.
>>>>>    - persisted the error of javafx being on normal classapth. Shouldnt it be on bootclassapth?
>>>>> (see
>>>>> nashorn around same lines) and see
>>>>> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2018-November/040492.html)
>>>>>    - added method which composes cp/bootcp from freshl located members and from java resources
>>>>> (jdk8
>>>>> and down only!)
>>>>> - main method made aware of this classpaths and so it it is included in params for
>>>>> os.spawn_java_process together with main method and  custom arguments from cmdline and ...
>>>>>    ***fanfares*** IT WORKS ***fanfares***
>>>>> - os dependent modules got : x ; classpathDelimiter
>>>>> - moved  TestLogger to utils - is now shared.
> 
> dirs_paths_helpers:
> 
> Inside path_to_string I think it is better to use the following:
> 
> path.to_str().expect(<...>).to_string()
done
> 
> 
> jars_helper:
> 
> 1. typo i[m]portant
done
> 
> 2. I suggest renaming throwaway variables in pattern matching to "_"

This si only splash, isnt it? Splash will be handled so the warning will disappear. Until then, it
is correct.
> 
> 3. in resolve_jar get_libsearch is called multiple times, shouldn't it be called only once?

In this case, I do not see difference between calling method twice, or declaring variable can
reusing this result. So kept. I will fix if you insists.
> 
> 4. in resolve_jar, it is better to use:
> 
> match pgmdir.parent()
> 
> instead of:
> 
> match pgmdir.clone().parent()
> 
> and clone it later where needed

done

> 
> 5. std::vec::Vec is a part of Prelude - no need for package prefix

done! thanx!
> 
> 6. in get_bootcp_members the same match is repeated 4 times, it is better to wrap it into some helper

done :)
> 
> 7. in compose_class_path,
> 
> while i != members.len()
> 
> should be written as:
> 
> for (i, mb) in members.iter().enumerate()

done. Thanx!
> 
> 
> hardcoded_paths:
> 
> Contents of itw_libsearch_to_enum should go to FromStr implementation.
> 

Done!
>>
>> [...]
>>
> 

Are you moreover happy with the handlidng of jars?
How do you feel about javaws.jar?
How do you feel about --with-itw-libs ?

>>> So this patch, in is making  rust lunchers to do something. Is there something missing/redundant in
>>> this todo list?
>>> todo:
>>>   -J arguments
>>>   splash (+headless)
>>>   LAUNCHER_FLAGS=-Xms8m (?)
>>>   file logging
>>>   jdk from path(I dont know how to make it properly)
>>>   jdk9+ support (--patch/--add*)
>>>   jdk9+ support (rt.jar, fxrt.jar, nashorn.jar)

Do I miss something in this todo?


Thanx a lot!
  J.

-- 
Jiri Vanek
Senior QE engineer, OpenJDK QE lead, Mgr.
Red Hat Czech
jvanek at redhat.com    M: +420775390109
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rustJarsResolverAndClassPathComposer3.patch
Type: text/x-patch
Size: 51012 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20190108/c74ac51f/rustJarsResolverAndClassPathComposer3-0001.patch>