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

[Alex Kashchenko]

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


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

Notes:

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

2. Please drop the "returns" where possible, they are currently used 
inconsistently

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":

     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()) {
 
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)

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")

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

6. I think that stored state for os-specific operations is not needed 
and a set of stateless 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.

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.

8. "verify_jdk_path" should take a reference arg (or should be unified 
with "verify_jdk_string")

9. logic behind splitting properties-related code into 
"jvm_from_properties" and "jvm_from_properties_resolver" files is not clear


-- 
-Alex