[rfc][icedtea-web] moving rust launchers to cargo

[Alex Kashchenko]

On 03/02/2018 02:27 PM, Jiri Vanek wrote:
> On 03/02/2018 03:20 PM, Jiri Vanek wrote:
>> On 03/02/2018 12:10 PM, Jiri Vanek wrote:
>>> On 03/02/2018 02:07 AM, Alex Kashchenko wrote:
>>>> Hi,
>>>>
>>>> Reviewing (ignoring autoconf changes):
>>>
>>> sad, But sure.
>>>>
>>>> 1. please rename "hardcoded_paths" - values there are not limited to 
>>>> paths (maybe to just "hardcoded")
>>>
>>> done.
>>>>
>>>> 2. add the following build.rs file to the rust-launcher dir ( see: 
>>>> https://doc.rust-lang.org/cargo/reference/build-scripts.html ) to 
>>>> have better control over extracting values from env variables:
>>>>
>>>> fn main() {
>>>>      // validation logic here for all vars
>>>>      println!("cargo:rustc-env=JRE={}", "foo1");
>>>>      println!("cargo:rustc-env=JAVA={}", "foo2");
>>>>      ...
>>>> }
>>>>
>>>> 3. in hardcoded.rs make all variable fetching non-optional (use only 
>>>> env! macro) and fill the default values in build.rs
>>>>
>>>> 4. drop tests from hardcoded.rs - this logic goes to build.rs
>>>
>>>
>>> I'm not sure how you expect this to work. 

Variables need to be fetched from env during compile time, validated 
(checking for missing/empty/invalid), substituted by default values 
where needed and then passed to rustc. This way inside hardcoded.rs 
env!() can be used, as all values are already validated in build.rs.


>>> The ones with options would 
>>> be dropped at the end of development, or made !env only.  So I would 
>>> let it be. If you insists, I can convert them imediately with soem 
>>> defaults, but that is not what println!("cargo:rustc-env=JAVA={}", 
>>> "foo2"); is doing.
>>> It *replace*  the value from env!.

Validation logic may be as complex as needed (e.g. distinguishing build 
initiated from autoconf from the one initiated directly/from IDE etc). 
Just please make sure that dev builds (direct cargo invocation) work all 
the time and don't require to add OS-level env vars on dev machine. 
Something like this can be used as a starter:

println!("cargo:rustc-env=ITW_JRE={}", 
option_env!("AC_JRE").unwrap_or("JRE-dev-unspecified"));


>>> I would ratehr stay with curent state
>>>   - optionals (and promiss that they will disapear ot be converted to 
>>> env! onl
>>>   - stay with the dummy test to invoke them
>>>
>>> build.rs do not sounds to me like used correctly here.

We well can embed static strings into binary manually, but 
"cargo:rustc-env" seems to be a recommended way to achieve that, from 
its doc:

"rustc-env=VAR=VALUE indicates that the specified environment variable 
will be added to the environment which the compiler is run within. The 
value can be then retrieved by the env! macro in the compiled crate. 
This is useful for embedding additional metadata in crate's code, such 
as the hash of Git HEAD or the unique identifier of a continuous 
integration server."


>> If nothing else, I think that this validation have to go to separate 
>> patch. And probably big one.
>> Attached is patch with build.rs removed.
> 
> returned also test.

Please add build.rs and drop the test.


>>>
>>>
>>>>
>>>> 5. drop module declaration from hardcoded.rs
>>>
>>> done
>>>
>>>
>>> Tahxn for nits!
>>>
>>> J.
>>>>
>>>>
>>>> On 02/26/2018 12:53 PM, Jiri Vanek wrote:
>>>>> Environment variables propagation check moved from test-time to 
>>>>> compile time.
>>>>>
>>>>> Thanx goes to Alkex!
>>>>>     J.
>>>>> On 02/15/2018 01:51 PM, Jiri Vanek wrote:
>>>>>> As agreed on privte discussion, this pathc is moving the 
>>>>>> responsibility to build of native launchers to cargo (from ourt 
>>>>>> makefiel and plain rustc).
>>>>>>
>>>>>> 2018-02-15  Jiri Vanek <jvanek at redhat.com>
>>>>>>      * configure.ac: added check for cargo
>>>>>>      * .hgignore: added target and Cargo.lock
>>>>>>      * .Makefile.am: dropped all 
>>>>>> launcher.in/libhardoced_paths_*.rs targets and rustc targets.
>>>>>>      (launcher.build/$(javaws) launcher.build/$(itweb_settings) 
>>>>>> launcher.build/$(policyeditor))
>>>>>>      adapted to use cargo. Variables handled in switch. (clean) 
>>>>>> now cleans all launcher.in*
>>>>>>      * rust-launcher/Cargo.toml: primitive declaration of package
>>>>>>      * rust-launcher/src/hardcoded_paths.rs: copied from 
>>>>>> rust-launcher/hardoced_paths.rs.in
>>>>>>      adapted to get substitution via cargo rather then by sed.  
>>>>>> Added test and wrapping getters
>>>>>>      * rust-launcher/src/main.rs: copied from 
>>>>>> rust-launcher/launchers.rs. Adapted imports to new 
>>>>>> infrastructure,s till just reprinting hardcoded stuff
>>>>>>
>>>>>> Thanx!
>>>>>>    J.
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
-Alex