-
Notifications
You must be signed in to change notification settings - Fork 8
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
CMake components #339
CMake components #339
Conversation
b132ed1
to
261f02d
Compare
@dholladay00 looks like https://github.com/lanl/singularity-eos/blob/main/test/test_eos_modifiers.cpp is broken, since |
9501625
to
847e305
Compare
@mauneyc-LANL the CI fix was actually quite simple. All I needed was avoid the inclusion of the closure header. |
Looked simple, I will review once passes |
@jhp-lanl @Yurlungur tested this in the SAP integration branch. |
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. Minor nitpick below, which I consider nonblocking. Also please modify changelog. And, if the instructions for linking to the library in downstream codes has changed, please update the docs.
|
||
target_sources(singularity-eos PRIVATE ${eos_srcs} ${eos_headers}) | ||
if(SINGULARITY_BUILD_CLOSURE) |
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.
It might be best to have variable like BUILD_LIBRARY
that (for now) automatically is set by BUILD_CLOSURE
so that we can extend in the future if needed.
@rbberger a conflict popped up, I think with a change @jonahm-LANL made last week. |
05c5120
to
a8636f4
Compare
PR Summary
Rewrites the CMake targets and installation to split up
Interface
andLibrary
components.Closes #323
PR Checklist
make format
command after configuring withcmake
.If preparing for a new release, in addition please check the following: