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]

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

[NobodyXu]

Thank you!

This is the last review pass from me, and it mostly contains code suggestions that can be applied easily.

Once resolved I will merge it.

[NobodyXu]

This should fix the pipeline

[mrkajetanp]

I squashed the changes into the original commit, don't like unnecessary commit pollution

[NobodyXu]

Thank you!