You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
As I said, this PR seems fine for me and I agree to merge. But perhaps we can go further in improving documentation, configuration and installation of Radium-engine related stuff.
Well, the file can also be generated differently with the same objectives in a more human readable way, which can be helpful in some circumstances, without more effort than a tiny adaptation of your proposal.
The generation of the file is a good thing, but relying on cmake configure_file (https://cmake.org/cmake/help/latest/command/configure_file.html) with variables substitution will help to simplify and generate something more robust :
set_external_dir do not prepend the external installation path with ${CMAKE_INSTALL_PREFIX} (line 48 of cmake/ExternalInclude.cmake) so that externals defines their path in a relative way according to an unknown prefix.
lines 105 to 114 of externals/CMakeLists.txt the script generate a file radium-options.cmake.in with the first (set RADIUM_DEP_PREFIX \"@CMAKE_INSTALL_PREFIX@\" and generates the lines for the configured externals by appending "set(${external}_DIR \"${RADIUM_DEP_PREFIX}/${${external}_DIR}\" CACHE PATH \"My ${external} location\")\n"
once the radium-options.cmake.in is filled, the script call configure_file(radium-options.cmake.in radium-options.cmake @ONLY)
the file radium-options.cmake is then installed using install(FILES radium-options.cmake TYPE DATA)
the installed file will then be available in the directory /path/to/external/install/share
Relying on "install" to put the radium-options.cmake to the right place will need to install externals explicitly and not during build using the sequence cmake build ..... && cmake install ... as for the Radium libs themselves.
If you think externals should be installed automatically at build -time, the radium-options.cmake could be copied at the right place using file(COPY .....) instead of generated directly at this location but this is not the standard way to do that.
But installing explicitly is more secure to relative paths versus absolute path handling and can be related to the recommandation of the documentation at https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html#generating-a-package-configuration-file saying ... under Windows and OSX, where users are used to choose the install location of a binary package at install time, independent from how `CMAKE_INSTALL_PREFIX` was set at build/cmake time.
You fix a problem related to this in this PR (by using TYPE instead of DESTINATION to install doc and licence files, so, why not following the same principle for externals ?
I'll be able to push an implementation of this proposal in the next few days and will be available to discuss that AFK if you prefer.
Well, the file can also be generated differently with the same objectives in a more human readable way, which can be helpful in some circumstances, without more effort than a tiny adaptation of your proposal.
The generation of the file is a good thing, but relying on cmake
configure_file
(https://cmake.org/cmake/help/latest/command/configure_file.html) with variables substitution will help to simplify and generate something more robust :set_external_dir
do not prepend the external installation path with${CMAKE_INSTALL_PREFIX}
(line 48 of cmake/ExternalInclude.cmake) so that externals defines their path in a relative way according to an unknown prefix.externals/CMakeLists.txt
the script generate a fileradium-options.cmake.in
with the first(set RADIUM_DEP_PREFIX \"@CMAKE_INSTALL_PREFIX@\"
and generates the lines for the configured externals by appending"set(${external}_DIR \"${RADIUM_DEP_PREFIX}/${${external}_DIR}\" CACHE PATH \"My ${external} location\")\n"
radium-options.cmake.in
is filled, the script callconfigure_file(radium-options.cmake.in radium-options.cmake @ONLY)
radium-options.cmake
is then installed usinginstall(FILES radium-options.cmake TYPE DATA)
/path/to/external/install/share
Relying on "install" to put the
radium-options.cmake
to the right place will need to install externals explicitly and not during build using the sequencecmake build ..... && cmake install ...
as for the Radium libs themselves.If you think externals should be installed automatically at build -time, the
radium-options.cmake
could be copied at the right place usingfile(COPY .....)
instead of generated directly at this location but this is not the standard way to do that.But installing explicitly is more secure to relative paths versus absolute path handling and can be related to the recommandation of the documentation at https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html#generating-a-package-configuration-file saying
... under Windows and OSX, where users are used to choose the install location of a binary package at install time, independent from how `CMAKE_INSTALL_PREFIX` was set at build/cmake time.
You fix a problem related to this in this PR (by using
TYPE
instead ofDESTINATION
to install doc and licence files, so, why not following the same principle for externals ?I'll be able to push an implementation of this proposal in the next few days and will be available to discuss that AFK if you prefer.
Originally posted by @MathiasPaulin in #1016 (comment)
The text was updated successfully, but these errors were encountered: