Fix typos with calculating flags for lto setting

[havasd]

Fixed typos in #3104

Default behavior was incorrect as the unspecified should have no-op on the compile flags. However this was not the case due to incorrect conditions.

#3104 could break users who are already having special LTO flags but with custom toolchains.

I will have some additional fixes but this seemed more urgent to me to keep the old behavior intact before 0.56.0

cc: @ParkMyCar , @UebelAndre

[illicitonion]

Thanks!

[ParkMyCar]

Yeah shoot, sorry I should have foreseen this.

IMO having "unspecified" map to object_only is the better experience because it will result in faster compile times by default. What do you think about adding a new mode of "manual" or "skip" that totally leaves these flags alone?

[UebelAndre]

Waiting for the response from @havasd to #3119 (comment)

[havasd]

IMO having "unspecified" map to object_only is the better experience because it will result in faster compile times by default. What do you think about adding a new mode of "manual" or "skip" that totally leaves these flags alone?

As this LTO settings and these flags are a total can of worms and I consider it a strong breaking change. In our project we experimented with them a lot and there are lots of misleading elements in the documentation.

For example embed-bitcode=no doesn't work on iOS. According to this it is required there. Also if somebody is using explicit LTO rustc flags it will break their builds. If combined with -C lto, -C embed-bitcode=no will cause rustc to abort at start-up, because the combination is invalid.

Also based on this code cargo decides does some calculations based on all crates.

If we start to mix it with linker-plugin-lto that brings in another set of problems because then that flag essentially requires you to have compatible linker with the bundled LLVM in rustc.

I would think that even if we want to default embed-bitcode=no it should happen only on non-opt builds or when the lto flag is not present at all in the rustc args.

For now I would leave it like this. I am planning to have additional changes as we would like to use this feature but it may require some adjustments.

[ParkMyCar]

For example embed-bitcode=no doesn't work on iOS. According to this it is required there. Also if somebody is using explicit LTO rustc flags it will break their builds. If combined with -C lto, -C embed-bitcode=no will cause rustc to abort at start-up, because the combination is invalid.

Good call on the iOS builds, that is something I missed in the original impl! Although IMO I would consider building Rust code for iOS a bit of an edge case? I could be wrong though! Regardless, folks building for iOS could set lto=manual?

Also based on this code cargo decides does some calculations based on all crates.

FWIW the implementation in #3104 is specifically based on that exact calculation from Cargo. AFAICT what that code tries to do is determine how the resulting library should be generated, and in the case a single crate shows up multiple times in the build graph it merges the determinations together. The implementation that is currently merged in rules_rust does not merge the determinations together like Cargo does, mainly because I wasn't sure if it was possible in Bazel. This doesn't effect correctness though, it might cause multiple builds of a single crate if that crate is being linked against multiple types of targets in a single build.

If we start to mix it with linker-plugin-lto that brings in another set of problems because then that flag essentially requires you to have compatible linker with the bundled LLVM in rustc.

I might be missing some nuance but I don't think this is much of an issue, at least not in the current implementation? -Clinker-plugin-lto is only specified for rlibs in an LTO context, which is what Cargo also does. In the general case rustc can read it's own LLVM bitcode, but if you're using a separate linker, I think it would already need to be compatible with rustc's bundled version of LLVM, regardless of whether or not linker-plugin-lto is specified on an intermediate rlib?

I am planning to have additional changes as we would like to use this feature but it may require some adjustments.

This is great! I'm always looking for ways to make builds shorter and run times faster!

Personally I am still in favor of introducing a "manual" option (I'll whip up a PR) and keeping the existing impl as-is, for two reasons:

1. It results in faster builds and follows recommendations from the Rust docs
2. The original change was released in 0.56.0 which is a semver breaking change, so it still follows stability guidelines.

All that being said, I'm 100% happy to merge this PR if the majority of folks think so, and/or follow the recommendations of @illicitonion and @UebelAndre, I'm just a contributor here :)

[ParkMyCar]

Here's a PR that implements the "manual" option if we need it, #3120, no worries if we discard it and instead go with the approach in this current PR!