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

dbus: with_x11=True does not mean libdbus depends on xorg/system #22134

Closed

Conversation

tsondergaard
Copy link
Contributor

When dbus is built with with_x11 an extra executable dbus-launch will be built. Only this executable depends on x11. By adding the explicit requirement trait visible=False we avoid passing all the X11 libraries in xorg/system as link libraries to applications that link with dbus.

Fixes #22133


When dbus is built with with_x11 an extra executable `dbus-launch`
will be built. Only this executable depends on x11. By adding the
explicit requirement trait visible=False we avoid passing all the X11
libraries in xorg/system as link libraries to applications that link
with dbus.
Copy link
Contributor

github-actions bot commented Jan 3, 2024

🤖 Beep Boop! This pull request is making changes to 'recipes/dbus//'.

👋 @jwillikers you might be interested. 😉

@ghost
Copy link

ghost commented Jan 3, 2024

@conan-center-bot

This comment has been minimized.

@tsondergaard
Copy link
Contributor Author

tsondergaard commented Jan 3, 2024

The Conan v1 build failure is caused by this:

FAILED: dbus/libdbus-internal.a.p/dbus-sysdeps-util-win.c.obj 
"cl" "-Idbus\libdbus-internal.a.p" "-Idbus" "-I..\src\dbus" "-I." "-I..\src" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/Od" "/Zi" "-D_GNU_SOURCE" "/wo4018" "/wd4090" "/wd4101" "/wd4127" "/wd4244" "/we4002" "/we4003" "/we4013" "/we4028" "/we4031" "/we4047" "/we4114" "/we4133" "/Fddbus\libdbus-internal.a.p\dbus-sysdeps-util-win.c.pdb" /Fodbus/libdbus-internal.a.p/dbus-sysdeps-util-win.c.obj "/c" ../src/dbus/dbus-sysdeps-util-win.c
../src/dbus/dbus-sysdeps-util-win.c(1444): error C2022: '312': too big for character
../src/dbus/dbus-sysdeps-util-win.c(1450): error C2022: '312': too big for character

I looked at the affected lines ../src/dbus/dbus-sysdeps-util-win.c:1444 and ../src/dbus/dbus-sysdeps-util-win.c:1450 - they both contain DBUS_PREFIX which is a value configured by conan. Looking higher in the log I can see the prefix path that conan uses:

dbus/1.15.6: Meson configure cmd: meson setup --native-file "C:/J2/w/prod-v1/bsr/82476/cdfcb/s\a21702\1\build-debug\conan\conan_meson_native.ini" "C:/J2/w/prod-v1/bsr/82476/cdfcb/s\a21702\1\build-debug" "C:/J2/w/prod-v1/bsr/82476/cdfcb/s\a21702\1\src" -Dprefix="C:/J2/w/prod-v1/bsr/82476/cdfcb/s\470296\1"

\470 (octal) is 312 decimal, so the problem is definitely caused by Conan v1 putting backslashes in the path. Looking at the Conan v2 logs this problem appears to have been fixed there. I believe this is due to commit 89f01e14860fd850 which was introduced in Conan 2.0.9 via conan-io/conan#14295. @memsharded , @RubenRBS would it be possible to cherry-pick this fix to Conan v1? Or is it better to disable the Windows build of dbus on CI with Conan v1?

@tsondergaard
Copy link
Contributor Author

tsondergaard commented Jan 4, 2024

The Conan v2 build appears to fail on Windows because it uses Visual Studio 2022 and dbus doesn't build with the meson build system with that compiler - I have opened https://gitlab.freedesktop.org/dbus/dbus/-/issues/494 about this.

Update: I see #19421 is about this problem.

@ghost ghost mentioned this pull request Jan 4, 2024
3 tasks
Copy link
Contributor

@jwillikers jwillikers left a comment

Choose a reason for hiding this comment

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

LGTM!

@valgur
Copy link
Contributor

valgur commented Jan 16, 2024

Just to be clear, does dbus-launch always link against X11 statically? Otherwise it will still need the shared libraries at runtime.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 2 (6a1c7aa6a60118fbeabe1d23b3f0a35795566ef9):

  • dbus/1.15.6:
    All packages built successfully! (All logs)

  • dbus/1.15.8:
    All packages built successfully! (All logs)

  • dbus/1.15.2:
    All packages built successfully! (All logs)

  • dbus/1.15.0:
    All packages built successfully! (All logs)

@tsondergaard
Copy link
Contributor Author

tsondergaard commented Jan 16, 2024

@valgur,

Just to be clear, does dbus-launch always link against X11 statically? Otherwise it will still need the shared libraries at runtime.

It is not the intention with this PR that it should indicate that you don't need the shared libraries at runtime. I only want to communicate that if you link with libdbus (shared or static) then you do not need to linx with xorg/system.

If what I have done doesn't achieve that, what is the right solution to the problem then? Can I track the dependencies separately per component somehow? So dbus-launch can be listed as depending on xorg/system, but the libdbus.a/libdbus.so doesn't. If I need to do something like that can you point to one or more recipes in conan-center-index that implement this for my inspiration?

@valgur
Copy link
Contributor

valgur commented Jan 16, 2024

@tsondergaard Conan does not yet have a way to model runtime-only dependencies well, unfortunately.
The best solution I can suggest would be to add a separate component in package_info() for the library and the executable and add the .requires.append("xorg::x11") only for the latter.

It would not hurt to set the exported requires info for glib and other dependencies in a more exact manner either, since the recipe is probably overlinking many irrelevant libraries from the deps. This can be left for another PR, though.

Anyway, I checked dbus-launch with objdump -p and it only lists pthread and libc as runtime dependencies for both values of */*:shared=True/False, so marking xorg as visible=False should be ok.

@tsondergaard
Copy link
Contributor Author

tsondergaard commented Jan 16, 2024

@valgur, weird, that you cannot see any X11 dependencies on dbus-launch. This is what I see on the dbus binaries from my AlmaLinux distribution:

$ for i in $(rpm -ql dbus-libs dbus-tools dbus dbus-common dbus-x11 | grep -E 'lib64/|bin/'); do echo $i; ldd $i |&grep -i libx; d
one
/usr/lib64/libdbus-1.so.3
/usr/lib64/libdbus-1.so.3.19.13
/usr/bin/dbus-monitor
/usr/bin/dbus-send
/usr/bin/dbus-update-activation-environment
/usr/bin/dbus-uuidgen
/usr/bin/dbus-launch
        libX11.so.6 => /lib64/libX11.so.6 (0x00007fcec950e000)
        libxcb.so.1 => /lib64/libxcb.so.1 (0x00007fcec91fd000)
        libXau.so.6 => /lib64/libXau.so.6 (0x00007fcec8f70000)

@valgur
Copy link
Contributor

valgur commented Jan 16, 2024

@tsondergaard My comment was about the executable found in the package binary directory. It's ok for the system executable to have different behavior.

Copy link
Contributor

@valgur valgur left a comment

Choose a reason for hiding this comment

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

LGTM. DBus does not use the X11 libraries at all, even for the executables. It only needs X11 to be present during the build for the "X11 autolaunch" feature to be enabled.

https://gitlab.freedesktop.org/dbus/dbus/-/blob/dbus-1.15.8/meson.build?ref_type=tags#L613-629

Edit: that's incorrect. X11 is used when building tools: https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/tools/dbus-launch-x11.c#L39-40

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.

[bug] dbus: generated dbus-1.pc incorrectly has Requires: xorg
6 participants