-
Notifications
You must be signed in to change notification settings - Fork 29
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
Synergistic IO dependency fix #1194
base: master
Are you sure you want to change the base?
Conversation
The synergistic library/executables/tests have now been adapted for DISABLE_Gadgetron etc. The work-around in common/utilities.* has therefore been removed. a job with DISABLE_Gadgetron has been added to GHA Fixes SyneRBI#622
4f97a0b
to
dc8bbb6
Compare
now rebased on top of master |
When using STIR release, we get when building pystir https://github.com/SyneRBI/SIRF/actions/runs/5073309434/jobs/9112163305?pr=1194#step:8:14957
while with STIR
which
|
- removed some superflous include statements - removed some predeclarations and explicitly include iutilities.h
- add more conditionals to see if certain things can be built - split csirf library into 2: csirf is now the C++ library, while cinterface-sirf contains the C wrappers. This is probably more logical, but also avoids a circular dependency on ImageDataWrapper (which sits in csyn, but depends on creg etc). ImageDataWrapper is currently not using in csirf, but only in cinterface-sirf (i.e. Python/MATLAB) - remove the option DISABLE_Synergistic when building Python/MATLAB stuff as disabling it would lead to linking errors (as they need ImageDataWrapper)
The above problem is fixed now. However, I still have a Python problem such as
@evgueni-ovtchinnikov |
Apparently it is - when I do "Go To Definition" on the prototype in |
cinterface-Reg depends on STIR for TextWriter Also fix some MATLAB things related to TextWriter (untested)
@evgueni-ovtchinnikov this is pretty much finished now. I've tried to give descriptive log messages, so maybe that helps in the review. Most important change is that
There are a few things that we could still do here:
These things would be nice, but we could also do them later. Would be cleanest to do the first bullet though (and it's very simple, so I can still do that). |
Main comment: I very much welcome suggested changes (thought about them myself, but never attempted for lack of understanding of CMake intricacies), but they look too dramatic to me for 3.5 - should this not go to 4.0? I need more time to study your PR, for now just one minor comment that cannot wait: frankly, I find |
Fixes #622 (I hope)
Currently sits on top of #1166, sorry. but I'll rebase once that's merged to
master
. i first wanted to see if this would work. It's really only the last commit at the moment)