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

Don't export symbols from static library #221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

BtbN
Copy link
Contributor

@BtbN BtbN commented Nov 15, 2023

The "both" case is likely still broken, but I see no good way to fix that with meson.

Background:
FFmpeg just started linking to libplacebo directly from ffplay.
This broke linking on Windows, since libavfilter ended up exporting the libplacebo symbols due to the forced dllexport on them.

Arguably, on Linux the visibility should likely also be set to hidden if PL_STATIC is set, since there is no point in exporting the symbols there either. But apparently the linker there is a bit smarter and figures things out.

@ePirat
Copy link
Contributor

ePirat commented Nov 15, 2023

LGTM

@kasper93
Copy link
Contributor

kasper93 commented Nov 16, 2023

The "both" case is likely still broken, but I see no good way to fix that with meson.

both case unfortunately is not fixable, because meson doesn't have any option to provide different flags for static/shared compilation. Objects are the same and in this case it is exported. Maybe one day they will support it, but not today. mesonbuild/meson#3304

Don't dllexport symbols when building static library commit is just cosmetic, no? You should not define PL_EXPORT when building static lib if you don't want it to have marked symbols for export. If you don't do that as you did in the other commit, it will work correctly. There is small use-case where you want to build static lib and still be able to export its symbols when linking into common shared library.

The meson change LGTM, I don't think we can do anything better with meson...

Arguably, on Linux the visibility should likely also be set to hidden if PL_STATIC is set, since there is no point in exporting the symbols there either. But apparently the linker there is a bit smarter and figures things out.

Most likely on Linux it is linked with -Wl,--exclude-libs,ALL as any sane person would do. On Windows it is just different, because dllexport just marks symbols that should be exported, so whenever they are linked they are made visible.

@haasn
Copy link
Owner

haasn commented Nov 17, 2023

(Style) Can you change the commit title to e.g. meson: set PL_STATIC when building static library ?

As for the config.h change, @kasper93 it's saying it's not needed? Seems like it wouldn't really fix anything, if you're saying it is still broken in the "both" case.

@Nevcairiel
Copy link
Contributor

meson should never set PL_EXPORT and PL_STATIC at the same time, so the config.h.in change just re-orders the defines but with no change in actual functionality.

@ePirat
Copy link
Contributor

ePirat commented Nov 17, 2023

@kasper93 I don't think both would ever be workable on windows because you need different installed headers for the static vs non-static library (due to the need for dllimport, or rely on the client using the library to set some define before including the header…)

The both case is mostly for a nice benefit of not rebuilding everything, which makes it somewhat pointless once you have different preprocessor macros for static vs dynamic cases as then you anyway need to rebuild everything…

@kasper93
Copy link
Contributor

kasper93 commented Nov 17, 2023

I don't think both would ever be workable on windows because you need different installed headers for the static vs non-static library (due to the need for dllimport, or rely on the client using the library to set some define before including the header…)

You need exactly the same headers. That's why PL_STATIC exist to add dllimport when requested. And is currently appended when pkgconf --static is requested. (Cflags.private)

@kasper93
Copy link
Contributor

We never merged that? Either way, meson now have c_static_args and c_shared_args, we could use that instead. mesonbuild/meson#11981

@kasper93
Copy link
Contributor

kasper93 commented Nov 1, 2024

@BtbN
Copy link
Contributor Author

BtbN commented Nov 1, 2024

Looks good and should allow the workaround on my side to be removed.

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.

5 participants