sesame2spiner refactor#641
Conversation
…es. Modify cmake so that we build a library.
|
Evidently there's some cmake stuff left to figure it when things like spiner are not requested or when not in submodule mode maybe |
Co-authored-by: Adam M. Dempsey <adamdemps@gmail.com>
There was a problem hiding this comment.
This file is not included anywhere I think?
There was a problem hiding this comment.
No it's not. We could add it to the singularity-eos whole test harness after this refactor perhaps? I think that can be a separate MR.
There was a problem hiding this comment.
Will do in separate MR
Okay, all tests are passing now so I think this is ready for review |
|
Is there an expected lifecycle for the forwarding headers? If so, how should we communicate to users that the forwarding headers are in place? Our changelog mostly just points users to MR's, but maybe in this case something more concrete about changes they should make? Issue a warning if the forwarding header is used? |
I see a few options:
I don't really think people outside of us are going to act on the warnings, so I'm leaning towards option 2. |
Yurlungur
left a comment
There was a problem hiding this comment.
I'm sorry for the long delay on this review. Some nitpicks below but they are all non-blocking.
| using table_utils::SpinerTableGridParams; | ||
| using Bounds = table_utils::Bounds<table_utils::NGRIDS>; | ||
| using Grid_t = Spiner::PiecewiseGrid1D<Real, table_utils::NGRIDS>; |
There was a problem hiding this comment.
I guess this one needs the using statements.
There was a problem hiding this comment.
Could also consider moving this into ports-of-call... actually there may be duplicated functionality already...
There was a problem hiding this comment.
Yeah, we can check
I am in favor of just making the next release breaking, but if we're worried about it I think a deprecation warning and then actually removing them one release later. |
I've removed them. They will show up as a rename in the diff now |
|
👍 merge when ready |
PR Summary
This does some refactoring on the code base to build
sesame2spineras a library thatsingularity-eoslinks in while maintaining the executablesesame2spiner.sesame2spinerwas linking insingularity-eosfor some things, so I have moved those shared headers into a new library calledsingularity-utils. For backwards compatibility in downstream codes, I have replaced the headers lost insingularity-eos/basewith forwarding headers tosingularity-utils. I have updated the includes insingularity-eosproper with the new ones.If I did everything correctly, nothing should've changed. So my plan is to have this MR merged, then follow up with adding the functions to
sesame2spinerthe library to process tables without reading a file.PR Checklist
make formatcommand after configuring withcmake.plan_historiesfolder, with a filename the same as the MR number.If preparing for a new release, in addition please check the following: