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 dependency on boost::thread #430

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

KrisThielemans
Copy link
Member

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.

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
Copy link
Member Author

@paskino can you test this on a machine with many cores? Travis has only 2

@KrisThielemans
Copy link
Member Author

@dchansen did something very similar now in Gadgetron. He's used std::lock_guard as opposed to std::unique_lock and a different way to initialise reader_thread_, see https://github.com/gadgetron/gadgetron/blob/8ea7f911e87575f666e30bfa5ffe7269023b0480/apps/clients/gadgetron_ismrmrd_client/gadgetron_ismrmrd_client.cpp#L1193.

I guess he knows what he's doing (as opposed to me), so I suggest that we sync this PR with his code.

@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #430 into master will increase coverage by 3.83%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   47.05%   50.89%   +3.83%     
==========================================
  Files           2        2              
  Lines        1883     1629     -254     
==========================================
- Hits          886      829      -57     
+ Misses        997      800     -197
Impacted Files Coverage Δ
src/xSTIR/pSTIR/STIR.py 53.38% <0%> (+3.23%) ⬆️
src/xGadgetron/pGadgetron/Gadgetron.py 48.02% <0%> (+4.6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbbc777...2acb08b. Read the comment docs.

@KrisThielemans
Copy link
Member Author

@paskino could you give this a quick spin? We need to merge this and then fix the OSX filesystem problem to get Travis Green again. (Also, it'll avoid installation problems for users of course)

@KrisThielemans
Copy link
Member Author

@rijobro problems with boost::thread have disappeared on OSX Travis (of course). Still the related std::filesystem error of course

[ 86%] Linking CXX executable sirf_crop_image

Undefined symbols for architecture x86_64:
  "boost::filesystem::detail::create_directory(boost::filesystem::path const&, boost::system::error_code*)", referenced from:
      sirf::NiftiImageData<float>::write(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int) const in libReg.a(NiftiImageData.cpp.o)
      sirf::AffineTransformation<float>::write(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) const in libReg.a(AffineTransformation.cpp.o)
  "boost::filesystem::detail::status(boost::filesystem::path const&, boost::system::error_code*)", referenced from:
      sirf::NiftiImageData<float>::write(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int) const in libReg.a(NiftiImageData.cpp.o)
      sirf::AffineTransformation<float>::write(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) const in libReg.a(AffineTransformation.cpp.o)
  "boost::filesystem::path::parent_path() const", referenced from:
      sirf::NiftiImageData<float>::write(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, int) const in libReg.a(NiftiImageData.cpp.o)
      sirf::AffineTransformation<float>::write(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) const in libReg.a(AffineTransformation.cpp.o)
ld: symbol(s) not found for architecture x86_64

As this is part of our continuing OSX problems, I strongly recommend we test this on a multi-core Linux/CentOS machine (@paskino, @rijobro ?) and merge.

@KrisThielemans
Copy link
Member Author

sadly, Travis gives segfaults in the MR_PYTHON_TESTS on Linux.

Not much useful to debug it in the log file

"MR_TESTS_PYTHON" start time: Sep 29 10:55 UTC
Output:
----------------------------------------------------------
<end of output>
Test time =  10.52 sec
----------------------------------------------------------
Test Failed.
2:     Start 3: MR_TESTS_PYTHON
2: 3/5 Test #3: MR_TESTS_PYTHON ..................***Exception: SegFault 10.52 sec
travis_time:end:03417ebb:start=1569754684195916395,finish=1569754684210980875,duration=15064480,event=script

sigh

@KrisThielemans
Copy link
Member Author

I'm a bit lost on this. It's the previous commit that made the linux versions crash (or something on travis itself of course, but unlikely as the previous one was 1 hour earlier and ok).

However, the only thing that that commit did was to remove linking boost files for cGadgetron, which it doesn't use anyway. Anyone any ideas?

@rijobro
Copy link
Contributor

rijobro commented Sep 29, 2019

I've restarted the Travis jobs just in case it was a temporary bug on their side. If that doesn't work, we can have a re-think.

@rijobro
Copy link
Contributor

rijobro commented Nov 29, 2019

This looks good to me if travis is happy

@rijobro
Copy link
Contributor

rijobro commented Dec 3, 2019

seg fault in MR ctests. I suppose that implies data race?

@KrisThielemans KrisThielemans added this to the v3.0 milestone Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants