build: Inherit flags from rustc

[mrkajetanp]

Where applicable, detect which RUSTFLAGS were set for rustc and convert them into their corresponding cc flags in order to ensure consistent codegen across Rust and non-Rust modules.

Resolves #1254

[mrkajetanp]

It's not fully ready yet, so far I'm looking for some feedback.
The main question is that as is the implementation doesn't check whether CC supports a flag before passing it. I tried using theis_flag_supported(_inner) functions to filter them out but the build script was infinite looping so I thought it could be good to just ask here first.

@NobodyXu

[NobodyXu]

Thanks, I have some feedback with the code style/lint:

[NobodyXu]

The main question is that as is the implementation doesn't check whether CC supports a flag before passing it. I tried using theis_flag_supported(_inner) functions to filter them out but the build script was infinite looping so I thought it could be good to just ask here first.

It's because in is_flag_supported we create a new cc::Build object and use it for detecting support.

https://docs.rs/cc/latest/src/cc/lib.rs.html#660

Because the rust flag is turned on by default, it causes infinite looping.

To fix this, you could use self.flags_supported, which is checked just before compilation without creating a new cc::Build `

https://docs.rs/cc/latest/src/cc/lib.rs.html#1909

[mrkajetanp]

To fix this, you could use self.flags_supported, which is checked just before compilation without creating a new cc::Build `

Ah interesting, I can't push to self.flags_supported because that'd require a &mut self, so I could only do that from an explicit builder function, from Build::new() or from compile().

I'm calling add_inherited_rustflags just before the snippet you linked where is_flag_supported_inner is called, should I not be able to just call that function myself directly instead of pushing to the Vec?

[NobodyXu]

I'm calling add_inherited_rustflags just before the snippet you linked where is_flag_supported_inner is called, should I not be able to just call that function myself directly instead of pushing to the Vec?

Yes in that case you can call is_flag_supported_inner directly

[mrkajetanp]

Hmm so I'm calling the new function like this:

        // Add cc flags inherited from matching rustc flags
        if self.inherit_rustflags {
            self.add_inherited_rustflags(&mut cmd, &target)?;
        }

        for flag in self.flags_supported.iter() {
            if self
                .is_flag_supported_inner(flag, &cmd.path, &target)
                .unwrap_or(false)
            {
                cmd.push_cc_arg((**flag).into());
            }
        }

and then inside the function even doing just this makes it infinite loop on creating the Tool for compiler inside is_flag_supported_inner.

        let supported = self
            .is_flag_supported_inner(&cc_flags[0], &cmd.path, &target)
            .unwrap_or(false);

Looking at the code I do not really understand why is_flag_supported_inner doesn't infinite loop on the first snippet already, can you explain what's different about these two calls? Sorry, not very familiar with the codebase..

[NobodyXu]

and then inside the function even doing just this makes it infinite loop on creating the Tool for compiler inside is_flag_supported_inner

Sorry, can you clarify on what function is is_flag_supported_inner being called in that trigger infinite loop?

[mrkajetanp]

Sorry, can you clarify on what function is is_flag_supported_inner being called in that trigger infinite loop?

Yep, I'm calling is_flag_supported_inner inside add_inherited_rustflags - that's the second snippet. add_inherited_rustflags is being called inside try_get_compiler - that's the first snippet.

[NobodyXu]

Sorri I made a mistake

is_flag_suppirted_inner actually creates another cc::Build

So calling it there would trigger infinite loop.

To fix that, you need to disable inherited flags detection in it:

https://docs.rs/cc/latest/src/cc/lib.rs.html#668

[mrkajetanp]

Ah indeed you're right, now it makes sense - many thanks! :)

[madsmtm]

Thanks for starting on this 🎉!

[madsmtm]

Looks a lot better to me, thanks for doing the work! Only nits from my side now.

[mrkajetanp]

Should be good now I think :)

[NobodyXu]

Thanks!

I am happy about the direction and general code, just a few questions regarding panic, ergonomic and some simple optimization

[NobodyXu]

Can we return an error instead of panicking?

IMO we can't fully rule out possibility of invalid input, and users could change env var before calling cc, so it's better to return an error, or ignore it and print out a warning.

[mrkajetanp]

@madsmtm specifically asked me add this so maybe he should weigh in here? I think CARGO_ENCODED_RUSTFLAGS is only supposed to be set by cargo so this should be fine. If a user manually sets this env var to something that wouldn't be accepted through RUSTFLAGS then they're breaking the assumptions cargo makes anyway.

[madsmtm]

Yeah, I requested it in #1279 (comment), copying my reasoning there:

This way, we know that a bare -Ccode-model wasn't specified (yes, this wouldn't be accepted by rustc, but we'd enter a wrong state if it was accepted in the future).

I'd be fine with a warning, ignoring the flag or returning an error, I just don't really want us to assume that it's something it's not.

[mrkajetanp]

Returning an error could work, how about this?

        fn flag_ok_or(flag: Option<String>, msg: &'static str) -> Result<String, Error> {
            flag.ok_or(Error::new(ErrorKind::InvalidArgument, msg))
        }

        match flag {
            // https://doc.rust-lang.org/rustc/codegen-options/index.html#code-model
            "-Ccode-model" => {
                self.code_model = Some(flag_ok_or(value, "-Ccode-model must have a value")?);
            }

I can add a new ErrorKind too if we want to distinguish them

[NobodyXu]

Yeah I think a new ErrorKind makes sense

Alternatively, if you decide to do logging instead, I would like the result of rustc flags to be discarded as it is ill-formed

[NobodyXu]

Suggested change

	fn arg_to_bool(arg: &str) -> Option<bool> {
	match arg {
	"y" | "yes" | "on" | "true" => Some(true),
	"n" | "no" | "off" | "false" => Some(false),
	_ => None,
	}
	}
	fn arg_to_bool_impl(arg: &str) -> Option<bool> {
	match arg {
	"y" | "yes" | "on" | "true" => Some(true),
	"n" | "no" | "off" | "false" => Some(false),
	_ => None,
	}
	}
	fn arg_to_bool(arg: impl AsRef<str>) -> Option<bool> {
	arg_to_bool(arg.as_ref())
	}

By accepting any impl AsRef<str>, you can simplify the .map_or to be:

.map_or(Some(...), arg_to_bool)

[mrkajetanp]

How about just this then? Not much point making it into two functions since it's only used here to avoid code duplication

        fn arg_to_bool(arg: impl AsRef<str>) -> Option<bool> {
            match arg.as_ref() {
                "y" | "yes" | "on" | "true" => Some(true),
                "n" | "no" | "off" | "false" => Some(false),
                _ => None,
            }
        }

[NobodyXu]

Yes that's fine to me, the reason I create two function is to avoid duplicate code monomorphisation, but it probably won't make much difference in real life so go for the simpler one please.

[NobodyXu]

Suggested change

	pub fn cc_flags(
	&self,
	build: &Build,
	path: &PathBuf,
	family: &ToolFamily,
	target: &Target,
	) -> Vec<OsString> {
	pub fn cc_flags(
	&self,
	build: &Build,
	path: &PathBuf,
	family: ToolFamily,
	target: &Target,
	args: &mut Vec<OsString>,
	) {

How about passing args directly so that we can push directly into it, and pass family as value since it's copyable and very small (much smaller than pointer)?

Alternatively, we can pass the tool: &mut Tool here, whichever is more convenient.

[mrkajetanp]

If we pass the vector as an argument, should I change the signature for push_if_supported or keep it as-is? Because I could make it into this

        target: &Target,
        flags: &mut Vec<OsString>,
    ) {
        let mut push_if_supported = |flag: OsString| {
            if build
                .is_flag_supported_inner(&flag, path, target)
                .unwrap_or(false)
            {
                flags.push(flag);
            } else {
                build.cargo_output.print_warning(&format!(
                    "Inherited flag {:?} is not supported by the currently used CC",
                    flag
                ));
            }
        };

[NobodyXu]

I'm ok with both honestly, one potential problem with removing the param, is that you can't push to the args directly and use the function at the same time.

If that doesn't happen, then yeah removing the param seems to be cleaner.

[NobodyXu]

Suggested change

	let push_if_supported = |flags: &mut Vec<OsString>, flag: OsString| {
	let push_if_supported = |flags: &mut Vec<OsString>, flag: String| {

Given that all usages of this function creates String, it would be more ergonomic to pass it to the function than doing additional .into()

[mrkajetanp]

The into() will be done anyway because it needs to be an OsString for is_flag_supported and to actually push it to the vector. Should I just move the into()s inside the helper function then?

[mrkajetanp]

It'd have to be something along the lines of

        let mut push_if_supported = |flag: String| {
            let flag = OsString::from(flag);
            if build
                .is_flag_supported_inner(&flag, path, target)
                .unwrap_or(false)
            {
                flags.push(flag);
            } else {
                build.cargo_output.print_warning(&format!(
                    "Inherited flag {:?} is not supported by the currently used CC",
                    flag
                ));
            }
        };

[mrkajetanp]

Some of the flags are &str as well so if we start taking String it'll be &str -> String -> OsString for those instead of &str -> OsString directly as it is now. For the ones that use format!() and do create a String it'd just be moving the call converting them to OsString. I can do it if you'd like me to but I think this change adds some overhead for certain flags.

[NobodyXu]

Maybe we can use impl Into<OsString> instead?

That'd work with &str, String or OsString, and no extra allocation.

[NobodyXu]

There's a merge conflict, I'd fine with either a merge or rebase, whatever is convenient to you.