[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

delcypher added a comment. In D137024#3931488 , @mgorny wrote: > Well, I'm certainly not opposed to making all the paths configurable. > However, I'm not sure if having CMake file accessible one way or another > wouldn't eventually be a necessity. For

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mstorsjo added a comment. In D137024#3931488 , @mgorny wrote: > Well, I'm certainly not opposed to making all the paths configurable. > However, I'm not sure if having CMake file accessible one way or another > wouldn't eventually be a necessity. For

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

tstellar added a comment. In D137024#3931499 , @thetruestblue wrote: > In D137024#3931497 , @tstellar > wrote: > >> @thetruestblue What paths besides LLVM_TOOLS_BINARY_DIR do you need? > > TBD. > So far this

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

thetruestblue added a comment. In D137024#3931497 , @tstellar wrote: > @thetruestblue What paths besides LLVM_TOOLS_BINARY_DIR do you need? TBD. So far this seems like the only necessary path. But I am working to confirm this. Repository: rG LLVM

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

tstellar added a comment. @thetruestblue What paths besides LLVM_TOOLS_BINARY_DIR do you need? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137024/new/ https://reviews.llvm.org/D137024 ___ cfe-commits

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny added a comment. Well, I'm certainly not opposed to making all the paths configurable. However, I'm not sure if having CMake file accessible one way or another wouldn't eventually be a necessity. For one thing, I would like to move more common code from standalone build codepaths of

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

delcypher added a comment. In D137024#3931167 , @mgorny wrote: > In D137024#3931126 , @thetruestblue > wrote: > >> This patch removes the logic that sets the binary tools dir using >> llvm-config. We don't have

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

thetruestblue added a comment. > While I sympathize with you, I don't think this is valid reason to maintain > full compatibility with `llvm-config`. A standalone build with LLVM_TOOLS_BINARY_DIR set directly could be a valid code path. We already have code to mock LLVMConfig.cmake. I

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny added a comment. In D137024#3931126 , @thetruestblue wrote: > This patch removes the logic that sets the binary tools dir using > llvm-config. We don't have llvmConfig.cmake in our toolchain or our build > tree and relied on llvm-config to >

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

thetruestblue added a comment. This breaks our use case. We run sanitizer tests built in a standalone build using an artifact toolchain that contains binaries only. Compiler-rt tests rely on the llvm tools binary being in the PATH. This patch removes the logic that sets the binary tools dir

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mstorsjo added a comment. In D137024#3905181 , @mgorny wrote: >> These workarounds are fine for me, but I wonder if it would be useful with a >> more direct cmake option to tell it not look for these files at all. > > CMake has something like that

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny added a comment. > These workarounds are fine for me, but I wonder if it would be useful with a > more direct cmake option to tell it not look for these files at all. CMake has something like that built-in. I think it's `-DCMAKE_DISABLE_FIND_PACKAGE_LLVM=ON`. Repository: rG LLVM

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mstorsjo added a comment. In D137024#3901342 , @mgorny wrote: > Thanks! Let's hope it works this time. FYI, this broke builds of mine (https://github.com/mstorsjo/llvm-mingw/actions/runs/3382275510/jobs/5617040099), but I just need to change/add a

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny added a comment. Thanks! Let's hope it works this time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137024/new/ https://reviews.llvm.org/D137024 ___ cfe-commits mailing list

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

This revision was automatically updated to reflect the committed changes. Closed by commit rG23c665371a98: [compiler-rt] Switch from llvm-config to find_package(LLVM) (authored by mgorny). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137024/new/

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

phosek accepted this revision. phosek added a comment. Still LGTM, thanks for the explanation. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137024/new/ https://reviews.llvm.org/D137024 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny updated this revision to Diff 472376. mgorny added a comment. Add an explanatory comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137024/new/ https://reviews.llvm.org/D137024 Files: compiler-rt/cmake/Modules/CompilerRTUtils.cmake

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny added inline comments. Comment at: compiler-rt/cmake/Modules/CompilerRTUtils.cmake:274-275 + "LLVM_CONFIG_PATH is deprecated, please use LLVM_CMAKE_DIR instead") +get_filename_component(LLVM_CMAKE_DIR "${LLVM_CONFIG_PATH}" DIRECTORY) +

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

phosek added inline comments. Comment at: compiler-rt/cmake/Modules/CompilerRTUtils.cmake:274-275 + "LLVM_CONFIG_PATH is deprecated, please use LLVM_CMAKE_DIR instead") +get_filename_component(LLVM_CMAKE_DIR "${LLVM_CONFIG_PATH}" DIRECTORY) +

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny added a comment. @phosek, could you take another look? The only changes are on top of `load_llvm_config`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137024/new/ https://reviews.llvm.org/D137024 ___ cfe-commits mailing list

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny updated this revision to Diff 472285. mgorny added a comment. Add a backwards compatibility path that uses `LLVM_CONFIG_PATH` to locate LLVM CMake files. I think it's sufficient to use path relative to the specified location, and we don't want to actually call it. Restore the explicit

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny added a comment. Ok, I think I know what the problem is — these buildbots rely on being able to override `LLVM_CONFIG_PATH`. I'm going to see if I can provide backwards compatibility with it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny added a comment. I'd really use some help with the buildbot failures because I can't even figure out how to reproduce them: https://lab.llvm.org/buildbot#builders/169/builds/13146 https://lab.llvm.org/buildbot#builders/70/builds/29330

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny reopened this revision. mgorny added a comment. This revision is now accepted and ready to land. Gotta figure the buildbot regression out. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137024/new/ https://reviews.llvm.org/D137024

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

This revision was automatically updated to reflect the committed changes. Closed by commit rG37acf9bdd468: [compiler-rt] Switch from llvm-config to find_package(LLVM) (authored by mgorny). Herald added a project: Sanitizers. Herald added a subscriber: Sanitizers. Repository: rG LLVM Github

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

phosek added inline comments. Comment at: compiler-rt/lib/xray/tests/CMakeLists.txt:68-80 +if (COMPILER_RT_HAS_LLVMXRAY OR COMPILER_RT_HAS_LLVMTESTINGSUPPORT) + if (LLVM_LINK_LLVM_DYLIB) +list(APPEND XRAY_UNITTEST_LINK_FLAGS -lLLVM) + endif() +else() +

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny added inline comments. Comment at: compiler-rt/lib/xray/tests/CMakeLists.txt:68-80 +if (COMPILER_RT_HAS_LLVMXRAY OR COMPILER_RT_HAS_LLVMTESTINGSUPPORT) + if (LLVM_LINK_LLVM_DYLIB) +list(APPEND XRAY_UNITTEST_LINK_FLAGS -lLLVM) + endif() +else() +

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

phosek accepted this revision. phosek added a comment. This revision is now accepted and ready to land. LGTM Comment at: compiler-rt/lib/xray/tests/CMakeLists.txt:68-80 +if (COMPILER_RT_HAS_LLVMXRAY OR COMPILER_RT_HAS_LLVMTESTINGSUPPORT) + if (LLVM_LINK_LLVM_DYLIB) +

[PATCH] D137024: [compiler-rt] Switch from llvm-config to find_package(LLVM)

mgorny created this revision. mgorny added reviewers: dberris, justinkb, tstellar, MaskRay, serge-sans-paille, Hahnfeld, mstorsjo, delcypher, phosek. Herald added subscribers: Enna1, StephenFan. Herald added a project: All. mgorny requested review of this revision. Replace the use of the