Skip to content

Commit

Permalink
Fix SOVERSION of shared library
Browse files Browse the repository at this point in the history 
Fixes #1434
  • Loading branch information
@dmah42
dmah42 committed Jul 18, 2022
1 parent 7a2024e commit d4bc509
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ add_library(benchmark::benchmark ALIAS benchmark)
set_target_properties(benchmark PROPERTIES
OUTPUT_NAME "benchmark"
VERSION ${GENERIC_LIB_VERSION}
SOVERSION 2
SOVERSION ${GENERIC_LIB_SOVERSION}
)
target_include_directories(benchmark PUBLIC
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
Expand Down

10 comments on commit d4bc509

@sergiud
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in #1434 (comment), this change is incorrect due to ABI incompatibility to previous release. SOVERSION does not refer to the major version of the library. More info here: https://cmake.org/cmake/help/latest/prop_tgt/SOVERSION.html.

@dmah42
Copy link
Member Author

@dmah42 dmah42 commented on d4bc509 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm missing something: why is it ABI incompatible? also this change was made based on package maintainers expecting the SO version to match the major version of the library (as i do).

@sergiud
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you need setting SOVERSION if it's the same as the major version anyway? If you strictly follow semantic versioning and ensure no ABI breaks, then keeping SOVERSION and major version in sync makes slightly more sense.

Regarding ABI incompatibility, see #1321. Previously, many internal symbols were exposed. This is no longer the case.

Either way, it's news to me that package maintainers get to decide how upstream versions its library. Particularly, it is not clear why someone would expect the SONAME to remain the same between releases. Such an expectation must completely ignore the purpose of an SONAME.

@dmah42
Copy link
Member Author

@dmah42 dmah42 commented on d4bc509 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the concern in full:
"I built the updated package but noticed some strange soname versions:

/usr/lib/libbenchmark.so.1 and /usr/lib/libbenchmark_main.so.1 are now both at
version 2 (libbenchmark.so.2 and libbenchmark_main.so.2) while the links with
the full version number are still named libbenchmark.so.1.6.1 and
libbenchmark_main.so.1.6.1 instead of libbenchmark.so.1.6.2 and
libbenchmark_main.so.1.6.2"

ie, there's an expectation (which i share) that SO version follows major version. i didn't consider the removal of internal symbols as ABI incompatible precisely because they were previously clearly labeled as internal (non-public) albeit with a sign, not a cop.

i would prefer to maintain a link between major version and SO version, though i must admit that i generally use minor version to indicate some part of the public API has changed (which is not strictly semantically valid.. maybe that should be reconsidered).

@sergiud
Copy link
Contributor

@sergiud sergiud commented on d4bc509 Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concern "strange name" is not valid, neither is the expectation given benchmark does not follow semantic versioning (by your own admission.) Removal of internal symbols exactly results in an ABI incompatibility because users cannot access (all) internal symbols anymore which was previously possible.

Aside from the muddle this changed has caused, there's no technical reason to keep SOVERSION at 1. Therefore, I recommend reverting the change.

@dmah42
Copy link
Member Author

@dmah42 dmah42 commented on d4bc509 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be even more correct to release 1.7 (indicating a more significant change) and set the SOVERSION to 7? then follow the minor version from that point on?

@sergiud
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that would be wrong. However, skipping several SOVERSIONs will only cause more confusion. Generally speaking, every time a backward incompatible change is made, SOVERSION should be incremented. This is sufficient as a versioning strategy.

The alternative is to drop SOVERSION altogether since it does not (currently) convey any meaningful information.

@dmah42
Copy link
Member Author

@dmah42 dmah42 commented on d4bc509 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you agree that it doesn't convey meaningful information then leaving it as 1 should be sufficient too.

@sergiud
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a false premise. If you provide the attribute, one assumes the information is in fact meaningful. By dropping SOVERSION you explicitly state the version conveys all the information you need. Implied information is clearly subject to interpretation.

@dmah42
Copy link
Member Author

@dmah42 dmah42 commented on d4bc509 Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a completely fair point.

i'm comfortable dropping it.

Please sign in to comment.