Skip to content

Conversation

@ghisvail
Copy link
Contributor

@ghisvail ghisvail commented Aug 4, 2015

lib/clSPARSE/cmake -> lib/cmake/clSPARSE

lib/clSPARSE/cmake -> lib/cmake/clSPARSE
@kknox
Copy link
Contributor

kknox commented Aug 5, 2015

I may not understand this completely, but this PR seems to be not needed? I am looking at the find_package() documentation, and the path searching logic reads:

<prefix>/                                               (W)
<prefix>/(cmake|CMake)/                                 (W)
<prefix>/<name>*/                                       (W)
<prefix>/<name>*/(cmake|CMake)/                         (W)
<prefix>/(lib/<arch>|lib|share)/cmake/<name>*/          (U)
<prefix>/(lib/<arch>|lib|share)/<name>*/                (U)
<prefix>/(lib/<arch>|lib|share)/<name>*/(cmake|CMake)/  (U)

My original path should correspond to the 4th row on windows and the 7th row on unix? Where does the cmake string precede the lib name? Is it not working for you?

@ghisvail
Copy link
Contributor Author

ghisvail commented Aug 5, 2015

The unofficial consensus on Linux is to use option 5. Never came across a project using option 7 TBH. Besides, this would make things consistent with other clMath projects and ArrayFire, which is always welcome from a package maintenance point-of-view.

Your call.

@kknox
Copy link
Contributor

kknox commented Aug 6, 2015

Strange how the eyes skip strings, they didn't register option 5. It's marked U, have you tested that path on windows?

@ghisvail
Copy link
Contributor Author

ghisvail commented Aug 6, 2015

have you tested that path on windows?

No

For Windows, I would rather rely on the user providing a set of paths via the CMAKE_MODULE_PATH cmake option, since there is no such things as a common installation tree like /usr in Linux. Most projects have dealt with either use the same as on Linux (option 5) or use option 2 if windows is detected.

Let me insist on this issue from a packaging point-of-view. I am the current maintainer of clBLAS, clFFT, and I am intending to package clSPARSE and clRNG as well. All these projects have similar build structures and cmake lists layout, which means I can share patches between projects resulting in significantly less maintenance overhead.

@kknox
Copy link
Contributor

kknox commented Aug 6, 2015

If you could, please link to other projects that use the configure_package_config_file() macro. It would be good to see/study how others do it.
I will consider the change, but I need time to see how it affects windows. I need something that works cross-platform.

@ghisvail
Copy link
Contributor Author

ghisvail commented Aug 6, 2015

Please correct me if I am wrong but all CONFIGURE_PACKAGE_CONFIG_FILE does is create a sane and relocatable FooConfig.cmake. So whether a project uses this macro or not does not matter for this issue, as long as the resulting FooConfig.cmake file is installed in an appropriate location for the cmake detection to work.

Then, an example from another clMathLibraries project, clFFT:

if(WIN32)
  set(destdir CMake)
else()
  set(destdir share/clFFT)
endif()
string(REGEX REPLACE "[^/]+" ".." reldir "${destdir}")
configure_file(
  ${CMAKE_CURRENT_SOURCE_DIR}/clFFTConfigVersion.cmake.in
  ${CMAKE_CURRENT_BINARY_DIR}/clFFTConfigVersion.cmake
  @ONLY)
configure_file(
  ${CMAKE_CURRENT_SOURCE_DIR}/clFFTConfig.cmake.in
  ${CMAKE_CURRENT_BINARY_DIR}/clFFTConfig.cmake
  @ONLY)
install(EXPORT Library DESTINATION ${destdir} FILE clFFTTargets.cmake)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/clFFTConfigVersion.cmake
  ${CMAKE_CURRENT_BINARY_DIR}/clFFTConfig.cmake
  DESTINATION ${destdir})

which I am currently patching share/clFFT to become lib/<arch>/cmake/clFFT. Hence, myself asking for a little bit of consistency...

kknox pushed a commit that referenced this pull request Aug 7, 2015
fix install location of CMake configuration files

Tested on windows by building our cmake clsparse samples; works
Looked at other projects hosted on GitHub; most use the suggested path 👍
@kknox kknox merged commit 0f8c66d into clMathLibraries:develop Aug 7, 2015
kknox pushed a commit that referenced this pull request Aug 18, 2015
* Added Travis CI build automation
* OPENCL_ROOT can be passed through env var to help find OpenCL
* ExternalBoost can select compiler through ENV{CC}
* Linux/OSX builds will find boost either in static or dynamic forms
* Cmake config package now installs to ${LIB_INSTALL_DIR}/cmake/clSPARSE - #116
* Library links to ${CMAKE_DL_LIBS} - #117

Merge branch 'develop'

Conflicts:
	CMakeLists.txt
	src/CMakeLists.txt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants