Open KrisThielemans opened 4 years ago
For your last point, we've already made the change such that we don't use target_include_directories
and only use the target ISMRMRD::ISMRMRD
:
The problem is that the SB installs everything into the same location. We could modify the SB installation method for better compartmentalisation? The env_ccppetmr.sh
would have to be slightly more complicated, but everything should be tidier. E.g,:
Install
STIR
include
lib
share
Gadgetron
include
lib
share
etc
Lastly, could this be the problematic bit of code? Put in by me 17 months ago apparently. Not sure why it uses the install interface as we don't currently install our headers.
For your last point, we've already made the change such that we don't use target_include_directories and only use the target ISMRMRD::ISMRMRD:
yep, but we still have the target_include_directories in L47 above. Not needed? (if we can remove it, it'd have to be moved inside the if(WIN32)
for now).
We could modify the SB installation method for better compartmentalisation?
we could do, and probably should be versioning as well, although I feel that that kind of thing should be handled by the packages. In any case I think this is a SIRF problem. SIRF itself has no control where things are going to be installed.
Lastly, could this be the problematic bit of code?
No, I don't think so. The INSTALL_INTERFACE
stuff is there for exported CMake config (which we don't right now, but will soon). It is ignored during build time.
Another reason why I don't think this is the case is that I was originally building SIRF with BUILD_Gadgetron=OFF
, therefore not including gadgetron
. I didn't have this problem then. (I had another one down the line, but not relevant to this issue).
cstir
depends on cgadgetron
because of https://github.com/SyneRBI/SIRF/blob/dcf7a4d37153f3d6334386255767e7d777f0aee6/src/common/CMakeLists.txt#L42-L44
I think this is #622, so possibly the current issue will disapper once we fix that.
Sorry, for your last code snippet, why does this mean that cstir
depends on cgadgetron
? I would have thought it meant that Syn
depends on cstir
and cgadgetron
, but not that they depend on each other.
Order of include directories is currently problematic. Checking with
make VERBOSE=ON
shows thatcstir
is compiled with an order likevarious-sirf-dirs .../install/include .../hdf5/... sirf-reg-dirs STIR-install-dir/include ...
This means that if there's an old STIR version in
.../install/include
, it gets picked up before the actualSTIR-install-dir/include
that I pointed to.I guess (but am not sure) this is because
cgadgetron
usesISMRMRD_INCLUDE_DIR
, https://github.com/SyneRBI/SIRF/blob/dcf7a4d37153f3d6334386255767e7d777f0aee6/src/xGadgetron/cGadgetron/CMakeLists.txt#L47Possible ways to resolve this:
cstir
depend oncgadgetron
?ismrmrd
headers directly in cgadgetron include files, but this seems a lot of workcstir
uses STIR include paths firsttarget_include_directories
but just depend onISMRMRD:ISMRMRD
although that will likely give the same dependencies.