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

Buildfailure on openSUSE, if built with "--no-undefined -Wl" #310

Closed
sfalken opened this issue Aug 27, 2022 · 6 comments · Fixed by #311
Closed

Buildfailure on openSUSE, if built with "--no-undefined -Wl" #310

sfalken opened this issue Aug 27, 2022 · 6 comments · Fixed by #311
Assignees

Comments

@sfalken
Copy link

sfalken commented Aug 27, 2022

Expected Behavior

When using the following CMake flag, it should successfully build:
-DCMAKE_SHARED_LINKER_FLAGS=" -Wl,--as-needed -Wl,--no-undefined -Wl,-z,now"

Current Behavior

Building with -DCMAKE_SHARED_LINKER_FLAGS=" -Wl,--as-needed -Wl,--no-undefined -Wl,-z,now" results in broken build:
https://susepaste.org/81371193

Possible Solution

Pass -DCMAKE_SHARED_LINKER_FLAGS="-Wl,--as-needed -Wl,-z,now" to CMake results in successful build

Steps to Reproduce (for bugs)

Build with "--no-undefined -Wl"

Context

Building latest released sources for distribution within openSUSE

System Information
  • Distribution & Version: openSUSE Tumbleweed & openSUSE Leap 15.4
  • Kernel: 5.19.2 (Tumbleweed) / 5.14.21 (Leap 15.4)
  • Qt Version: 5.15.5 (Tumbleweed) / 5.15.2 (Leap 15.4)
  • libqtxdg Version: 3.9.1 (Tumbleweed) / 3.8.0 (Leap 15.4)
  • lxqt-build-tools Version: 0.11.0 (Tumbleweed) / 0.10.0 (Leap 15.4)
  • Package version: 2.4.0
@tsujan
Copy link
Member

tsujan commented Aug 27, 2022

I haven't investigated it, but I confirm the build failure under Arch/Manjaro too.

EDIT: It's because of --no-undefined.

@tsujan
Copy link
Member

tsujan commented Aug 28, 2022

The problem can be solved by making extedit static:

diff -ruNp screengrab-orig/src/modules/extedit/CMakeLists.txt screengrab/src/modules/extedit/CMakeLists.txt
--- screengrab-orig/src/modules/extedit/CMakeLists.txt
+++ screengrab/src/modules/extedit/CMakeLists.txt
@@ -24,12 +24,10 @@ qt5_translation_loader(extedit_QM_LOADER
 )
 
 add_library(extedit
-    SHARED
+    STATIC
         ${extedit_SRC}
         ${extedit_QMS}
         ${extedit_QM_LOADER}
 )
-set_property (TARGET extedit PROPERTY SOVERSION 1.0.0)
-install(TARGETS extedit DESTINATION ${SG_LIBDIR})
 
 target_link_libraries(extedit Qt5::Widgets Qt5::X11Extras Qt5Xdg)

Since I have no idea why it was a shared library, I'm not sure that this is the best solution. Screengrab seems to work fine with it though.

I neither know why qkeysequencewidget has an option for making it a shared library, although it's OFF by default.

The code of screengrab has always seemed strange to me...

@sfalken
Copy link
Author

sfalken commented Aug 28, 2022

openSUSE generally has a "no static libraries" policy in their packaging, but if that's how the thing is supposed to work, I can make the case for it, and see if the Factory maintainers are fine with it.

@tsujan
Copy link
Member

tsujan commented Aug 28, 2022

To be honest, I see no reason for having libraries here, but I respect the choices of the original authors anywhere in LXQt, as far as they cause no problem.

Anyway, qkeysequencewidget has been static from start.

The code of screengrab needs to be reorganized. qkeysequencewidget isn't needed when Qt has QKeySequenceEdit (although it was needed when the code was written), and the single-instance mode has a problem.

As for this issue, I think extedit was a fake shared library; --no-undefined just revealed that. Waiting for a counterargument by LXQt devs....

@sfalken
Copy link
Author

sfalken commented Aug 28, 2022

Aye, it's not a huge deal for us, it's just one of our "Packaging Guidelines" there can be exceptions where they make sense.

@tsujan
Copy link
Member

tsujan commented Aug 28, 2022

The difference for packaging is only that there will be no ".so" files; nothing more.

tsujan added a commit that referenced this issue Aug 29, 2022
`qkeysequencewidget` was static by default. But `extedit` was shared, while depending on the core code. It made the compilation fail with `--no-undefined`.

Fixes #310
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this issue Sep 1, 2022
https://build.opensuse.org/request/show/1000717
by user jubalh + dimstar_suse
- Add patch to correct failing build.  [gh#lxqt/screengrab#310]
  * fix-no-undefined-builderror.patch
- Cleaned up %build section
- Modified %files, due to new additions from upstream

- Update to 2.4.0:
  * Used an SVG icon instead of the blurry PNG icon.
  * Fixed several problems in calling external editors or default app.
  * Added metadata file.
- Use -DSG_EXT_EDIT=ON
- Add screengrab-link.patch
tsujan added a commit that referenced this issue Sep 15, 2022
* Removed shared libraries

`qkeysequencewidget` was static by default. But `extedit` was shared, while depending on the core code. It made the compilation fail with `--no-undefined`.

Fixes #310

* Removed remaining traces of shared libraries

Thanks to @palinek, for finding an old commit.
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 a pull request may close this issue.

3 participants