[PATCH] D141050: [standalone-build] outsource, simplify and unify repetitive CMake code

Tue Jan 10 04:37:11 PST 2023

kwk added a comment.

Thank you @phosek for your review! I really appreciate it. I've addressed all of your comments. Could you give it another review?

================
Comment at: clang/CMakeLists.txt:35
+  include(StandaloneBuildHelpers)
+  get_llvm_utility_binary_path("llvm-tblgen" "LLVM_TABLEGEN_EXE")

----------------
phosek wrote:
> It's somewhat unusual to quote output variable names in our CMake files, I'd prefer to follow existing conventions for consistency.
I obey to the consistency.

================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:12-19
+if (NOT (CLANG_BUILT_STANDALONE
+         OR LLD_BUILT_STANDALONE
+         OR COMPILER_RT_STANDALONE_BUILD
+         OR OPENMP_STANDALONE_BUILD
+         OR MLIR_STANDALONE_BUILD
+         OR LLDB_BUILT_STANDALONE))
+    message(FATAL_ERROR "Make sure you build in standalone mode")
----------------
phosek wrote:
> I'd omit this, I don't think it's necessary to restrict the usage of this module.
I think restricting the usage makes it clear for what it is. And right now this CMake file does stuff just when you include it which not a good thing in terms of isolation. If you don't mind I'd like to keep this restriction and only remove it before this patch lands in a better shape. Okay?

================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:25-30
+if(NOT MSVC_IDE)
+set(LLVM_ENABLE_ASSERTIONS ${ENABLE_ASSERTIONS}
+  CACHE BOOL "Enable assertions")
+# Assertions should follow llvm-config's.
+mark_as_advanced(LLVM_ENABLE_ASSERTIONS)
+endif()
----------------
phosek wrote:
> Do you know why is this needed or this just copy pasted from Clang? I'd consider dropping this altogether (ideally in a separate change).
Honestly, I started to move the CMake code from clang over to this file piece by piece once I found that the exact or similar formatted code existed in other projects like lld. But you caught me red-handed :).

This is the patch that introduced this particular section: [[ https://github.com/llvm/llvm-project/commit/e6d79ec0ebac350355c76d8132e9f1cce62e0cac | e6d79ec0ebac350355c76d8132e9f1cce62e0cac ]]. It is from 2013.

I'm not so long involved in the llvm project that I can give a solid answer to this but hasn't llvm-config been deprecated for building LLVM itself? If that is the case, I'm happy to remove this section from the `clang/CMakeLists.txt` in the first place in a separate patch. 

================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:45-46
+
+# Automatically add the current source and build directories to the include path.
+set(CMAKE_INCLUDE_CURRENT_DIR ON)
+
----------------
phosek wrote:
> This shouldn't be needed since you've already had to insert the module path manually.
I don't see what the one has to do with the other immediately. Maybe I'm a fool. The `CMAKE_INCLUDE_CURRENT_DIR` is for including C/C++ files and not CMake files so it doesn't matter what I have in my module path, no?. Here's a little example project I created to proof to myself that my assumption is really true: https://github.com/kwk/cmake-example-CMAKE_INCLUDE_CURRENT_DIR . Just run `make` once you've clone it. Then comment out [[ https://github.com/kwk/cmake-example-CMAKE_INCLUDE_CURRENT_DIR/blob/main/src/CMakeLists.txt#L10 | this line ]] to see the effect: `include(EnableCurrentIncludeDir)`.

================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:61
+function(get_llvm_utility_binary_path utility out_var)
+    set(_imploc IMPORTED_LOCATION_NOCONFIG)
+    # Based on the build type that the installed LLVM was built with,
----------------
phosek wrote:
> We use 2 spaces for indentation in our CMake files.
I've fixed the indention to use 2 spaces.

================
Comment at: cmake/Modules/StandaloneBuildHelpers.cmake:82-109
+# The set_lit_defaults macro exists because the code existed in multiple
+# locations before.
+macro(set_lit_defaults)
+    set(LIT_ARGS_DEFAULT "-sv")
+    if (MSVC OR XCODE)
+      set(LIT_ARGS_DEFAULT "${LIT_ARGS_DEFAULT} --no-progress-bar")
+    endif()
----------------
phosek wrote:
> Is there any reason to keep these two macros separate? Could we combine them?
Yes, to both of your questions :) . Yes, I find it easier to read the files that include this file and use the functions and macros when they are separated. And yes, we could combine them to `setup_external_lit` or something alike. But for the course of this patch I'd like to keep them separated and do the merge later if you don't mind.

================
Comment at: lld/CMakeLists.txt:57
+    set_lit_defaults()
+    get_llvm_utility_binary_path("FileCheck" "FileCheck_EXE")
+    get_llvm_utility_binary_path("count" "count_EXE")
----------------
phosek wrote:
> Where are these variables being used?
They were not being used, yet. I forgot to make the second parameter of `get_llvm_utility_binary_path` an optional one. Now it is optional and you can omit the second parameter. I've also renamed the function to `require_llvm_utility_binary_path` to convey the importance that the utility exists through the name of the function.

Also, I'd like to refer to the TODO in this patch's description:

> make sure the correct binaries of FileCheck and count and not
get substituted in lit test files.

I'm quite positive that lit will use the correct binaries because we've imported the targets but still I wanna be sure. Having said that, you should not need the path except for `LLVM_TABLEGEN_EXE`. That variable existed before and I worked my function around it to fit this usecase.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141050/new/

https://reviews.llvm.org/D141050