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

mpv: fix introspector build #12081

Merged
merged 2 commits into from
Jun 18, 2024
Merged

Conversation

kasper93
Copy link
Contributor

@kasper93 kasper93 commented Jun 18, 2024

Workarounds the issue where compile tests would fail with
-Werror=ignored-optimization-argument because Meson doesn't allow
linker flags in CFLAGS or CXXFLAGS.

See: mesonbuild/meson#6377 (comment)

Thanks to @evverx for the idea:
#7583 (comment)

This is a fragile workaround, but it looks like there isn't much else we
can do.

kasper93 added 2 commits June 18, 2024 02:45
Workarounds the issue where compile tests would fail with
`-Werror=ignored-optimization-argument` because Meson doesn't allow
linker flags in `CFLAGS` or `CXXFLAGS`.

See: mesonbuild/meson#6377 (comment)

Thanks to @evverx for the idea:
google#7583 (comment)

This is a fragile workaround, but it looks like there isn't much else we
can do.
For introspector build with LTO it can get quite memory heavy. Not sure
how much memory is available on builders, but let's be on the safe side.
Copy link

kasper93 is either the primary contact or is in the CCs list of projects/mpv.
kasper93 has previously contributed to projects/mpv. The previous PR was #12019

@kasper93
Copy link
Contributor Author

By the way, have you considered using LLD with thin lto? It would be much faster, current build is quite resource heavy.

@jonathanmetzman
Copy link
Contributor

Hmmm...I guess we probably should be doing this (though I've vaguely heard that moLD is the best right now!).
Linking is blackmagic to me.
I'm confused though, why are you using gold but suggesting lld?

@jonathanmetzman jonathanmetzman merged commit cd891dd into google:master Jun 18, 2024
16 checks passed
@kasper93
Copy link
Contributor Author

I'm confused though, why are you using gold but suggesting lld?

I use gold, because this is what is requested in default CFLAGS. I don't want to change default flags, this change is only to force Meson to do the correct thing.

@jonathanmetzman
Copy link
Contributor

I'm confused though, why are you using gold but suggesting lld?

I use gold, because this is what is requested in default CFLAGS. I don't want to change default flags, this change is only to force Meson to do the correct thing.

Ah introspector asks for this. :-(

@DavidKorczynski
Copy link
Collaborator

We need full LTO for introspector though -- @kasper93 do you know if lld supports full lto?

@kasper93
Copy link
Contributor Author

do you know if lld supports full lto?

Yes, of course. In fact in both cases the actual LTO is handled by the same libLTO https://github.com/llvm/llvm-project/tree/main/llvm/lib/LTO For gold it is loaded as a LTO plugin. For LLD it is directly integrated inside the linker. But the main code is the same in both cases. I don't know what are the actual differences in cases like linking also with "non-lto" objects. Generally, nowadays LLD is good default option and since the builds are already bases heavily on LLVM it could be done for linking too. Though without thin lto, there would be no difference in performance actually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants