[rfc][icedtea-web]detecting jre in rust lunchers

[Jiri Vanek]

On 04/24/2018 12:03 AM, Alex Kashchenko wrote:
> Hi Jiri,
> 
> Jiri Vanek wrote:
>  > Hello!
>  >
>  > Here is the patch adapted to our cargo. It bloated a lot, as I used
>  > cargo to fully test it
>  > (according to
>  > https://users.rust-lang.org/t/tutorial-how-to-collect-test-coverages-for-rust-project/650
>  > we have
>  > 99% now :)  Will add coverage report as separate target in future)
>  >
>  > Patch contains one hunk of unrelated formating chnages. I will push it
>  > as separate chnageset.
>  >
>  > Otherwise the patch is nearly same as before. Jsut more modular and
>  > tested. Andhopefuly aligned to
>  > all agreed conventions.
>  >
>  > Again, sory for its size. The real program code is not smallest, but is
>  > the smallest I managed to do
>  > as complete.
>  > I hope other features for luncher will be a bit smaller.
>  >
>  > Tahnx!
>  >     J.
>  > On 02/13/2018 11:03 AM, Jiri Vanek wrote:
>  >  > Reformatted by rustfmt.
>  >  >
>  >  > j.
>  >  > On 02/12/2018 02:20 PM, Jiri Vanek wrote:
>  >  >> As agreed somewhen around October, this patch is enhancing new rust
>  > binaries to detect jre:
>  >  >> Fallbacking as follow:
>  >  >>   - from properties (user; legacy user; global; legacy global)
>  >  >>   - from java home
>  >  >>   - from registry(win only)
>  >  >>   - hardcoded (linux/cygwinonly)
>  >  >>
>  >  >>
>  >  >> I abandoned original regexes, as to use dependencies without cargo
>  > (as agreed with Alex)  is
>  >  >> "human-impossible". If we eve rturn to deps, Cargo will take it nice
>  > place here. Tat will evolve
>  >  >> from windows part of implementation. If we move to cargo, Iwill
>  > change the proeprty split from
>  >  >> swtch to regex.
>  >  >>
>  >  >> The linux x windows specific backends are not distinguished here -
>  > although many can be currently
>  >  >> reused - Also that will evolve.
>  >  >>
>  >  >> 2018-02-12  Jiri Vanek <jvanek at redhat.com>
>  >  >>      * Makefile.am: building jvm_from_properties and os_access.
>  > Using them to build launchers.
>  >  >>      * rust-launcher/jvm_from_properties.rs: new file. library to
>  > find various ITW properties
>  >  >> files and check, parse proeprties form it, and find jre one if possible
>  >  >>      * rust-launcher/os_access.rs:  new file. trait for current
>  > os-depndent parts.  Contains
>  >  >> current linux implementation
>  >  >>      * rust-launcher/launchers.rs: now tries to locate the find jre
>  > as originally done in sh/bat
>  >  >> lunchers - from properties (user; legacy user; global; legacy
>  > global), from JAVA_HOME, from
>  >  >> registry(win/cygwin only), hardcoded (linux/cygwinonly)
>  >  >>
>  >  >
>  >  >
> 

Thank you very much for careful review. Most of the hints followed.

> 
> Overall, I don't think that launcher ought to have that much testing, but of course it won't harm.

How come!
It no cathced bugs in "return removal" refactroing you wanted...

> 
> Notes:
> 
> 1. I suggest moving all inline tests out of main sources (and test-only utils.rs file in src dir is 
> misleading)

Ugh. Are you sure?   Rust clearly saysi unittests to source file, integration tests to test folder. 
All those are unittests. So kept as it was.
> 
> 2. Please drop the "returns" where possible, they are currently used inconsistently

Done. Although i dont like the smicolon meaning in that.... KEpt only returns which servers also as 
breaks. Heaving noon-return return with break was just not nice...
> 
> 3. I think writing test data to OS temp directory is a bad idea. As cargo is quite limited about 
> test scratch dirs ( https://github.com/rust-lang/cargo/issues/2841 ), I suggest using something like 
> this instead of "env::temp_dir":

It seems to me that the thread complains about scratch dir. Not tmp dir(??). So I kept the tmp dir.

> 
>      pub fn create_scratch_dir() -> std::path::PathBuf {
>          let exepath = std::env::current_exe().expect("Current exe path not accessible");
>          let mut project_dir = exepath.parent().expect("Cannot get parent");
>          while(!project_dir.to_str().expect("Cannot get path name").ends_with("rust-launcher")) {
>              project_dir = project_dir.parent().expect("Cannot get parent");
>          }
>          let mut scratch_dir = std::path::PathBuf::new();
>          scratch_dir.push(project_dir);
>          scratch_dir.push("target");
>          scratch_dir.push("scratch");
>          if(!scratch_dir.exists()) {

To create a scratch dir in build dir seems quite nasty to me. Imho there is no reason to not use 
regular tmp...

> 
> std::fs::create_dir_all(scratch_dir.as_path()).expect("Cannot create scratch dir");
>          }
>          scratch_dir
>      }
> 
> Also it may be better to drop "clean_fake_files" (it was the first thing I've done to actually 
> inspect generated files)

The tests should have clean up after themselves.  I have added booleans  to disable the files deletion.
> 
> 4. Test content generation inside "create_tmp_propfile_with_custom_jre_content" is super-ugly, I 
> suggest using some formatting macro, like the one used in "try_jdk_from_properties_files", maybe 
> writing straight to the file (just please, without "unwraps")

Done. ..jsut with unwraps. How do you recommend to get rid of them?  Using expect?  /me hopaing for 
better trick:(
> 
> 5. I personally found concurrent tests (cargo default) a questionable idea. But if you've gone all 
> the way to have atomic counter for concurrent tests, you can also change Ordering::SeqCst to 
> Ordering::AcqRel as global ordering is not needed here

Done.
> 
> 6. I think that stored state for os-specific operations is not needed and a set of stateless 

I dont belive it will ever remain stateless. And even if does, the traits arehere exactly for this 
case.  Maybe I'm living in java universe, where the "get rid of static context and move to Object 
ASAP"  is quite gold rule, and is not so in Rust, but it fits me here.

If you think it is more of taste, and can live with "objects", then  its OK. As I would have issues 
to live without ;)
> functions can be used instead of a trait. But that is a matter of taste. I don't like the TestLogger 
> either, where "println" or "writeln" (synchronized as necessary) may suffice.

Wait - you dont like TestLogger? Then there must be fundametnal misunderstanding somewhere.
Thw logger part was done only because of windows. Because of you had concerns about stdout behaviour 
and usefulness.

I'm +1  to rework this to simple stdout/err, however I'm afraid some loggingbottleneck will be 
ncessary if the rust launchers wants to mimic java-part's logging.
> 
> 7. I suggest changing "struct Property" to a tuple of Strings. Otherwise, if you really want to 
> encapsulate it - please go all the way to Properties.load(). Currently "check_file_for_property" 
> lies somewhere in between.

Done! Proper sctruct with load method implemented. Thank you for this kick!
> 
> 8. "verify_jdk_path" should take a reference arg (or should be unified with "verify_jdk_string")

Great catch! Done.
> 
> 9. logic behind splitting properties-related code into "jvm_from_properties" and 
> "jvm_from_properties_resolver" files is not clear


Property.rs - responsible for loading proerty key+value
jvm_from_properties.rs - responisble for loading prperties from files
jvm_from_properties_resolver.rs responsible for (testable) correct fallback in in searched for files.

If you like some slightly different granularity, dont hesitate to suggest. My granularity was 
strongly inititated by lines in files.

> 
> 

Updated patch attached.

    TYVM!

-- 
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: cargizedJdkFRecognition2.patch
Type: text/x-patch
Size: 34692 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20180514/2031af27/cargizedJdkFRecognition2-0001.patch>