Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build-sys: Disable LTO by default #3999

Merged
merged 1 commit into from
Sep 10, 2022
Merged

Conversation

cgwalters
Copy link
Member

Way back when, we landed a2bbc12 "rust: Enable lto by default".

However at the time, rpm-ostree was a C program with a Rust shared library. Later we flipped it to being a Rust program with a C++ shared library.

And...that fixes the problem of leaking symbols that motivated having LTO on by default.

Let's defer the decision of using LTO to higher level build systems; operating systems/distros might set RUSTFLAGS=-C lto globally or per package as desired.

The immediate motivation here is that I don't always remember to do non-release builds locally and LTO is still a lot slower than non-LTO.

Way back when, we landed a2bbc12
"rust: Enable lto by default".

However at the time, rpm-ostree was a C program with a Rust
shared library.  Later we flipped it to being a Rust program
with a C++ shared library.

And...that fixes the problem of leaking symbols that motivated
having LTO on by default.

Let's defer the decision of using LTO to higher level build systems;
operating systems/distros might set `RUSTFLAGS=-C lto` globally or per package
as desired.

The immediate motivation here is that I don't always remember to
do non-release builds locally and LTO is still a lot slower
than non-LTO.
@cgwalters
Copy link
Member Author

I verified that LTO only shrinks things by a tiny bit:

$ ls -al target/release{,lto}/rpm-ostree
-rwxr-xr-x. 2 walters walters 12851336 Sep  9 12:11 target/release/rpm-ostree*
-rwxr-xr-x. 2 walters walters 12728464 Sep  9 12:11 target/releaselto/rpm-ostree*
$

Definitely not worth being on by default.

@cgwalters
Copy link
Member Author

cgwalters commented Sep 9, 2022

Er actually sorry, the real fix was f35afb1 which meant we're no longer dragging Rust at all into our (legacy) shared library.

Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jmarrero jmarrero merged commit 6c12e28 into coreos:main Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants