Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 51.57% 51.65% +0.07%
==========================================
Files 18 18
Lines 1648 1663 +15
Branches 746 752 +6
==========================================
+ Hits 850 859 +9
Misses 390 390
- Partials 408 414 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| DIRECTORY ${PROJECT_SOURCE_DIR}/config/cmake/ | ||
| DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME} | ||
| FILES_MATCHING | ||
| PATTERN "Find*.cmake" |
There was a problem hiding this comment.
Why are we limiting the installed files to the Find modules only? The other cmake file is included as macro in most of the Find modules
There was a problem hiding this comment.
Good question. I need to understand why this is working, even without the macro (I tested all this compiling CP2K with DFTD4 support).
There was a problem hiding this comment.
The multicharge-utils.cmake is correctly installed, probably here:
https://github.com/RMeli/dftd4/blob/83f78b546ac5785ffc996d6f2f2aa6933f74ca61/config/CMakeLists.txt#L57-L61
I think what I added might not be needed, I'll test again and eventually remove it.
There was a problem hiding this comment.
Any update @RMeli? We would like to make a new release of multicharge, since we moved the coordination number logic to mctc-lib. Should we wait for any fixes, or can we proceed?
There was a problem hiding this comment.
Thanks for the ping. This work was bumped down my priority list by other items, but I can probably have a look at this in the afternoon if it is not too late. Sorry for the delay.
Otherwise please go ahead without this PR, if it is merged just after a release it should be easy to use it as a patch, if needed.
There was a problem hiding this comment.
No worries, just wanted to check on the current status. We will make the release some time next week then.
There was a problem hiding this comment.
Unfortunately I don't have access to our ARM system this week. I'll try to find an alternative to finish this off tomorrow, but please don't wait on me for a release. Sorry about the delays!
There was a problem hiding this comment.
Ok, then we will make the release now. There will be another release soon anyway, containing a new charge model.
There was a problem hiding this comment.
@RMeli We implemented the new charge model and would make another release. Any chance, you are able to fix this issue in the near future?
There was a problem hiding this comment.
@marvinfriede sorry, I dropped the ball on this. I'll try to resuscitate the work I did so far.
3 participants
Looks like #44 broke some things upstream, because
dftd4@3.6.0does not pin the version ofmultichargebeing fetched.The issue is that
custom-lapack_FOUNDwas not properly set inFindcustom-lapack.cmake, and thereforefind_dependency(custom-lapack)was not working properly when usingmultichargeanddftd4in upstream projects.This PR adds the use of
find_package_handle_standard_argsso that the correct*_FOUNDvariables are properly set. I also namespaced BLAS and LAPACK withmulticharge::so that things are clearer downstream.I tested this PR by building CP2K with DFTD4 support, and I'll follow up with similar changes for NVPL in
dftd4.