[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd3da9067d143: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR (authored by mstorsjo). Repository: rG LLVM Github

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

beanz accepted this revision. beanz added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131052/new/ https://reviews.llvm.org/D131052 ___

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

mstorsjo added a comment. In D131052#4052563 , @mstorsjo wrote: > Switched the macro to a function and changed the variables to cache > variables, to allow setting them in the grandparent scope without being a > macro. @beanz - Does this seems ok to

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

mstorsjo updated this revision to Diff 489098. mstorsjo added a comment. Switched the macro to a function and changed the variables to cache variables, to allow setting them in the grandparent scope without being a macro. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

mstorsjo added a comment. In D131052#4051948 , @beanz wrote: > The convention that `find_program` uses is to cache the variables, which > causes them to be defined at global scope. Right, I guess that could work too. It would be less of a pure

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

beanz added a comment. The convention that `find_program` uses is to cache the variables, which causes them to be defined at global scope. That also avoids needing to recompute filesystem lookups in incremental builds, which is desirable. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

mstorsjo added inline comments. Comment at: llvm/cmake/modules/AddLLVM.cmake:2401 + +macro(setup_host_tool tool_name setting_name exe_var_name target_var_name) + cmake_parse_arguments(ARG "" "SCOPE" "" ${ARGN}) beanz wrote: > Please make this a `function`

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

beanz added inline comments. Comment at: llvm/cmake/modules/AddLLVM.cmake:2401 + +macro(setup_host_tool tool_name setting_name exe_var_name target_var_name) + cmake_parse_arguments(ARG "" "SCOPE" "" ${ARGN}) Please make this a `function` instead of a `macro`.

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

mstorsjo updated this revision to Diff 488982. mstorsjo added a comment. Herald added subscribers: Moerafaat, zero9178, bzcheeseman, sdasgup3, wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, stephenneuendorffer, liufengdb, aartbik,

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

smeenai added a comment. Yeah, this makes sense to me if we factor out the commonalities. I agree that it's nicer to have a single option vs. having to specify every single native tool individually. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

sammccall added a comment. TL;DR: Thanks for explanation, LG if we move the common logic into a macro. In D131052#3734986 , @mstorsjo wrote: > In D131052#3731894 , @sammccall > wrote: > >> FWIW I have no idea:

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

mstorsjo added a comment. In D131052#3731894 , @sammccall wrote: > FWIW I have no idea: in principle this makes sense, but I don't use such a > configuration and don't have a clear idea of what people who do use it want. Thanks for having a look and

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

sammccall added a comment. FWIW I have no idea: in principle this makes sense, but I don't use such a configuration and don't have a clear idea of what people who do use it want. It also adds significant CMake complexity: e.g. clang-pseudo-gen now has 30 lines just to support the non-default

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

mstorsjo added a comment. Ping - is there any interest in this - a single flag for pointing towards existing prebuilt native executables instead of having to name every one (llvm-tblgen, clang-tblgen, lldb-tblgen, clang-pseudo-gen, clang-tidy-confusable-chars-gen) individually? Repository:

[PATCH] D131052: [CMake] Allow setting the location of host tools with LLVM_NATIVE_TOOL_DIR

mstorsjo created this revision. mstorsjo added reviewers: sammccall, tstellar, smeenai, beanz. Herald added subscribers: carlosgalvezp, mgorny. Herald added a project: All. mstorsjo requested review of this revision. Herald added projects: LLVM, clang-tools-extra. Herald added a subscriber: