Skip to content

Add soname to libShowMySky #8

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

Merged
merged 1 commit into from
Oct 7, 2022
Merged

Add soname to libShowMySky #8

merged 1 commit into from
Oct 7, 2022

Conversation

mattiaverga
Copy link
Contributor

To meet criteria for packaging CalcMySky into Fedora, I'd like to propose to add soname to libShowMySky.
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=2131842

@10110111 10110111 merged commit 45a3541 into 10110111:master Oct 7, 2022
@DarthGandalf
Copy link
Contributor

Reading the bug linked, I don't think soname by itself will help. You'll also need to patch Stellarium to open libShowMySky.so.0 in runtime instead of libShowMySky.so

@10110111
Copy link
Owner

10110111 commented Oct 7, 2022

Reading the bug linked, I don't think soname by itself will help.

Why? The bug talks about policy (comment 3) and package splitting (comment 6), not solving technical problems, and in this case adding soname seems to be sufficient.

@DarthGandalf
Copy link
Contributor

The libShowMySky.so would be in -devel package, and libShowMySky.so.0 would be in the main package. So if stellarium tries to open libShowMySky.so, it still needs to depend on the devel package.

Libraries which link during build time, don't have this problem because the executable remembers that it needs to load .so.0. That's why .so is only useful during build time to resolve -lShowMySky-like flag to ld

@10110111
Copy link
Owner

10110111 commented Oct 7, 2022

Hmm, you're right. Just checked, indeed it doesn't load if I remove libShowMySky.so.

@DarthGandalf
Copy link
Contributor

/usr/bin/showmysky itself also opens it like that, so would also need to be moved to the devel package, or patched

@10110111
Copy link
Owner

10110111 commented Oct 7, 2022

I think I'll add a version argument to the QLibrary constructor. This way it should work in the same manner as direct linking does. A quick check shows that it works.

@10110111
Copy link
Owner

10110111 commented Oct 7, 2022

Pushed 8e535d2.

@mattiaverga
Copy link
Contributor Author

Sorry, I admit I know nearly nothing of programming and I did a shot in the dark here.

So, I assume that also stellarium needs a way to read the value of ShowMySky_ABI_version during build time and add it to the QLibrary constructor?

@10110111
Copy link
Owner

10110111 commented Oct 8, 2022

With current master of CalcMySky the patch for vanilla Stellarium v1.0 should look something like this:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 902239662d..ef21a6b872 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -685,10 +685,10 @@ SET(CMAKE_AUTOUIC ON) # ?
 SET(CMAKE_INCLUDE_CURRENT_DIR ON)
 
 IF(ENABLE_SHOWMYSKY)
-    FIND_PACKAGE(ShowMySky REQUIRED)
+    FIND_PACKAGE(ShowMySky-Qt${QT_VERSION_MAJOR} REQUIRED)
     ADD_DEFINITIONS(-DENABLE_SHOWMYSKY)
-    GET_TARGET_PROPERTY(ShowMySky_INCLUDE_DIRECTORIES ShowMySky::ShowMySky INTERFACE_INCLUDE_DIRECTORIES)
-    GET_TARGET_PROPERTY(ShowMySky_LIBRARY ShowMySky::ShowMySky LOCATION)
+    GET_TARGET_PROPERTY(ShowMySky-Qt${QT_VERSION_MAJOR}_INCLUDE_DIRECTORIES ShowMySky::ShowMySky INTERFACE_INCLUDE_DIRECTORIES)
+    GET_TARGET_PROPERTY(ShowMySky-Qt${QT_VERSION_MAJOR}_LIBRARY ShowMySky::ShowMySky LOCATION)
     IF(EXISTS ${ShowMySky_LIBRARY})
     MESSAGE(STATUS "Found ShowMySky library: ${ShowMySky_LIBRARY}")
     ELSE()
diff --git a/src/core/modules/AtmosphereShowMySky.cpp b/src/core/modules/AtmosphereShowMySky.cpp
index 41b3538ca3..f58406d09c 100644
--- a/src/core/modules/AtmosphereShowMySky.cpp
+++ b/src/core/modules/AtmosphereShowMySky.cpp
@@ -407,7 +407,7 @@ void AtmosphereShowMySky::resolveFunctions()
 }
 
 AtmosphereShowMySky::AtmosphereShowMySky()
-    : showMySkyLib("ShowMySky")
+    : showMySkyLib(SHOWMYSKY_LIB_NAME, ShowMySky_ABI_version)
     , viewport(0,0,0,0)
     , gridMaxY(44)
     , gridMaxX(44)

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.

3 participants