Skip to content

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
@paskino
Copy link
Contributor

paskino commented Feb 4, 2025

It looks to me that this PR can be safely closed as everything has been implemented with other PRs.

@KrisThielemans
Copy link
Member Author

Indeed, #986 did all/a lot.

@KrisThielemans KrisThielemans deleted the REMOVE_BOOST_THREAD branch February 5, 2025 07:30
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.

4 participants