Skip to content

[CMake] Discourage the use of soversion=ON #18876

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[CMake] Discourage the use of soversion=ON #18876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15f19dd
Revert "[ci] Enable soversion option for all builds"
guitargeek May 27, 2025
243bb25
[CMake] Discourage the use of `soversion=ON`
guitargeek May 27, 2025
06a16be
Update cmake/modules/RootBuildOptions.cmake
guitargeek May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/root-ci-config/buildconfig/global.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ rpath=ON
runtime_cxxmodules=ON
shadowpw=OFF
shared=ON
soversion=ON
soversion=OFF
spectrum=ON
sqlite=ON
ssl=ON
Expand Down
2 changes: 1 addition & 1 deletion cmake/modules/RootBuildOptions.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ ROOT_BUILD_OPTION(rpath ON "Link libraries with built-in RPATH (run-time search
ROOT_BUILD_OPTION(runtime_cxxmodules ON "Enable runtime support for C++ modules")
ROOT_BUILD_OPTION(shadowpw OFF "Enable support for shadow passwords")
ROOT_BUILD_OPTION(shared ON "Use shared 3rd party libraries if possible")
ROOT_BUILD_OPTION(soversion OFF "Set version number in sonames (recommended)")
ROOT_BUILD_OPTION(soversion OFF "Set version number in sonames for shared libraries. Not recommended, as the pcm and rootmap files are unversioned and always point to the non-versioned shared libraries.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ROOT_BUILD_OPTION(soversion OFF "Set version number in sonames for shared libraries. Not recommended, as the pcm and rootmap files are unversioned and always point to the non-versioned shared libraries.")
ROOT_BUILD_OPTION(soversion OFF "Set version number in sonames for shared libraries. Not recommended, as the pcm and rootmap files do not (yet) support versioning and always point to the non-versioned shared libraries.")

Copy link

Choose a reason for hiding this comment

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

Nice idea! If we have a way to proper version the rootmap files and others, we can definitely recommend the soversion=ON. But as long as this is not done by our build system, we can't recommend it.

It shouldn't be all that difficult to do with the current build system. Just install .rootmap in lib/, and include that directory in the system.rootrc as part of the DynamicPath setting. I believe ROOT looks for .rootmap files in that path and those set by some environment variable (not LD_LIBRARY_PATH, if I remember correctly).

Point is, you can do it tomorrow if you want. Better fix this in a proper way right away than keep pushing down the line.

Yours,
Christian

Copy link

Choose a reason for hiding this comment

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

in lib/,

Should have been lib/<version>

Copy link

Choose a reason for hiding this comment

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

Of course, even better if it is in the code to load the .rootmap files.

ROOT_BUILD_OPTION(spectrum ON "Enable support for TSpectrum")
ROOT_BUILD_OPTION(sqlite ON "Enable support for SQLite")
ROOT_BUILD_OPTION(ssl ON "Enable support for SSL encryption via OpenSSL")
Expand Down
Loading