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

[Jiri Vanek]

On 03/02/2018 11:24 PM, Alex Kashchenko wrote:
> 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"));

Hello!

One more attempt to make default build pass, and still protect the launcher from being built without 
vars set.

Using the original approach (no build.rs), havinf  option_env! to private fields, and having 
unwrapping gettets in hardcoded.rs.  With difference that the getter will return default instead of 
failing.


In addition I would keep the test, ensuring that the necessary values are not empty nor default.
By that, the default build you require will be kept, and  my fear that default-vars build will leak 
removed.


hm?

Thanx!
   j.
> 
> 
>>>> 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.
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Jiri Vanek
Senior QE engineer, OpenJDK QE lead, Mgr.
Red Hat Czech
jvanek at redhat.com    M: +420775390109