build: add soname to shared libraries #2521

bkmgit · 2024-10-27T04:39:02Z

bkmgit · 2024-10-27T04:41:03Z

At present the version is defined in the CMakeLists.txt file. It could be defined elsewhere
so that when a new release is made, the tag is also automatically defined. Currently the
tag seems to be defined manually.

tchaikov

i'd use SEASTAR_API_LEVEL for the SO version.

CMakeLists.txt

avikivity · 2024-10-27T17:58:49Z

Please explain in the patch changelog why we're making this change.

bkmgit · 2024-10-28T02:28:57Z

Please explain in the patch changelog why we're making this change.

Done.

tchaikov

modulo a nit, lgtm

CMakeLists.txt

tchaikov · 2024-10-28T13:56:24Z

@bkmgit hi Benson, just out of curiosity, do you really plan to package seastar for a distro like fedora?

avikivity · 2024-10-28T19:17:44Z

CMakeLists.txt

@@ -780,6 +780,11 @@ add_library (seastar
  src/websocket/server.cc
  )



Use API level as soname to enable applications to select
and link agains shared libraries based on their SO versions.

Actually a neat idea. But note, API levels are only about source-level compatibility. It's possible for a function signature to change without change the API level.

Example: if we add a trailing default parameters to a function, it's still source compatible, but links against an older .so will fail.

Given this, is this still helpful for you?

Am happy to follow up with another change. It would be nice not to carry patches, but yes ideally soname would reflect ABI compatibility, not API compatibility. Still learning about Seastar, so not sure how often ABI is broken in a backwards incompatible manner. If the release year and month is used, then this would always increment the soname, and it would be assumed that typically ABI is broken on each update.

One could run a tool such as libabigail when doing a release and then automatically bump the soname if there is an ABI change.

bkmgit · 2024-10-28T19:24:49Z

@bkmgit hi Benson, just out of curiosity, do you really plan to package seastar for a distro like fedora?

https://bugzilla.redhat.com/show_bug.cgi?id=2321986

DPDK is already there, but need to see if changes allow it to be used.
Scylladb would be nice as well.

tchaikov · 2024-10-29T05:50:29Z

@bkmgit hi Benson, just out of curiosity, do you really plan to package seastar for a distro like fedora?

https://bugzilla.redhat.com/show_bug.cgi?id=2321986

DPDK is already there, but need to see if changes allow it to be used. Scylladb would be nice as well.

thanks . i see. regarding DPDK, one thing i can think about is RTE_MBUF_REFCNT_ATOMIC. because seastar is inherently sharded by CPU cores, so we don't need to enable RTE_MBUF_REFCNT_ATOMIC. this could hurt the performance because the overhead of atomic operations. but this is not the end of world.

Avi's comment about SO versioning makes a lot of sense. I hadn't considered that perspective before. we even sometimes unintentionally change APIs over time, as seen in #2410 (comment) . and AFAICT, we don't have a release schedule (yet). so probably it's best to encode date in the version number to be on the safe side, or just starts with "1" and increment it every time a new release is created, and the ABI is changed across the builds. sorry for the bike shedding. BTW, if we want to use date as the version number, probably should include the month in it. and we probably can set the version in the command line, like cmake -DPROJECT_VERSION=23.3.14 or cmake -DPROJECT_VERSION=202410.3.14, and use PROJECT_VERSION_MAJOR for so version. or, just pass the Seastar_VERSION_MAJOR to cmake.

bkmgit · 2024-10-30T03:54:47Z

As the version is currently 1, it seems ok to use that as the soname, but use the date as the software version number since that is the tag that is used in GitHub. Perhaps we can automate this to be updated whenever a release is made rather than manually setting a CMake variable. It will be less prone to error. Will update the pull request with this.

Use PROJECT_VERSION.API_LEVEL.RELEASE_DATE as soname to enable applications to select and link against shared libraries based on their SO versions.

bkmgit · 2024-11-03T04:14:58Z

Have used a versioning scheme of
PROJECT_VERSION.API_LEVEL.RELEASE_DATE

If this is ok, can add tooling that would update this automatically whenever a release is made by checking for ABI changes. If someone is using a commit that is different from a release, it would be upto them to check ABI compatibility. Tooling can be done in a follow up pull request.

bkmgit · 2024-11-03T04:15:35Z

PROJECT_VERSION is currently 1, so can keep that.

avikivity · 2024-11-03T14:02:20Z

@bkmgit hi Benson, just out of curiosity, do you really plan to package seastar for a distro like fedora?

https://bugzilla.redhat.com/show_bug.cgi?id=2321986

DPDK is already there, but need to see if changes allow it to be used. Scylladb would be nice as well.

Note, ScyllaDB releases use a fork of Seastar in https://github.com/scylladb/scylla-seastar.git. The fork is created on-demand (if we need to backport a Seastar commit), so not all releases reference it.

See scylladb/scylladb@6d779f5 (branch-5.4) and following submodule updates for an example. It's really geared towards bundling, not independently moving versions.

bkmgit · 2024-11-04T06:35:33Z

Thanks. Will bundle when packaging Scylladb. Having other consumers of seastar would improve testing. Bundling multiple times is not great and it is already used in a number of projects Packages make deployments easier, though usually with a performance tradeoff. However, understand if it is too much extra effort unrelated to the main project goals to support a soname.

tchaikov · 2024-11-05T02:29:41Z

CMakeLists.txt

+  STRING
+  "Last release date")
+
+set_property (CACHE Seastar_RELEASE_DATE


why do we need a property for the release date?

tchaikov · 2024-11-05T02:46:12Z

CMakeLists.txt

@@ -780,6 +790,15 @@ add_library (seastar
  src/websocket/server.cc
  )

+# Shared library soname  
+set_target_properties(seastar PROPERTIES
+  VERSION ${PROJECT_VERSION}.${Seastar_API_LEVEL}.${Seastar_RELEASE_DATE}


In semantic versioning (semver), a version number consists of three components: MAJOR.MINOR.PATCH. The major version number changes least frequently and indicates incompatible API changes. The minor version is incremented more often for backward-compatible feature additions, while the patch version is used for backward-compatible bug fixes.

The Seastar_API_LEVEL changes less frequently than the ABI version, as it is only incremented for significant API changes rather than ABI-breaking modifications.

PROJECT_VERSION is also composed of multiple component. see https://cmake.org/cmake/help/latest/variable/PROJECT_VERSION.html.

Based on the discussion above, I suggest:

PROJECT_VERSION should be treated as a versioning string with multiple components (major.minor.patch) rather than a single number.

Although Seastar_API_LEVEL is crucial for API versioning, it may not belong in the libraries' version numbers since we make ABI changes within the same Seastar_API_LEVEL.

Including the release date in the version number makes sense given our flexible release schedule.

tchaikov · 2024-11-05T02:47:25Z

CMakeLists.txt

+
+# We disable _FORTIFY_SOURCE because it generates false positives with longjmp() (src/core/thread.cc)
+set_source_files_properties(src/core/thread.cc
+  PROPERTIES COMPILE_FLAGS -U_FORTIFY_SOURCE)


you are partially reverting c96444f. probably want to drop this change?

bkmgit · 2024-11-13T10:16:43Z

Thanks for the suggestions.

tchaikov suggested changes Oct 27, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

bkmgit force-pushed the library-soname branch 2 times, most recently from 3b1a5a4 to cd7be4b Compare October 28, 2024 02:17

bkmgit force-pushed the library-soname branch from cd7be4b to d46ec1d Compare October 28, 2024 02:34

tchaikov approved these changes Oct 28, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

bkmgit force-pushed the library-soname branch from d46ec1d to 60bbb7e Compare October 28, 2024 19:14

avikivity reviewed Oct 28, 2024

View reviewed changes

bkmgit force-pushed the library-soname branch from 60bbb7e to f31f8ce Compare October 28, 2024 19:36

build: add soname to shared libraries

caac782

Use PROJECT_VERSION.API_LEVEL.RELEASE_DATE as soname to enable applications to select and link against shared libraries based on their SO versions.

bkmgit force-pushed the library-soname branch from f31f8ce to caac782 Compare November 3, 2024 04:11

tchaikov self-requested a review November 5, 2024 02:28

tchaikov reviewed Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add soname to shared libraries #2521

build: add soname to shared libraries #2521

bkmgit commented Oct 27, 2024

bkmgit commented Oct 27, 2024

tchaikov left a comment

avikivity commented Oct 27, 2024

bkmgit commented Oct 28, 2024

tchaikov left a comment

tchaikov commented Oct 28, 2024 •

edited

Loading

avikivity Oct 28, 2024

bkmgit Oct 28, 2024

bkmgit Oct 28, 2024

bkmgit commented Oct 28, 2024 •

edited

Loading

tchaikov commented Oct 29, 2024 •

edited

Loading

bkmgit commented Oct 30, 2024

bkmgit commented Nov 3, 2024

bkmgit commented Nov 3, 2024

avikivity commented Nov 3, 2024

bkmgit commented Nov 4, 2024

tchaikov Nov 5, 2024

tchaikov Nov 5, 2024

tchaikov Nov 5, 2024

bkmgit commented Nov 13, 2024

		@@ -780,6 +780,11 @@ add_library (seastar
		src/websocket/server.cc
		)

build: add soname to shared libraries #2521

Are you sure you want to change the base?

build: add soname to shared libraries #2521

Conversation

bkmgit commented Oct 27, 2024

bkmgit commented Oct 27, 2024

tchaikov left a comment

Choose a reason for hiding this comment

avikivity commented Oct 27, 2024

bkmgit commented Oct 28, 2024

tchaikov left a comment

Choose a reason for hiding this comment

tchaikov commented Oct 28, 2024 • edited Loading

avikivity Oct 28, 2024

Choose a reason for hiding this comment

bkmgit Oct 28, 2024

Choose a reason for hiding this comment

bkmgit Oct 28, 2024

Choose a reason for hiding this comment

bkmgit commented Oct 28, 2024 • edited Loading

tchaikov commented Oct 29, 2024 • edited Loading

bkmgit commented Oct 30, 2024

bkmgit commented Nov 3, 2024

bkmgit commented Nov 3, 2024

avikivity commented Nov 3, 2024

bkmgit commented Nov 4, 2024

tchaikov Nov 5, 2024

Choose a reason for hiding this comment

tchaikov Nov 5, 2024

Choose a reason for hiding this comment

tchaikov Nov 5, 2024

Choose a reason for hiding this comment

bkmgit commented Nov 13, 2024

tchaikov commented Oct 28, 2024 •

edited

Loading

bkmgit commented Oct 28, 2024 •

edited

Loading

tchaikov commented Oct 29, 2024 •

edited

Loading