phosek added inline comments.
================ Comment at: bolt/CMakeLists.txt:88 + if(CMAKE_SYSROOT) + list(APPEND extra_args -DCMAKE_SYSROOT=${CMAKE_SYSROOT}) + endif() ---------------- michaelplatings wrote: > phosek wrote: > > michaelplatings wrote: > > > 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. > > 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. > 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. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150752/new/ https://reviews.llvm.org/D150752 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits