-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Restore the possibility of forcing "fat" static library links on a per-target basis #9453
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9453 +/- ##
==========================================
- Coverage 67.31% 67.30% -0.01%
==========================================
Files 396 396
Lines 85468 85474 +6
Branches 17661 17663 +2
==========================================
+ Hits 57530 57532 +2
- Misses 23249 23251 +2
- Partials 4689 4691 +2 ☔ View full report in Codecov by Sentry. |
c8c2e14
to
94ab2ad
Compare
This PR looks good to me, just missing a unit test ideally. However, we are not supposed to add new API in a point release. Since thin archives is only a performance optimisation and I don't think it has any functional difference, I think we should for 0.60.1 revert to not use thin archives by default and add this API for 0.61.0. What do you think? |
@xclaesse Favourably disposed. |
94ab2ad
to
a6e8be2
Compare
@jpakkane @dcbaker @eli-schwartz what do you think? Should we revert the thin archive stuff and postpone it to 0.61.0? |
Well, git master is already for 0.61, the question is whether to revert it in 0.60.1 :p In #9294 I wasn't sure whether this might sometimes be unwanted, but I wasn't expecting cases where it's unwanted because of what the meson.build does with the output lol. I wish we'd discovered this earlier... no one seemed to be quite sure what the potential pitfalls of thin archives might be. Is this a problem for meson's own cuda compiler too? |
@eli-schwartz By pure luck, the Meson testcases pass for CUDA because they are shielded with an |
@obilaniu I'm not sure your test case actually test much, we have no way to verify the generated archive actually is thin or not, do we? Are there tools that can tell? |
Actually, seems pretty easy to check: https://stackoverflow.com/questions/17297761/detect-whether-static-library-is-a-thin-archive. |
@xclaesse If the Filesystem's module's Is |
There are not that many platforms that supports thin archives in the first place... Tbh as long as it pass on CI platforms it's fine. |
Has this issue been reported to NVidia? This seems like a thing that they should handle. |
a6e8be2
to
09c67ec
Compare
09c67ec
to
81d630d
Compare
Continuing with my shower thoughts... there wouldn't happen to be some way we could detect that the archive is going to be used by a custom_target and disable generating thin archives in that case, would there be? In order to have a slightly safer/more conservative default... |
Some custom_target()s may want to receive "fat" archives, so making use of thin static_library() archives conditional only on it not being installed, and without the possibility of an override by the user, is undesirable. In mesonbuild#9453 I propose adding a thin: kwarg defaulting to true, but that's a new feature that can only land in 0.61.
Some custom_target()s may want to receive "fat" archives, so making use of thin static_library() archives conditional only on it not being installed, and without the possibility of an override by the user, is undesirable. In #9453 I propose adding a thin: kwarg defaulting to true, but that's a new feature that can only land in 0.61.
@jpakkane @eli-schwartz Here is a scenario where you can quickly observe all kinds of dysfunctions of NVCC and (in some places) our CUDA support. In this file, I compile four At the top of
I have tested this with CUDA Toolkit 10.2 (old) and 11.4 (newest aside from before-before-yesterday's 11.5 release), both with similar results. The odds are 11.5 will be affected as well. Even if NVIDIA fixed this tomorrow, there's a huge legacy of slightly-broken compilers to support. I therefore use a For this reason, I made this PR, which proposes returning control to the user through the Footnote: When using |
Some custom_target()s may want to receive "fat" archives, so making use of thin static_library() archives conditional only on it not being installed, and without the possibility of an override by the user, is undesirable. In #9453 I propose adding a thin: kwarg defaulting to true, but that's a new feature that can only land in 0.61.
This seems more like a task for a built-in option, than a keyword argument. |
It seems newer versions of meson[1] started to generate "thin" static libraries, which cannot be stripped by Debians package helper script (called by bdpg). The common workaround to avoid thin archives in meson seems to mark the static library with "install: true", until an explicit option might become available[2]. [1] mesonbuild/meson#9479 [2] mesonbuild/meson#9453 Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com>
Bug: mesonbuild/pull/9453 Bug: mesonbuild/issues/9479#issuecomment-953485040
Bug: mesonbuild/pull/9453 Bug: mesonbuild/issues/9479#issuecomment-953485040
Bug: mesonbuild/pull/9453 Bug: mesonbuild/issues/9479#issuecomment-953485040
Bug: /pull/9453 Bug: /issues/9479#issuecomment-953485040
Bug: /pull/9453 Bug: /issues/9479#issuecomment-953485040 (cherry picked from commit b6e5683)
Bug: mesonbuild/pull/9453 Bug: mesonbuild/issues/9479#issuecomment-953485040
With 0.60.0, non-installed
static_library()
targets are built by default as thin archives where supported, such as withar csrT
. This broke a flow where I was statically linking code into an archive, then passing the archive to acustom_target()
that was expecting a "fat" archive and wasn't handling "thin" archives (specifically, it'snvcc
being driven with custom flags).Thin archives are handy, but "fattening" an archive where needed is quite involved and annoying.
Add a
thin:
kwarg tostatic_library()
and default it totrue
, continuing the new behaviour of using thin archives in 0.60.0+. Passing the non-defaultthin: false
restores the known-good behaviour of Meson prior to 0.60.0.