Conversation
|
hi @GPMueller , thanks for making this repository available! I recently stumbled upon it and couldn't build it due to changes in Eigen/CMake. Please find in this MR updates to CMake and Eigen. Review would be much appreciated, thanks! |
GPMueller
left a comment
GPMueller
left a comment
There was a problem hiding this comment.
Thanks for the PR, you made a bunch of nice improvements!
|
|
||
| ### Find includes in corresponding build directories | ||
| set( CMAKE_INCLUDE_CURRENT_DIR ON ) | ||
| ######### Have the binary placed into the source head |
There was a problem hiding this comment.
If you remove this section, the build does not match the docs, which say
To run an example, do
./main
(on msvc, but also if you implement my comments on the readme)
There was a problem hiding this comment.
oh okay, running Ubuntu here, not sure I can look into this
There was a problem hiding this comment.
why did you remove the section placing the binary into the root cmake dir?
CMakeLists.txt
Outdated
| find_package(CUDA REQUIRED) | ||
| if (USE_CUDA) | ||
| enable_language(CUDA) | ||
| add_definitions(-DUSE_CUDA) |
There was a problem hiding this comment.
if you'd like to modernise the cmake here, you could place this further below with the set_property:
target_compile_definitions(test PRIVATE
USE_CUDA
)
(also, I believe this if (USE_CUDA) could be merged with the one further below?)
There was a problem hiding this comment.
added USE_CUDA as private property.
On the other note: The language has to be enabled or Eigen won't compile with cuda support. Not sure how to do this differently
There was a problem hiding this comment.
I'm not suggesting to remove it, but it seems like it should be fine to place
enable_language(CUDA)
add_definitions(-DUSE_CUDA)
into the if-block further down,
if (USE_CUDA)
add_library(test ........
Or do you mean enable_language(CUDA) needs to be called before FetchContent_MakeAvailable(Eigen3)? If that's the case it's fine like it is
| FetchContent_Declare( | ||
| Eigen3 | ||
| GIT_REPOSITORY [email protected]:libeigen/eigen.git | ||
| GIT_TAG master |
There was a problem hiding this comment.
I'm not sure about fetching a branch instead of a commit or tag. It will mean people will get varying results over time.
You say
couldn't build it due to changes in Eigen/CMake
But this would change the Eigen version for people over time, without any relation to the CUDA version -- assuming an old CUDA version I believe this example should still build, even on newer CMake. Newer CUDA versions will require newer Eigen versions, but unfortunately it is also my experience that newer Eigen versions break on older CUDA.
Maybe it would be better to specify with which CUDA versions a given Eigen version should work?
Unfortunately, I don't see a way to future-proof this except maybe using conan or vcpkg, which seems a bit overkill...
There was a problem hiding this comment.
generally agreed that using a tag / hash is better than a plain branch.
Is there a table that maps cuda / eigen versions somehow?
There was a problem hiding this comment.
I'm not aware of one and I think it would be overkill to implement such a mapping here.
For me it would be fine to choose a version and write a short comment with the CUDA version with which this MWE works.
|
hi @GPMueller thanks for reviewing and the nice comments. The comments are all valid, I'll have a look at them soonish |
2 participants
This PR
FetchContent