This addresses https://github.com/llvm/llvm-project/issues/62748.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
bolt/CMakeLists.txt | ||
---|---|---|
88 | The 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? | |
88 | 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. | |
98 | 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. | |
103 | 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. |
bolt/CMakeLists.txt | ||
---|---|---|
88 | That documentation is inaccurate. CMAKE_SYSROOT can be set outside of the toolchain file, and thus needs to be passed through as is frequently done in LLVM, see for example https://github.com/llvm/llvm-project/blob/7ca1bcbc6043b7ab4635f04746d1b9480960e384/llvm/cmake/modules/LLVMExternalProjectUtils.cmake#L261 or https://github.com/llvm/llvm-project/blob/7ca1bcbc6043b7ab4635f04746d1b9480960e384/compiler-rt/cmake/Modules/AddCompilerRT.cmake#L650. |
LGTM
bolt/CMakeLists.txt | ||
---|---|---|
88 | Deviating from what's documented as permitted could cause a problem with future CMake versions. But evidently this pattern is already established in LLVM, and if it becomes a problem in future then it can be fixed at that time. |
bolt/CMakeLists.txt | ||
---|---|---|
88 | I agree, ideally we would use the same helper function everywhere where we need to invoke CMake as a subbuild so the passthrough is handled uniformly, but that can be done as a future improvement. |
The 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?