Skip to content
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

remove boost dependency as much as possible #241

Closed
KrisThielemans opened this issue Nov 18, 2018 · 11 comments · Fixed by #986
Closed

remove boost dependency as much as possible #241

KrisThielemans opened this issue Nov 18, 2018 · 11 comments · Fixed by #986
Assignees
Milestone

Comments

@KrisThielemans
Copy link
Member

We're having problems with conflicts with boost versions. This is a serious problem for the boost libraries that are compiled (i.e. not header-only), see e.g. #240, although header-only libraries could still throw up surprising things.

Doing this completely might be hard, but we should at least aim to get rid of explicit includes of boost in our .h files. That way, the boost dependency could be made private. But far better would be to get rid of it completely.

It seems to me that there are at present only few places where we use boost. For instance, in our .h files.

find ../../.. -type f -name \*.h -exec grep -nH -e boost/ {} +
../../../src/xGadgetron/cGadgetron/cgadgetron_shared_ptr.h:24:#include "boost/shared_ptr.hpp"
../../../src/xGadgetron/cGadgetron/gadget_lib.h:35:#include <boost/algorithm/string.hpp>
../../../src/xGadgetron/cGadgetron/gadgetron_client.h:33:#include <boost/asio.hpp>
../../../src/xGadgetron/cGadgetron/gadgetron_client.h:34:#include <boost/shared_ptr.hpp>
../../../src/xGadgetron/cGadgetron/gadgetron_client.h:35:#include <boost/thread/mutex.hpp>
../../../src/xGadgetron/cGadgetron/gadgetron_client.h:36:#include <boost/thread/thread.hpp>
../../../src/xGadgetron/cGadgetron/gadgetron_data_containers.h:33:#include <boost/algorithm/string.hpp>
../../../src/xGadgetron/cGadgetron/gadgetron_x.h:38:#include <boost/thread/mutex.hpp>
../../../src/xGadgetron/cGadgetron/gadgetron_x.h:39:#include <boost/shared_ptr.hpp>
../../../src/xGadgetron/cGadgetron/xgadgetron_utilities.h:36:#include <boost/thread/mutex.hpp>
../../../src/xSTIR/cSTIR/stir_types.h:24:#include <boost/algorithm/string.hpp>
../../../src/Registration/cReg/SIRFReg.h:55:#include <boost/filesystem.hpp>
../../../src/Registration/cReg/SIRFRegMisc.h:33:#include <boost/filesystem.hpp>
../../../src/Registration/cReg/SIRFRegNiftyResample.h:40:#include <boost/filesystem.hpp>
../../../src/Registration/cReg/SIRFRegParser.h:37:#include <boost/filesystem.hpp>

Quite a few of these seem to be left-overs from the past. Most can and should be replaced by their std versions, in particular uses of boost/shared_ptr.hpp. Hopefully mutex as well.

SIRFReg depends on boost/filesystem. Sadly, std::filesystem exists from C++-14 only. I'm a bit reluctant to require C++-14, although Gadgetron does it now anyway. I suggest to do a CMake check on existence of std::filesystem, and only use boost::filesystem if we need to.

If we cannot remove the dependency completely, a trick to get rid of it in the .h files is to use pre-declaration in the .h, and include it in the .cpp. This works for pointers and references. So, as opposed to have a boost::filesystem member, we could have a std::shared_ptr<boost::filesystem>. That's a bit ugly though.

@KrisThielemans
Copy link
Member Author

Actually, std::filesystem is C++-17. It currently still causes extra trouble with gcc due to extra linking requirements.

@rijobro how useful was boost:filesystem for you? @NikEfth write a FilePath class for STIR to provide some similar functionality. If we're desperate, we could use that in SIRF somehow (splitting it off from STIR or whatever)

@ashgillman
Copy link
Member

I think my changes to the geometry pulled in boost for linear algebra. I could use something else.

@KrisThielemans
Copy link
Member Author

KrisThielemans commented Nov 26, 2018 via email

@rijobro
Copy link
Contributor

rijobro commented Nov 29, 2018

@rijobro how useful was boost:filesystem for you?

I can get rid of it pretty easily.

How do I know if each aspect of boost that I'm using is just from the header, or requires the compiled Boost libraries?

If I remove all CMake lines akin to target_link_libraries(SIRFReg Boost::system Boost::filesystem) from my code, does that mean that it can only access the uncompiled headers? The SIRFReg code compiles fine without those CMake lines.

@KrisThielemans
Copy link
Member Author

not sure if there's an easy way to find out if it's header-only, except by checking...

I'm somewhat amazed that you don't need to link to boost::filesystem, but I'd be a bit warry of using boost/filesystem headers and not linking to it. it's going to break at some point/with some version.

Obviously, boost is useful. but if dependence is really small, I'd try to cut it out. Might be worth having a look at stir::FilePath.

or we can get Mathworks to do the proper thing and link privately/statically...

@rijobro
Copy link
Contributor

rijobro commented Nov 30, 2018

Ok. Well I've removed linking to the boost libs (58f4f55). If this becomes a problem as the boost version changes, I'll cross that bridge when I get to it.

@KrisThielemans
Copy link
Member Author

Another motivation is at SyneRBI/SIRF-SuperBuild#161

@KrisThielemans
Copy link
Member Author

@evgueni-ovtchinnikov could you check if we actually need #include <boost/algorithm/string.hpp> (see above). I don't think it's used.

@KrisThielemans
Copy link
Member Author

At present, our CMake files force linking with Boost::system Boost::filesystem Boost::thread Boost::date_time Boost::chrono, see e.g. here. My impression is that we only need thread, but this depends on system and chrono (and header-only datetime). (More info on dependencies here

Of course, even if we don't really need to link with datetime, CMake has its own ideas on dependencies and will require it anyway.

Seems that we can get rid of filesystem though.

@evgueni-ovtchinnikov
Copy link
Contributor

<boost/algorithm/string.hpp> defines iequals, which we use

@KrisThielemans
Copy link
Member Author

KrisThielemans commented Aug 30, 2019

let's

  • create our own iequals e.g. like here such that we can remove string.hpp/
  • replace boost::mutex with std::mutex
  • remove boost/shared_ptr.hpp

but leave the rest as-is for now (boost::asio is header-only, boost::shared_ptr as well and is only used for old STIR, and apparently filesystem can be used header-only as well).

KrisThielemans added a commit that referenced this issue Sep 13, 2019
instances of boost::thread and boost::mutex are all replaced by std versions.
This means we don't need to link with boost::thread anymore.

This should reduce boost version conflicts, such as those in #429 and #241.
@KrisThielemans KrisThielemans modified the milestones: v2.1, v2.2 Oct 14, 2019
@KrisThielemans KrisThielemans modified the milestones: v3.0, v3.1 Feb 11, 2021
evgueni-ovtchinnikov added a commit that referenced this issue Jul 26, 2021
* started replacing boost::iequals with sirf::iequals

* finished replacing boost::iequals with sirf::iequals

* removed obsolete sources from cGadgetron

* removed obsolete headers from cGadgetron

* replaced boost::mutex with std::mutex

* removed unused #include <boost/shared_ptr.hpp> from JacobiCG.h

* removed includes of obsolete ismrmrd_fftw.h

* small amendment in JacobiCG.h

* added missing #include<cstring> (for std::memcpy)

* added missing #include<memory> (for std::shared_ptr)

* sorted out de-boost build on linux, fixes #241

* [ci skip] small amendments

* [ci skip] doxygen comments in iequals.h corrected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants