-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a libtrixi cmake module #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
[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 😬) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.sh
file 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 <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
Co-authored-by: Michael Schlottke-Lakemper <michael@sloede.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with just one more (small) comment left
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work!
…btrixi into libtrixi-cmake-module
Resolves #121