[Pkg-rust-maintainers] dh-cargo does not respect LTO options from DEB_BUILD_OPTIONS

[Fabian Grünbichler]

On Sat, Apr 15, 2023, at 2:32 AM, Zixing Liu wrote:
> Hi Ximin and Fabian,
> 
> Recently I sent an email about the Rust toolchain LTO issue to the Debian Rust Maintainer mailing list. Due to the high-volume nature of that list, my email communication probably did not reach you. So here is a copy of the email in case the previous one landed inside your spam folder:

Hi, I did see the previous one but had no time to answer yet. We do actually have a separate, lower-volume discussion list at debian-rust at lists.debian.org (although the response ratio there is also not optimal, to be honest ;)).

> I am Zixing Liu from Canonical. Recently it came to my attention that `dh-cargo` and the `cargo` wrapper do not honour the `DEB_BUILD_OPTIONS=optimize=-/+lto` flag.
> Instead, whatever the project upstream defines in the `Cargo.toml` file (the `lto` parameter) takes precedence.
> 
> To better control the building parameters and avoid issues like https://github.com/rust-lang/rust/pull/89041, we need to be able to switch on or off LTO regardless of what the upstream project declares.
> 
> I propose adding a segment in the `dh-cargo` wrapper where we can pass the argument `--config "profile.release.lto = true/false"` to the `cargo` accordingly.
> 
> However, one thing I need some help with is the Rust toolchain's LTO option is a tri-state one, where it has `false,` `"thin,"` and `"fat"` (or "true") three possible values. In Debian, since the `DEB_BUILD_OPTIONS` is designed around GCC's conventions, it does not quite fit in our situation: `fat` may cause issues with debug information collection (see https://github.com/rust-lang/rust/pull/89041 for more information, this is an LLVM bug that has been around for quite a while now), and `thin` may generate inferior machine code during the LTO phase when comparing to `fat.` So we can't use a 100% bullet-proof default value for the `+lto` situation.
> 
> What's your opinion on this? I can handle the implementation if we reach a conclusion.

I don't have any strong opinions either way. 

One possible approach would be to do the following:

nothing set => keep whatever is in Cargo.toml
+lto set, LTO enabled in Cargo.toml => keep Cargo.toml
+lto set, LTO not enabled in Cargo.toml => override Cargo.toml with thin for now, since that causes less problems (?)
-lto set => disable LTO, regardless of Cargo.toml

the second and third option could be customized by patching Cargo.toml, or, if really needed, we could allow forcing the set value via an ENV variable as well. an alternate approach would be to honor +-lto with +lto always meaning thin (or fat), regardless of Cargo.toml's contents, and having a separate mechanism (ENV var, cargo wrapper parameter) to force the value.

note that dh-cargo is mainly (meant to be) used via debcargo, so it probably makes sense to implement the option throughout the stack, so that packages that are not using debcargo/dh-cargo (like firefox, librsvg, ..) but only the wrapper directly can also profit:

debcargo
- generates d/rules
d/rules
- sets up and calls dh-cargo
dh-cargo
- calls the packaged cargo's cargo wrapper in /usr/share/cargo/bin/cargo , which in turn might call the real cargo

my guess is that this means that actually the cargo wrapper is the one that implements handling of DEB_BUILD_OPTIONS (else we need to parse it twice) and the override env var (if one exists), and debcargo handles setting the override env var (if we want to implement one), with dh-cargo doing nothing at all. if you find an approach that seems better, feel free to just propose it as an MR on salsa in the respective repository, or discuss it here or in #debian-rust on IRC. given the freeze, it will likely be a while until it gets uploaded in any case.

Ximin is currently semi-afk, so I am not sure if he'll respond (soon).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/pkg-rust-maintainers/attachments/20230417/0b2638b3/attachment.htm>