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

build issue: ld.lld #2986

Closed
haampie opened this issue Sep 6, 2024 · 2 comments
Closed

build issue: ld.lld #2986

haampie opened this issue Sep 6, 2024 · 2 comments

Comments

@haampie
Copy link

haampie commented Sep 6, 2024

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

find_program(LLVM_LINKER NAMES "ld.lld")

Setting the linker flag here:

if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.29)
set(CMAKE_LINKER_TYPE "LLD" CACHE STRING "Linker type")
else()
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fuse-ld=lld")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fuse-ld=lld")
set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} -fuse-ld=lld")
endif()

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:

  • OS: Ubuntu 22.04
  • MRtrix3 version: dev branch
@haampie haampie added the bug label Sep 6, 2024
@daljit46
Copy link
Member

daljit46 commented Sep 9, 2024

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 daljit46 added the build label Sep 9, 2024
daljit46 added a commit that referenced this issue Sep 10, 2024
We no longer use LLD by default if it is available for reasons mentioned in #2986.
The use of LLD can now be enabled by using -DMRTRIX_USE_LLD=ON.
Additionally, we use CMake's check_cxx_compiler_flag to see if the compiler supports
LLD regardless of the platform (instead of simply relying on the presence of ld.lld).
@daljit46
Copy link
Member

daljit46 commented Sep 12, 2024

Closed by #2989.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants