[PATCH] D150752: [bolt] Use correct output paths and passthrough necessary options

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added inline comments.


================
Comment at: bolt/CMakeLists.txt:88
+  if(CMAKE_SYSROOT)
+    list(APPEND extra_args -DCMAKE_SYSROOT=${CMAKE_SYSROOT})
+  endif()
----------------
The [[ https://cmake.org/cmake/help/latest/variable/CMAKE_TOOLCHAIN_FILE.html | documentation for CMAKE_SYSROOT  ]] says "This variable may only be set in a toolchain file specified by the CMAKE_TOOLCHAIN_FILE variable."

Maybe pass CMAKE_TOOLCHAIN_FILE through instead?


================
Comment at: bolt/CMakeLists.txt:88
+  if(CMAKE_SYSROOT)
+    list(APPEND extra_args -DCMAKE_SYSROOT=${CMAKE_SYSROOT})
+  endif()
----------------
michaelplatings wrote:
> The [[ https://cmake.org/cmake/help/latest/variable/CMAKE_TOOLCHAIN_FILE.html | documentation for CMAKE_SYSROOT  ]] says "This variable may only be set in a toolchain file specified by the CMAKE_TOOLCHAIN_FILE variable."
> 
> Maybe pass CMAKE_TOOLCHAIN_FILE through instead?
`list(APPEND variable_that_may_or_may_not_exist ...` is quite fragile. Should someone have `-Dextra_args=X` on their cmake command line then that will be included here, which I don't think is the intent. I suggest adding `set(extra_args "")` above the `if`. This also improves readability because otherwise you have to scan the whole file to figure out that `extra_args` wasn't previously defined.


================
Comment at: bolt/CMakeLists.txt:98
                -DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}
-               -DCMAKE_INSTALL_PREFIX=${LLVM_BINARY_DIR}
+               -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
                -DLLVM_LIBDIR_SUFFIX=${LLVM_LIBDIR_SUFFIX}
----------------
Given that the install step now does nothing, and the `install` statement below passes `-DCMAKE_INSTALL_PREFIX` explicitly, could this line be deleted? Alternatively perhaps `-DCMAKE_INSTALL_PREFIX` could be removed from the `install` statement.


================
Comment at: bolt/CMakeLists.txt:103
+    INSTALL_COMMAND ""
+    STEP_TARGETS configure build
     BUILD_ALWAYS True
----------------
These targets don't appear to be used. I suggest either deleting this line, or if they're needed/useful somehow then adding a comment either here or in the commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150752