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

[Alex Kashchenko]

Hi Jiri,

On 05/14/2018 04:03 PM, Jiri Vanek wrote:
>>
>> [...]
>>
> 
> 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?   

Test-only utils.rs doesn't belong to src, and when moving it to tests 
some (most?) other tests can go with it.


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

Please make the build to not write its files anywhere except its own 
build directory.

I currently have 1360 entries in my /tmp dir and don't need more of 
them. I know that /tmp handling depends on distro, but there is no 
reason to abuse it.


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

Yes, please use "expect", if there are no better alternatives.


>> 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 think you can just use writeln to a log file.


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

The whole this point is a matter of taste.


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

Minor things/typos:

jvm_from_properties.rs:

get_itw_global_config_file and get_itw_legacy_global_config_file - 
literals shouldn't be raw.


jvm_from_properties_resolver.rs:

order of methods in TestLogger doesn't match the one in trait declaration.


os_access.rs:

order of methods in Linux doesn't match the one in trait declaration.


property.rs:

inside Property::fmt write result is not checked (possibly intentionally 
omitted?).

typo in method name "che[kc]_property"


utils.rs:

typos in method names "debug[g]able_remove_file" and 
"debug[g]able_remove_dir"


> 
>     TYVM!
> 


-- 
-Alex