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

build issue: ld.lld #2986

Closed haampie closed 2 months ago

haampie commented 2 months ago

Describe the bug

Sometimes ld.lld can be located as an executable, but not used by the compiler: -fuse-ld=lld fails.

Locating happens here

https://github.com/MRtrix3/mrtrix3/blob/025eb0fe509be4b4681920c3300848b55d04bfa2/cmake/LinkerSetup.cmake#L11

Setting the linker flag here:

https://github.com/MRtrix3/mrtrix3/blob/025eb0fe509be4b4681920c3300848b55d04bfa2/cmake/LinkerSetup.cmake#L16-L22

Two requests:

  1. Please make this opt-in. lld may work well, but let it be a user choice instead of overriding toolchain defaults.
  2. Test for -fuse-ld=lld with try-compile instead of searching for ld.lld the executable.

Platform/Environment/Version

Please provide the following information:

daljit46 commented 2 months ago

The intention of using LLD when available was to enable fast (incremental) build times by default (especially for non-developers who want to build MRtrix3). I admit that overriding toolchain defaults is undesirable though and we should probably not mess with that. Maybe it's best to hide this behind a MRTRIX_USE_LLD option. Ideally, even for other options such as MRTRIX_WARNING_AS_ERRORS we should probably rely on CMake presets instead of hardcoding compiler flags inside our CMakeLists.txt files. However, since 3.16 is our minimum CMake target version we cannot use them everywhere.

Test for -fuse-ld=lld with try-compile instead of searching for ld.lld the executable.

Yes, this also makes sense to me.

daljit46 commented 2 months ago

Closed by #2989.