[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

    23 files      23 suites   3d 22h 18m 41s ⏱️
 3 879 tests  3 879 ✅ 0 💤 0 ❌
80 093 runs  80 093 ✅ 0 💤 0 ❌

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)