MRtrix3 / mrtrix3

MRtrix3 provides a set of tools to perform various advanced diffusion MRI analyses, including constrained spherical deconvolution (CSD), probabilistic tractography, track-density imaging, and apparent fibre density
http://www.mrtrix.org
Mozilla Public License 2.0
294 stars 181 forks source link

Project doesn't compile against Eigen 3.3.7 #2926

Closed daljit46 closed 2 months ago

daljit46 commented 5 months ago

I came across this when building on Ubuntu 20.04 (which is what we will use for building MRtrix3 release packages). Compilation error:

FAILED: cmd/CMakeFiles/dwi2adc.dir/dwi2adc.cpp.o 
/opt/hostedtoolcache/sccache/0.8.1/x64/sccache /usr/bin/clang++-17 -DMRTRIX_BASE_VERSION=\"3.0.4\" -DMRTRIX_BUILD_TYPE=\"Release\" -DMRTRIX_HAVE_LGAMMA_R -DMRTRIX_PNG_SUPPORT -DMRTRIX_TIFF_SUPPORT -I/home/runner/work/mrtrix3-ci-test/mrtrix3-ci-test/src -I/home/runner/work/mrtrix3-ci-test/mrtrix3-ci-test/core -isystem /usr/include/eigen3 -O3 -DNDEBUG -std=gnu++17 -flto=thin -fPIE -Winvalid-pch -Xclang -include-pch -Xclang /home/runner/work/mrtrix3-ci-test/mrtrix3-ci-test/mrtrix_tarball_tmp/build_tarball/cmd/CMakeFiles/pch_cmd.dir/cmake_pch.hxx.pch -Xclang -include -Xclang /home/runner/work/mrtrix3-ci-test/mrtrix3-ci-test/mrtrix_tarball_tmp/build_tarball/cmd/CMakeFiles/pch_cmd.dir/cmake_pch.hxx -MD -MT cmd/CMakeFiles/dwi2adc.dir/dwi2adc.cpp.o -MF cmd/CMakeFiles/dwi2adc.dir/dwi2adc.cpp.o.d -o cmd/CMakeFiles/dwi2adc.dir/dwi2adc.cpp.o -c /home/runner/work/mrtrix3-ci-test/mrtrix3-ci-test/cmd/dwi2adc.cpp
/home/runner/work/mrtrix3-ci-test/mrtrix3-ci-test/cmd/dwi2adc.cpp:94:50: error: no member named 'all' in namespace 'Eigen'
   94 |       const Eigen::MatrixXd bsub = b(idx, Eigen::all);
      |                                           ~~~~~~~^
/home/runner/work/mrtrix3-ci-test/mrtrix3-ci-test/cmd/dwi2adc.cpp:106:16: error: no matching function for call to object of type 'Eigen::VectorXd' (aka 'Matrix<double, Dynamic, 1>')
  106 |       dwisub = dwi(idx);
      |                ^~~
/usr/include/eigen3/Eigen/src/Core/DenseCoeffsBase.h:423:5: note: candidate function not viable: no known conversion from 'std::vector<size_t>' (aka 'vector<unsigned long>') to 'Index' (aka 'long') for 1st argument
  423 |     operator()(Index index)
      |     ^          ~~~~~~~~~~~
/usr/include/eigen3/Eigen/src/Core/DenseCoeffsBase.h:178:5: note: candidate function not viable: no known conversion from 'std::vector<size_t>' (aka 'vector<unsigned long>') to 'Index' (aka 'long') for 1st argument
  178 |     operator()(Index index) const
      |     ^          ~~~~~~~~~~~
/usr/include/eigen3/Eigen/src/Core/DenseCoeffsBase.h:115:41: note: candidate function not viable: requires 2 arguments, but 1 was provided
  115 |     EIGEN_STRONG_INLINE CoeffReturnType operator()(Index row, Index col) const
      |                                         ^          ~~~~~~~~~~~~~~~~~~~~
/usr/include/eigen3/Eigen/src/Core/DenseCoeffsBase.h:362:5: note: candidate function not viable: requires 2 arguments, but 1 was provided
  362 |     operator()(Index row, Index col)

This will be irrelevant if #2859 or an alternative solution is merged. Not an issue for releases as we can always install eigen manually from Gitlab, but for end users who want to build from source we should try to fix this.

jdtournier commented 5 months ago

Is this on dev or master? If on dev, that's expected. If master, we'd need to look into it...

daljit46 commented 5 months ago

This is on dev, but since we're targeting Ubuntu 20.04 LTS as the minimum baseline for building MRtrix3, I thought it'd still be relevant. Well, unless we aim to no longer depend on Eigen being installed on the user's system (e.g. by implementing #2859).

Lestropie commented 5 months ago

See relevant comment in #2578. It had a larger discussion in video call, the online documentation of the conversation is a bit light. I'd say that either #2859 needs to happen for 3.1.0, or this code needs to be reverted to be Eigen 3.3 compatible.

daljit46 commented 2 months ago

Closing as we merged #2859.