Implement CMake LLVM version overrides; add hard MAX and MIN versions

[Kangie]

Fixes #691

• Move minimum LLVM version to variable; add max version
• Implement -DLLVM_ROOT_DIR and -DLLVM_FIND_VERSION
• If the user has set a particular version and CMake can't find it; die
• Intentionally fail if LLVM version is unsupported
• Made LLVM detection messages STATUS rather than NOTICE

LLVM_MAX_VER set to 13.0.1 based on #574.

[micahsnyder]

Thanks for the PR. Nice catch with info -> STATUS and "FATAL" -> "FATAL_ERROR".

I haven't tested this yet but it reads well. I just have a few changes to request.

[Kangie]

Feedback addressed - I appreciate the prompt response.

Seems fine from my end. I don't have the infra available right now to test against versions of LLVM that need FindLLVM.cmake, but your suggestion to deduplicate that code seems sane.

Due to the use of VERSION_GREATER_EQUAL I've removed the conditional for CMake versioning and set that to 3.7 across the board. It's from 2016; if someone is running a system with an older version of CMake they'll have to update. Arguably this should have been done long before me; that check was already in use.

Someone on your team will need to review your CMakelists files (or ideally migrate to something more sane, like Meson). The minimum version for CMake across the board should be at least 3.7 based on the use of VERSION_GREATER_EQUAL in code before I made changes. Changing this breaks (at the very least) libclamunrar_iface. </3 CMake

I note that LLVM_FIND_VERSION doesn't really do much on its own - it doesn't provide any hints to CMake about which version of LLVM it should find and doesn't override CMake's detection of LLVM versions. Instead this change just just forces an error if what you specify isn't what CMake finds (which appears to always be the highest version without LLVM_ROOT_DIR being set).

I feel like this is desirable behaviour, even if it just encourages users to set LLVM_ROOT_DIR if they're on a multi-impl system; If I specify 13.0.1 and 14.0.x or 12.x.x is found I'd rather that the configure process error instead of compile against a version that's not the one that I explicitly specified.

This shouldn't impact most users though - it's unusual to see multi-impl LLVM on distros outside of Gentoo anyway, and the ebuild for clamav there has been updated to set both -DLLVM_FIND_VERSION and -DLLVM_ROOT_DIR.

[micahsnyder]

Due to the use of VERSION_GREATER_EQUAL I've removed the conditional for CMake versioning and set that to 3.7 across the board. It's from 2016; if someone is running a system with an older version of CMake they'll have to update. Arguably this should have been done long before me; that check was already in use.

Someone on your team will need to review your CMakelists files (or ideally migrate to something more sane, like Meson). The minimum version for CMake across the board should be at least 3.7 based on the use of VERSION_GREATER_EQUAL in code before I made changes. Changing this breaks (at the very least) libclamunrar_iface. </3 CMake

The minimum CMake versions for ClamAV are already 3.16 (windows) and 3.14 (others) https://github.com/Cisco-Talos/clamav/blob/main/CMakeLists.txt#L4-L8 Using a feature from 3.7 shouldn't be a problem.

I note that LLVM_FIND_VERSION doesn't really do much on its own - it doesn't provide any hints to CMake about which version of LLVM it should find and doesn't override CMake's detection of LLVM versions. Instead this change just just forces an error if what you specify isn't what CMake finds (which appears to always be the highest version without LLVM_ROOT_DIR being set).

I feel like this is desirable behaviour, even if it just encourages users to set LLVM_ROOT_DIR if they're on a multi-impl system; If I specify 13.0.1 and 14.0.x or 12.x.x is found I'd rather that the configure process error instead of compile against a version that's not the one that I explicitly specified.

This shouldn't impact most users though - it's unusual to see multi-impl LLVM on distros outside of Gentoo anyway, and the ebuild for clamav there has been updated to set both -DLLVM_FIND_VERSION and -DLLVM_ROOT_DIR.

Our FindLLVM.cmake originates from https://github.com/ldc-developers/ldc/blob/master/cmake/Modules/FindLLVM.cmake. I'm happy to make improvements on our end. Ideally we'd share them back with the ldc project as well. If you'd like to work on making LLVM_FIND_VERSION work better, we'd be happy to review your work.

In case it interests you, there is another/newer mechanism for finding LLVM using LLVMConfig.cmake files installed to under /usr/lib/llvm-10/cmake/LLVMConfig.cmake (for example). See https://github.com/Cisco-Talos/clamav/blob/main/INSTALL.md?plain=1#L711-L737 for more details.

[Kangie]

The minimum CMake versions for ClamAV are already 3.16 (windows) and 3.14 (others)

I looked at that for far too long yesterday. Those were the strings I replaced. Why I parsed 3.16 as < 3.7 I will never know. :)

[micahsnyder]

Is this ready for re-review?

[Kangie]

Should be!

While I haven't touched LLVM_FIND_VERSION or FindLLVM.cmake I'm happy enough that if a particular version of LLVM is specified and it's not what CMake finds that the configure process intentionally fails; ldc seems to agree that setting LLVM_ROOT_DIR is appropriate in that case.

Additionally it's probably only an issue on multi-impl systems where it appears that the highest version is preferred regardless of what's specified.

I've had this patch applied to the Gentoo clamav package for a few weeks with no reported issues. Happy to make any changes/improvements or address additional feedback! (Edit: Though JIT/LLVM is not enabled by default there!)