Is @tra able to land this? I'm not

Accepting this, with the comment that I think it's better to follow up with a patch that reverts autodetecting hip from clang based on the spack directory structure.

Hi @yaxunl, generally looks good to me. I've just tried this on top of https://github.com/spack/spack/pull/23859 and I'm getting:

Hi Yaxunl,

I've created a pull request to spack here: https://github.com/spack/spack/pull/23859, hopefully that's enough to revert this patch.

Hi @tra and @yaxunl, I'm commenting as a reviewer of the spack pull request for the rocm 4.2.0 ecosystem. First of all: thanks for caring about spack installations, that's highly appreciated.

I don't have commit access, would be great if you could do that for me!

Should this be merged?

I dropped the awk bits, since there's no portable way to do it there, and replaced all perl -w with perl and use warnings.

@JDevlieghere, would you care to review? This is another instance of moving away from system binaries to whatever the user / package manager wants the build to use.

Also note that the changes are not likely to be backported to 11 even: https://reviews.llvm.org/rG31e5f7120bdd2f76337686d9d169b1c00e6ee69c#942622.

D86134 should not be added, it was reverted afterwards. What eventually landed was D85820, which adds a better fix for what D86134 was supposed to solve.

Another take on this isuse, incorporating @phosek's comment on making find_library not find shared libs when compiling with LLVM_BUILD_STATIC=ON, and making sure that get_system_libname does not add empty items to a regex capture group, as CMake's regex engine does not support this.

@phosek it would be great to get that patch in, but the truth is it is difficult testing across multiple platforms for me; I can only test on Linux and macOS, not Windows (GNU). Since that patch would touch predefined, platform-dependent cmake variables, I would want to test it properly before submitting a patch, which might take a bit more time. So for me it would be best to temporarily accept this reverting patch, and submit a properly tested patch afterwards.

@gkistanova It's true that this change has lead to more issues I could ever imagine, but I think the link you provided is the last remaining problem.

@mati865 can you test this on your platform? You only have to run cmake ../llvm and check whether tools/llvm-config/BuildVariables.inc: has a proper #define LLVM_SYSTEM_LIBS "-lrt -ldl -lpthread -lm -lz -ltinfo"

@phosek do you know what is required to backport this as well as the ncurses detection to the LLVM 11 release branch?

@aeubanks and @phosek: see https://reviews.llvm.org/D86245 for a last fix that checks whether CMAKE_FIND_LIBRARY_PREFIXES and CMAKE_FIND_LIBRARY_SUFFIXES are non-empty before applying the regex to fix this window issue

@gribozavr2: can you try https://reviews.llvm.org/D86134? It should fix your issue, as well as the same issue w.r.t. the ZLIB changes of https://reviews.llvm.org/D79219 on macOS.

Move the output variable to the end and use lowercase for arguments.

@srj could you please try / review https://reviews.llvm.org/D86173 for a fix for your problem?

In D85820#2221999, @JDevlieghere wrote:

Let me know if you need me to land this for you.

DRY in find_library

use REQUIRED when FORCE_ON

Use LLVM_ENABLE_TERMINFO and make it respect FORCE_ON.

I also considered set(CURSES_NEED_NCURSES true) and then find_package(Curses), but indeed CURSES_LIBRARIES is the only thing to work with, which lists too many libraries. I think find_library is cleanest. Thanks for the LLVM_ENABLE_TERMINFO, I can add that change.

Simplified detection a bit

Great, one benefit of this is that zlib can now be detected in non-system libs. Maybe we should handle ncurses / TERMINFO in a similar manner? It currently has similar logic as finding zlib had