Conversation
sloede
left a comment
There was a problem hiding this comment.
Thanks for getting started on this. I think it would be great if we could also provide a test for this, e.g., if we add a CMakeLists.txt that shows how to build one of the examples against an existing libtrixi installation.
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
=======================================
Coverage 98.05% 98.05%
=======================================
Files 13 13
Lines 565 565
=======================================
Hits 554 554
Misses 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
[no ci]
|
I haven't looked at it in detail, but we should add a test where the external CMakeLists.txt is actually used to build an example executable. Otherwise it is likely that the script will not work anymore at some point (see the Doxygen example from a few days ago 😬) |
sloede
left a comment
There was a problem hiding this comment.
This looks very, very good imho, thanks a lost! Just one thought:
Right now, there is a lot of copy pasting in the CI such that a user does not just see what needs to be done from looking at the example CMakeLists.txt alone. Therefore, I suggest to
- get rid of the copy paste stuff and
- add a
build.shfile that encapsulates the remaining commands.
That way, everything is tested in CI and nothing is "hidden" in some CI yml file.
I tried to implement this as suggestions, but I think there might be some things I missed.
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
sloede
left a comment
There was a problem hiding this comment.
LGTM, with just one more (small) comment left
…btrixi into libtrixi-cmake-module
Resolves #121