Skip to content

Conversation

@dimitrivlachos
Copy link
Collaborator

@dimitrivlachos dimitrivlachos commented Nov 6, 2025

Previously, the installation and export sections in CMakeLists.txt were
commented out to streamline development. This worked well for rapid
iteration but prevented proper library installation, which is now
required for building Docker containers with cleanly separated build and
runtime environments.

This PR re-enables and extends the installation system to support both
submodule usage and standalone installation, while dynamically handling
dependencies to avoid duplication and export conflicts.

  • Add installation targets for dx2 library, headers, and CMake package
    config files.
  • Re-enable CMake export system to support find_package(DX2) in
    downstream projects.
  • Conditionally install FetchContent dependencies (nlohmann_json, fmt)
    only when they were fetched rather than found via system packages,
    preventing duplicate installations and export conflicts.
  • Add STREQUAL fix for proper CMake path comparison in root detection.

These changes enable proper Docker container builds with managed
dependencies while maintaining full compatibility with existing
submodule usage patterns and adding find_package support for projects
that prefer system-managed installations.

@dimitrivlachos dimitrivlachos self-assigned this Nov 6, 2025
@dimitrivlachos dimitrivlachos added the enhancement New feature or request label Nov 6, 2025
@jbeilstenedmands
Copy link
Contributor

Hi, I tried testing this with the ffs, however I encountered an error at the CMake Generate step:

CMake Error in dx2/CMakeLists.txt:
  install(EXPORT "DX2Targets" ...) includes target "dx2" which requires
  target "nlohmann_json" that is not in any export set.

Reading around, it seems that a solution is to always export nlohmann_json like:

if(TARGET nlohmann_json)
    install(TARGETS nlohmann_json EXPORT DX2Targets)
endif()

which then builds successfully for me. However it's not clear to me why the same issue wouldn't arise with fmt where you use a similar pattern (I guess it's down to differences in how the installs are defined in their respective CMake files).
Also it's not clear why I would get this error if you didn't 🤔

Further reading around suggests it might be cleaner to use find_package here rather than FetchContent, which would change nlohmann_json from an optional to required dependency - but we are heavily tied into this so that doesn't sound like an issue to me. But I haven't tried this so perhaps it brings other issues.

@ndevenish ndevenish self-requested a review November 7, 2025 09:10
Copy link
Contributor

@jbeilstenedmands jbeilstenedmands left a comment

Choose a reason for hiding this comment

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

Thanks, I can confirm this now work as expected for me.

@dimitrivlachos dimitrivlachos merged commit e6720b7 into main Nov 7, 2025
5 checks passed
@dimitrivlachos dimitrivlachos deleted the lib_install branch November 7, 2025 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants