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

Sway static compilation failure on set_cloexec #4677

Closed
sheenobu opened this issue Oct 27, 2019 · 6 comments · Fixed by #4682
Closed

Sway static compilation failure on set_cloexec #4677

sheenobu opened this issue Oct 27, 2019 · 6 comments · Fixed by #4682

Comments

@sheenobu
Copy link
Contributor

sheenobu commented Oct 27, 2019

  • Sway Version: master
  • Wlroots version: master

Compiling sway in static mode fails due to conflicting symbols exposed by both wlroots and sway. The symbol is set_cloexec. This function is duplicated a few times across both code bases.

Simple version of my build commands:

> meson --default-library static --buildtype release build
> ninja -C build

Full version of my build script: https://gist.github.com/sheenobu/bfa8f758878184a4c5b72066a938bd8f

Expected output:

a binary with sway and wlroots statically linked together

Actual output:

/usr/bin/ld: subprojects/wlroots/libwlroots.a(sockets.c.o): in function `set_cloexec':
sockets.c:(.text+0x0): multiple definition of `set_cloexec'; common/libsway-common.a(util.c.o):util.c:(.text+0x290): first defined here

I was able to fix this by inlining set_cloexec within include/util.h but i think fixing this in wlroots might be better? just make set_cloexec a wlroots util function that can be imported in?

HACK, don't use:

diff --git a/include/util.h b/include/util.h
index 6d9454e0..930162f0 100644
--- a/include/util.h
+++ b/include/util.h
@@ -1,9 +1,12 @@
 #ifndef _SWAY_UTIL_H
 #define _SWAY_UTIL_H
 
+#include <stdlib.h>
 #include <stdint.h>
 #include <stdbool.h>
 #include <wayland-server-protocol.h>
+#include <fcntl.h>
+#include "log.h"
 
 /**
  * Wrap i into the range [0, max[
@@ -32,6 +35,22 @@ float parse_float(const char *value);
 
 const char *sway_wl_output_subpixel_to_string(enum wl_output_subpixel subpixel);
 
-bool set_cloexec(int fd, bool cloexec);
+static inline bool set_cloexec(int fd, bool cloexec) {
+       int flags = fcntl(fd, F_GETFD);
+       if (flags == -1) {
+               sway_log_errno(SWAY_ERROR, "fcntl failed");
+               return false;
+       }
+       if (cloexec) {
+               flags = flags | FD_CLOEXEC;
+       } else {
+               flags = flags & ~FD_CLOEXEC;
+       }
+       if (fcntl(fd, F_SETFD, flags) == -1) {
+               sway_log_errno(SWAY_ERROR, "fcntl failed");
+               return false;
+       }
+       return true;
+}
@emersion
Copy link
Member

emersion commented Oct 28, 2019

Hmm. This function shouldn't be exported by wlroots because we use a version script to filter symbols: https://github.com/swaywm/wlroots/blob/master/wlroots.syms

Maybe the version script isn't run properly when generating a static lib?

@ascent12
Copy link
Member

Yeah, the "rules" for symbol exporting when statically linking are not the same.
That's pretty annoying. I guess we need to be more careful of the symbol names we choose, even if they're not part of our exported API.

@emersion
Copy link
Member

Hmm, I'd rather fix our exported symbols for the static lib.

@sheenobu
Copy link
Contributor Author

If all the static libs are just being pushed directly into the sway binary, there isn't a way to actually enforce the exported/not-exported settings since it's all just a bunch of .o files in the end?

Or is my linker knowledge out of date? can you $LINKER -o binary file1.o -Wl=filter-for-file-2 file2.o ?

@emersion
Copy link
Member

Hmm, actually Meson doesn't use ld at all, it just uses ar to merge all the *.o files. So we can't even use linker args here.

sheenobu added a commit to sheenobu/sway that referenced this issue Oct 29, 2019
set_cloexec is defined by both sway and wlroots (and who-knows-else),
so rename the sway one for supporting static linkage. We also remove
the duplicate version of this in client/.

Fixes: swaywm#4677
@sheenobu
Copy link
Contributor Author

PR is probably not an ideal but a fix none-the-less.

ddevault pushed a commit that referenced this issue Nov 1, 2019
set_cloexec is defined by both sway and wlroots (and who-knows-else),
so rename the sway one for supporting static linkage. We also remove
the duplicate version of this in client/.

Fixes: #4677
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants