[geom] privatize bvh headers that are only used by cxx files and mv to builtins folder

[ferdymercury]

This Pull request:

Changes or fixes:

They were probably accidentally installed, since they were in the inc/ folder but never listed in the CMakeLists.txt

Never documented, so hard that someone was relying on those

Also move BVH to builtins folder and add missing MIT license file. fyi @ellert (potential for unbundling)

[pcanal]

@sawenzel Note this update/change.

[github-actions]

Test Results

    24 files      24 suites   3d 22h 45m 2s ⏱️
 3 883 tests  3 881 ✅ 0 💤 2 ❌
83 111 runs  83 108 ✅ 0 💤 3 ❌

For more details on these failures, see this check.

Results for commit de8f78b.

♻️ This comment has been updated with latest results.

[sawenzel]

@sawenzel Note this update/change.

This is not ideal. I am accessing these headers elsewhere in O2.

[ferdymercury]

This is not ideal. I am accessing these headers elsewhere in O2.

One question, why not having bvh as a full library ?

Note that this is problematic for future Debian packaging, we are moving away from having external software copy-pasted into ROOT, especially if it has licenses differing from that of ROOT.

I checked the upstram bvh repo, and it installs both headers and CMake targets:

-- Installing: /tmp/bvh/install/include/bvh
-- Installing: /tmp/bvh/install/include/bvh/v2
-- Installing: /tmp/bvh/install/include/bvh/v2/vec.h
-- Installing: /tmp/bvh/install/include/bvh/v2/tri.h
-- Installing: /tmp/bvh/install/include/bvh/v2/top_down_sah_builder.h
-- Installing: /tmp/bvh/install/include/bvh/v2/thread_pool.h
-- Installing: /tmp/bvh/install/include/bvh/v2/sweep_sah_builder.h
-- Installing: /tmp/bvh/install/include/bvh/v2/stream.h
-- Installing: /tmp/bvh/install/include/bvh/v2/sphere.h
-- Installing: /tmp/bvh/install/include/bvh/v2/node.h
-- Installing: /tmp/bvh/install/include/bvh/v2/mini_tree_builder.h
-- Installing: /tmp/bvh/install/include/bvh/v2/index.h
-- Installing: /tmp/bvh/install/include/bvh/v2/executor.h
-- Installing: /tmp/bvh/install/include/bvh/v2/default_builder.h
-- Installing: /tmp/bvh/install/include/bvh/v2/bvh.h
-- Installing: /tmp/bvh/install/include/bvh/v2/binned_sah_builder.h
-- Installing: /tmp/bvh/install/include/bvh/v2/utils.h
-- Installing: /tmp/bvh/install/include/bvh/v2/stack.h
-- Installing: /tmp/bvh/install/include/bvh/v2/split_heuristic.h
-- Installing: /tmp/bvh/install/include/bvh/v2/reinsertion_optimizer.h
-- Installing: /tmp/bvh/install/include/bvh/v2/ray.h
-- Installing: /tmp/bvh/install/include/bvh/v2/platform.h
-- Installing: /tmp/bvh/install/include/bvh/v2/bbox.h
-- Installing: /tmp/bvh/install/include/bvh/v2/c_api/bvh.h
-- Installing: /tmp/bvh/install/lib/cmake/bvh/v2/bvh-targets.cmake
-- Installing: /tmp/bvh/install/lib/cmake/bvh/v2/bvh-config.cmake
-- Installing: /tmp/bvh/install/lib/cmake/bvh/v2/bvh-config-version.cmake

So the idea would be to do in ROOT:

find_package(bvh REQUIRED)
target_link_libraries(Geom PRIVATE bvh)

Then, in O2 you could do the same:

target_link_libraries(O2 PRIVATE ROOT::Geom bvh)

I am accessing these headers elsewhere in O2.

The goal is to rely less on ROOT to serve as package manager and rely more on the OS package manager, that is safer in general since it gets automatic security updates, etc. So if bvh gets added to Debian/Fedora/homebrew, that would be less of an issue. Alternatively, you could add at the beginning of the O2 software a AddExternalProject command that pulls bvh.

Or

FetchContent_Declare(
    bvh
    GIT_REPOSITORY https://github.com/madmann91/bvh/
    GIT_TAG        master # Or commit hash
)
FetchContent_MakeAvailable(bvh)
target_link_libraries(O2 PUBLIC bvh)

[sawenzel]

It is just the way it is at the moment. It can be changed. You may proceed.

[agheata]

OK from my side, the intention was not to expose them, and it is not a native ROOT package (e.g. it is not persisted out of the box). LGTM!

[dpiparo]

Thanks for these changes. Indeed bvh looks indeed like a posterchild for becoming a builtin, maybe initially vendored, then treated as potential system package if the usual requirements are met (mainly installability through apt, dnf and brew)

[ferdymercury]

CI failures seem unrelated

[pcanal]

One of the errors (rebuild leads to unexpected compilation) might be related to this PR or might be related to missing update. Closing and Reopening to rebase the builds.

[pcanal]

@sawenzel @agheata @ferdymercury What is the consensus direction?
a - Use this PR which no longer distribute the headers but risk having conflicting symbols with other uses of this BVH (O2 and/or VecGeom)? (or are we successfully hiding the symbols?)
b - Move forward to using the find_package but offer the user the ability to use a builtin via FetchContent?

[ferdymercury]

a - but risk having conflicting symbols

Since it's an INTERFACE CMake target (header-only library) I think it's not a risk? Worst case we could add CXX_VISIBILITY_PRESET hidden C_VISIBILITY_PRESET hidden to that CMakeLists?

b - Move forward to using the find_package but offer the user the ability to use a builtin via FetchContent?

that would be #23469 but I think it's better to leave that for a future step.

[ferdymercury]

Failures seem unrelated

[sawenzel]

@sawenzel @agheata @ferdymercury What is the consensus direction? a - Use this PR which no longer distribute the headers but risk having conflicting symbols with other uses of this BVH (O2 and/or VecGeom)? (or are we successfully hiding the symbols?) b - Move forward to using the find_package but offer the user the ability to use a builtin via FetchContent?

I think we can go ahead here. VecGeom is not using these headers. It has it's own GPU-capable solution. In Alice O2, I can easily avoid symbol conflicts if they arise.