Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

meson: Make private static library symbols local #2969

Merged
merged 2 commits into from
Jun 17, 2021

Conversation

kennylevinsen
Copy link
Member

Static libraries are not affected by our symbol file, so private symbols
are globally visible by default.

Use objcopy to make symbols that we do not want to expose local.

Closes: #1892
Closes: #2952

meson.build Outdated Show resolved Hide resolved
@kennylevinsen kennylevinsen force-pushed the static_linking branch 2 times, most recently from c3b7f1e to 84ee02d Compare June 14, 2021 21:50
emersion added a commit to emersion/wlroots that referenced this pull request Jun 14, 2021
Let's expose all of our prefixed symbols. Instead of trying to have
fine-grained rules and only expose our public API, let's just expose
all symbols that won't cause a conflict.

Users won't be able to use the symbols without a proper header
declaration anyways. If they go through the process of re-defining
wlr_ symbols manually, that's on them if their build breaks.

This aligns the rules with [1].

[1]: swaywm#2969
meson.build Outdated Show resolved Hide resolved
@kennylevinsen kennylevinsen marked this pull request as ready for review June 14, 2021 21:56
meson.build Outdated Show resolved Hide resolved
Static libraries are not affected by our symbol file, so private symbols
are globally visible by default.

Use objcopy to make symbols that we do not want to expose local.

Closes: swaywm#1892
Closes: swaywm#2952
@kennylevinsen
Copy link
Member Author

Meson lacks support for prelinking on clang. See mesonbuild/meson#8883.

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

LGTM with static libs disabled on Clang on BSD and Arch (waiting for the Meson patch to get in)

@emersion emersion added this to the 0.14.0 milestone Jun 15, 2021
@kennylevinsen
Copy link
Member Author

LGTM with static libs disabled on Clang on BSD and Arch (waiting for the Meson patch to get in)

Do you want to conditionally disable the feature so that existing static linking workarounds remain possible?

@emersion
Copy link
Member

Do you want to conditionally disable the feature so that existing static linking workarounds remain possible?

As you prefer. I don't really like hardcoding specific compilers and Meson versions, but making all Clang builds fail when static libs are enabled isn't nice, especially if we're going to recommend static libs.

If we don't set prelink: true, we could always set link_args: '-Wl,--relocatable' I suppose, and that should work on all linkers of interest?

@kennylevinsen
Copy link
Member Author

If we don't set prelink: true, we could always set link_args: '-Wl,--relocatable' I suppose, and that should work on all linkers of interest?

What would the idea be here?

Prelinking adds an additional linker step which links all objects into a single object, which runs before packing the final .a (which is not linking, but just making anar archive of all final .o's).

I'll make the new feature gcc-only for now, while I look at fixing meson.

@emersion
Copy link
Member

Prelinking adds an additional linker step which links all objects into a single object, which runs before packing the final .a (which is not linking, but just making anar archive of all final .o's).

Hrm, indeed. The linker doesn't get invoked when building a static lib. It doesn't seem like objcopy has a mode where it can do what ld --relocatable does.

Let's just get the CI green for now then, just need to disable --default-libraries=both on Clang builds.

This allows us to validate the objcopy trick in CI. It's basically free
to build both static and shared libraries anyway.

We only enable this for GCC builds right now, as Meson currently lacks
support for prelinking with other compilers.
@kennylevinsen
Copy link
Member Author

Static linking is now only enabled for GCC in CI.

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@emersion emersion merged commit 8d2a94b into swaywm:master Jun 17, 2021
@emersion
Copy link
Member

Maybe we should add static linking to one of the Sway CI jobs?

emersion added a commit that referenced this pull request Jun 17, 2021
Let's expose all of our prefixed symbols. Instead of trying to have
fine-grained rules and only expose our public API, let's just expose
all symbols that won't cause a conflict.

Users won't be able to use the symbols without a proper header
declaration anyways. If they go through the process of re-defining
wlr_ symbols manually, that's on them if their build breaks.

This aligns the rules with [1].

[1]: #2969
@kennylevinsen kennylevinsen deleted the static_linking branch June 17, 2021 12:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Static linking of wlroots Static library contains private symbols
2 participants