Paths

Table of Contentst

Fix X11 library underlinking that makes the package FTBFS in Debian with bfd linker
ClosedPublic
Actions

Authored by Matteo F. Vescovi (mfv) on Nov 20 2020, 11:30 PM.

Details

Reviewers

Sergey Sharybin (sergey)
Sybren A. Stüvel (sybren)

Commits

rB9258b70453df: Explicitly link X11 libraries

Summary

From Ubuntu Hirsute changelog:

blender (2.83.5+dfsg-4ubuntu1) hirsute; urgency=medium

  * Try to also link ghost library with x11, needed because of missing
    XConvertSelection symbol link (used in ghost static library).
  * Don't use gold, but switch to bfd linker that seems to be working better
    on ppc64el.

 -- Gianfranco Costamagna <locutusofborg@debian.org>  Wed, 11 Nov 2020 14:17:29 +0100

Diff Detail

Repository: rB Blender

Event Timeline

Matteo F. Vescovi (mfv) requested review of this revision.Nov 20 2020, 11:30 PM

Matteo F. Vescovi (mfv) created this revision.

Matteo F. Vescovi (mfv) edited the summary of this revision. (Show Details)Nov 20 2020, 11:34 PM

To me it makes sense to link libraries explicitly, and not rely on some platform/version/implicit/indirect behavior.

The complicated part is that it doesn't seem the patch can be applied on the current master branch. What revision the patch is against?

Also, adding Sybren who is the current Linux maintainer.

In D9617#239114, @Sergey Sharybin (sergey) wrote:

What revision the patch is against?

AFAIK, v2.83.5 that is the actual version in Debian unstable/sid (the patch was prepared by fellow DD Gianfranco, not me).

Ah, I see.
An initial/experimental support of Wayland has been added since 2.83, and that's what causing merge conflicts.

Any chance we can handle this as a part of 2.91 transition in Debian (assuming you're planning it any time soon)?

The updated diff has been made against master branch.

In D9617#239283, @Sergey Sharybin (sergey) wrote:

Any chance we can handle this as a part of 2.91 transition in Debian (assuming you're planning it any time soon)?

I've just updated the diff to reflect the changes against the actual master branch.
Hope this helps, somehow.

Overall the patch looks fine to me. There is one thing I notice, though.
In your patch, almost every list(APPEND INC_SYS ...) gets an accompanying list(APPEND LIB ...), except in this block:

if(X11_XF86keysym_INCLUDE_PATH)
  add_definitions(-DWITH_XF86KEYSYM)
  list(APPEND INC_SYS
    ${X11_XF86keysym_INCLUDE_PATH}
  )
endif()

Is it not necessary here? I'm fine with the patch either way, I just wanted to double-check that this wasn't accidentally missed.

Hi!

In D9617#245430, @Sybren A. Stüvel (sybren) wrote:
Overall the patch looks fine to me. There is one thing I notice, though.
In your patch, almost every list(APPEND INC_SYS ...) gets an accompanying list(APPEND LIB ...), except in this block:
if(X11_XF86keysym_INCLUDE_PATH)
  add_definitions(-DWITH_XF86KEYSYM)
  list(APPEND INC_SYS
    ${X11_XF86keysym_INCLUDE_PATH}
  )
endif()
Is it not necessary here? I'm fine with the patch either way, I just wanted to double-check that this wasn't accidentally missed.

Checking for the library doesn't return anything, this is why I didn't add that LIB

grep X11_XF86keysy . -R
./intern/ghost/CMakeLists.txt:  if(X11_XF86keysym_INCLUDE_PATH)
./intern/ghost/CMakeLists.txt:      ${X11_XF86keysym_INCLUDE_PATH}
./obj-x86_64-linux-gnu/CMakeCache.txt:X11_XF86keysym_INCLUDE_PATH:PATH=/usr/include
./obj-x86_64-linux-gnu/CMakeCache.txt://ADVANCED property for variable: X11_XF86keysym_INCLUDE_PATH
./obj-x86_64-linux-gnu/CMakeCache.txt:X11_XF86keysym_INCLUDE_PATH-ADVANCED:INTERNAL=1
./build_files/cmake/platform/platform_unix.cmake:  find_path(X11_XF86keysym_INCLUDE_PATH X11/XF86keysym.h ${X11_INC_SEARCH_PATH})
./build_files/cmake/platform/platform_unix.cmake:  mark_as_advanced(X11_XF86keysym_INCLUDE_PATH)

Compare with:

grep X11_Xfixes . -R
./intern/ghost/CMakeLists.txt:      ${X11_Xfixes_INCLUDE_PATH}
./intern/ghost/CMakeLists.txt:      ${X11_Xfixes_LIB}
./obj-x86_64-linux-gnu/CMakeCache.txt:X11_Xfixes_INCLUDE_PATH:PATH=/usr/include
./obj-x86_64-linux-gnu/CMakeCache.txt:X11_Xfixes_LIB:FILEPATH=/usr/lib/x86_64-linux-gnu/libXfixes.so
./obj-x86_64-linux-gnu/CMakeCache.txt://ADVANCED property for variable: X11_Xfixes_INCLUDE_PATH
./obj-x86_64-linux-gnu/CMakeCache.txt:X11_Xfixes_INCLUDE_PATH-ADVANCED:INTERNAL=1
./obj-x86_64-linux-gnu/CMakeCache.txt://ADVANCED property for variable: X11_Xfixes_LIB
./obj-x86_64-linux-gnu/CMakeCache.txt:X11_Xfixes_LIB-ADVANCED:INTERNAL=1
./build_files/cmake/platform/platform_unix.cmake:    if(X11_Xfixes_LIB)
./build_files/cmake/platform/platform_unix.cmake:      list(APPEND PLATFORM_LINKLIBS ${X11_Xfixes_LIB})

Also, looking at ./build_files/cmake/platform/platform_unix.cmake

find_package(X11 REQUIRED)

 find_path(X11_XF86keysym_INCLUDE_PATH X11/XF86keysym.h ${X11_INC_SEARCH_PATH})
 mark_as_advanced(X11_XF86keysym_INCLUDE_PATH)

 list(APPEND PLATFORM_LINKLIBS ${X11_X11_LIB})

Looks like that XF86keysym is included in X11_X11_LIB (something I added already as part of this patch)

Hope this helps.

Hope this helps.

It does, thanks.

This revision is now accepted and ready to land.Dec 17 2020, 12:03 PM

Closed by commit rB9258b70453df: Explicitly link X11 libraries (authored by Sybren A. Stüvel (sybren)). · Explain WhyDec 17 2020, 1:33 PM

This revision was automatically updated to reflect the committed changes.

Sybren A. Stüvel (sybren) added a commit: rB9258b70453df: Explicitly link X11 libraries.

Revision Contents
Changeset List

Path

Size

intern/

ghost/

CMakeLists.txt

14 lines

Diff 32100

View Options

intern/ghost/CMakeLists.txt

Fix X11 library underlinking that makes the package FTBFS in Debian with bfd linkerClosedPublicActions