10110111 / CalcMySky

Simulator of light scattering by planetary atmospheres
GNU General Public License v3.0
29 stars 7 forks source link

Add soname to libShowMySky #8

Closed mattiaverga closed 1 year ago

mattiaverga commented 1 year ago

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

DarthGandalf commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

DarthGandalf commented 1 year ago

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

10110111 commented 1 year ago

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 commented 1 year ago

Pushed 8e535d2d6e8c44ec4e6266f6ce5da00870c5a706.

mattiaverga commented 1 year ago

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 commented 1 year ago

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)