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

[Zixing Liu]

Hi Fabian,

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 ;)).
>

Okay, I will send future email communications to that mailing list instead.

>
> 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.
>

Actually, we could pass the `--config  "profile.release.lto = <value>`
parameter to the cargo to achieve the same effect without touching the
Cargo.toml file.
Passing `-Clto` through `RUSTFLAGS` has its own problems where if the
project contains procedural macros or non-final-binary artifacts, the
compiler will abort the compilation and complain that `-Clto` does not
apply for Rust rlib libraries and proc_macro crates.


> 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.
>

I guess we could make the Cargo wrapper at /usr/share/cargo/bin/cargo to
handle the LTO switches since it's closer to the real Cargo and easier to
override in the packaging configurations.
I will post my patches on the Debian Salsa when I finish the implementation.

>
> Ximin is currently semi-afk, so I am not sure if he'll respond (soon).
>

Understood. I will ping them on the Merge Request regarding the review
process.

Thanks,
Zixing
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/pkg-rust-maintainers/attachments/20230417/8a0bcb6d/attachment-0001.htm>