Switch to dynamic libraries #1512

lbonn · 2020-01-08T15:58:43Z

Besides the obvious reuse we have between aktualizr and aktualizr-info, it might be time to ship libaktualizr as a standalone library.
For now, aktualizr_static_lib still exists, we might remove it at a later point.

This will also allow some simplifications in #1498.

pattivacek

Seems pretty cool.

pattivacek · 2020-01-08T16:08:28Z

src/libaktualizr/CMakeLists.txt

-target_include_directories(aktualizr_static_lib PUBLIC
-    $<TARGET_PROPERTY:package_manager,INCLUDE_DIRECTORIES>)
+target_include_directories(aktualizr_static_lib PUBLIC $<TARGET_PROPERTY:package_manager,INCLUDE_DIRECTORIES>)
+target_include_directories(aktualizr_lib PUBLIC $<TARGET_PROPERTY:package_manager,INCLUDE_DIRECTORIES>)


Why are these special bits for package_manager required?

To have access to ostree.h. I can double check that but it seems necessary.

Ok, actually it doesn't look like it's making a difference... Removed in the last version.

pattivacek · 2020-01-08T16:09:57Z

src/sota_tools/CMakeLists.txt

-    ${OPENSSL_LIBRARIES}
-    ${sodium_LIBRARY_RELEASE}
-    ${GLIB2_LIBRARIES})
-
 if (BUILD_SOTA_TOOLS)


The endif and if here are redundant now.

pattivacek · 2020-01-08T16:14:27Z

CMakeLists.txt

@@ -213,6 +213,9 @@ if (CMAKE_CXX_COMPILER_ID MATCHES "(GNU|Clang)")
    endif ()
 endif()

+# helpful for debugging installed executables, see `man ld-linux.so`
+set(CMAKE_INSTALL_RPATH "$ORIGIN/../lib")


Do we really want this? I've had bad experiences with rpaths.

It should be for comfort only, I'll try without.

Specifically, this is the difference:

With:

$ DESTDIR=/tmp/test ninja install $ /tmp/test/usr/local/bin/aktualizr --version # works

without:

$ DESTDIR=/tmp/test ninja install $ /tmp/test/usr/local/bin/aktualizr --version # does not work $ LD_LIBRARY_PATH=/tmp/test/usr/local/bin /tmp/test/usr/local/bin/aktualizr --version # works

But then, it might be bad practice and cause other issues. Do you remember some details about the issues you had?

The problem with using rpath is that you're forcing your path structure on your users. It means that when dynamically linking, everything in "$ORIGIN/../lib" will be taken over anything in the user's $LD_LIBRARY_PATH, which can have unintended side effects.

There are two points of view on this depending on what you think one's users prefer. One is that rpath makes it easier to find your library, but the downside is messing around with link path searching. The other is that rpath breaks traditional Linux norms and you should only really install things into default locations or change your PATH and/or LD_LIBRARY_PATH accordingly.

My preference is the latter, but I'm not set on that. Our circumstances are quite different than my past experience. If this makes things easier, it's fine. It does seem it'd make testing locally a bit easier.

My view is that if a user installs it to a non-standard path, it is up to them to adjust LD_LIBRARY_PATH accordingly.

The problem with using rpath is that you're forcing your path structure on your users. It means that when dynamically linking, everything in "$ORIGIN/../lib" will be taken over anything in the user's $LD_LIBRARY_PATH, which can have unintended side effects.

According to the ld-linux.so's manual page, and a quick test, this doesn't seem to be the case. What CMake calls RPATH is actually RUNPATH which has a lower priority than LD_LIBRARY_PATH:

readelf -a usr/local/bin/aktualizr | grep PATH 0x000000000000001d (RUNPATH) Library runpath: [$ORIGIN/../lib]

That said, the risks of confusion might indeed outweigh the benefits, so I've removed it for now.

What CMake calls RPATH is actually RUNPATH

Wow, that is actually the most confusing part of all of this! If it's actually just setting RUNPATH, I don't really mind it. For now, though, if you've already removed it, let's see how it goes without it, and we can add it later if we think it'd be helpful.

test_aktualizr_secondary_uptane_verification uses ostree Signed-off-by: Laurent Bonnans <[email protected]>

Identical functionality can directly be achieved through CMake Signed-off-by: Laurent Bonnans <[email protected]>

Signed-off-by: Laurent Bonnans <[email protected]>

codecov-io · 2020-01-09T10:35:21Z

Codecov Report

Merging #1512 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1512      +/-   ##
==========================================
+ Coverage    80.7%   80.72%   +0.02%     
==========================================
  Files         184      184              
  Lines       11156    11156              
==========================================
+ Hits         9003     9006       +3     
+ Misses       2153     2150       -3

Impacted Files	Coverage Δ
src/libaktualizr/primary/sotauptaneclient.cc	`88.71% <0%> (-0.15%)`	⬇️
src/libaktualizr/storage/sqlstorage.cc	`76.56% <0%> (ø)`	⬆️
src/libaktualizr/isotp_conn/isotp_conn.cc	`0.97% <0%> (+0.97%)`	⬆️
src/libaktualizr/uptane/isotpsecondary.cc	`1.33% <0%> (+1.33%)`	⬆️
src/libaktualizr/package_manager/ostreereposync.cc	`1.35% <0%> (+1.35%)`	⬆️
src/libaktualizr/storage/sql_utils.h	`86.11% <0%> (+1.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff077a7...fce010b. Read the comment docs.

pattivacek · 2020-01-09T11:02:05Z

src/sota_tools/CMakeLists.txt

-endif (BUILD_SOTA_TOOLS)
-
+# do not link tests with libaktualizr
+list(REMOVE_ITEM TEST_LIBS aktualizr_lib)


I thought that the garage tools used some of functionality from libaktualizr, like Utils and such. Is that linked in still somehow, or how does that work now?

It's now included in sota_tools_lib which links statically with libaktualizr. I've pushed some new changes to make it clearer.

* fix linking issues with secondary tests These are difficult to link as they use parts of the primary and secondary libs which don't play well with each others. * link sota_tools_lib with aktualizr_static_lib to make it self-contained Signed-off-by: Laurent Bonnans <[email protected]> fixup! Fix linking issues with secondary tests

lbonn requested review from pattivacek, eu-siemann, mike-sul, xcheng-here, kbushgit and mchekhovoi January 8, 2020 15:58

lbonn mentioned this pull request Jan 8, 2020

Adapt to shared libraries changes in aktualizr advancedtelematic/meta-updater#659

Merged

pattivacek reviewed Jan 8, 2020

View reviewed changes

lbonn added 3 commits January 9, 2020 11:23

Fix with tests build without BUILD_OSTREE

e1c1819

test_aktualizr_secondary_uptane_verification uses ostree Signed-off-by: Laurent Bonnans <[email protected]>

Remove special launcher for debian test

8b6792c

Identical functionality can directly be achieved through CMake Signed-off-by: Laurent Bonnans <[email protected]>

Use libaktualizr as shared library

89d34bd

Signed-off-by: Laurent Bonnans <[email protected]>

lbonn force-pushed the feat/dyn-library branch from f78aebd to c8379e3 Compare January 9, 2020 10:35

lbonn mentioned this pull request Jan 9, 2020

Adapt to shared libraries changes in aktualizr (bis) advancedtelematic/meta-updater#660

Merged

pattivacek reviewed Jan 9, 2020

View reviewed changes

lbonn force-pushed the feat/dyn-library branch from c8379e3 to fce010b Compare January 9, 2020 11:06

eu-siemann approved these changes Jan 9, 2020

View reviewed changes

pattivacek approved these changes Jan 9, 2020

View reviewed changes

lbonn merged commit bc54d32 into master Jan 9, 2020

lbonn deleted the feat/dyn-library branch January 9, 2020 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to dynamic libraries #1512

Switch to dynamic libraries #1512

lbonn commented Jan 8, 2020

pattivacek left a comment

pattivacek Jan 8, 2020

lbonn Jan 8, 2020

lbonn Jan 9, 2020

pattivacek Jan 8, 2020

pattivacek Jan 8, 2020

lbonn Jan 8, 2020

lbonn Jan 9, 2020

pattivacek Jan 9, 2020

eu-siemann Jan 9, 2020

lbonn Jan 9, 2020

pattivacek Jan 9, 2020

codecov-io commented Jan 9, 2020 •

edited

Loading

pattivacek Jan 9, 2020

lbonn Jan 9, 2020

Switch to dynamic libraries #1512

Switch to dynamic libraries #1512

Conversation

lbonn commented Jan 8, 2020

pattivacek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 9, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 9, 2020 •

edited

Loading