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.

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

Conversation

guitargeek
Copy link
Contributor

Discourage the use of soversion=ON because the rootmap files don't
support it. Therefore, this option has no clear benefit. It seemingly
allows you to have multiple ROOT versions in parallel on the system, by
things will break anyway since e.g. the rootmap files are unversioned.

Discourage the use of `soversion=ON` because the rootmap files don't
support it. Therefore, this option has no clear benefit. It seemingly
allows you to have multiple ROOT versions in parallel on the system, by
things will break anyway since e.g. the rootmap files are unversioned.
Copy link
Member

@bellenot bellenot left a comment

Choose a reason for hiding this comment

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

LGTM, but see my review. I'll let you take the final decision

Copy link

github-actions bot commented May 27, 2025

Test Results

    19 files      19 suites   3d 8h 47m 18s ⏱️
 2 777 tests  2 777 ✅ 0 💤 0 ❌
51 267 runs  51 267 ✅ 0 💤 0 ❌

Results for commit 06a16be.

♻️ This comment has been updated with latest results.

@guitargeek guitargeek added the clean build Ask CI to do non-incremental build on PR label May 27, 2025
@guitargeek guitargeek closed this May 27, 2025
@guitargeek guitargeek reopened this May 27, 2025
@cholmcc
Copy link

cholmcc commented Jun 6, 2025

Just to be clear - numbers in so-names makes the runtime environment much more stable. An application linked to ROOT X.Y.Z will not use libraries from ROOT X.Y.W. As for the rootmap files - yes, they are not versioned (but probably should be). A simple fix is that you install .rootmap files in a versioned sub-directory of lib - e.g., lib/X.Y.Z - along with any plugin "libraries". This is the only way to make a clean system that can accommodate multiple run-time environments and which adhere to the FHS.

@ferdymercury
Copy link
Collaborator

which adhere to the FHS.

Cross-link: https://its.cern.ch/jira/browse/ROOT-10151

@guitargeek
Copy link
Contributor Author

guitargeek commented Jun 6, 2025

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.

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean build Ask CI to do non-incremental build on PR in:Build System in:CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants