[PATCH] D71430: [llvm/runtimes] Add runtimes as a dependency of clang-bootstrap-deps

[Xin-Xin Wang via Phabricator via llvm-commits]

xinxinw1 marked an inline comment as done.
xinxinw1 added inline comments.


================
Comment at: llvm/runtimes/CMakeLists.txt:567
+      # built as part of runtimes and we need it for PGO
+      add_dependencies(clang-bootstrap-deps runtimes)
     endif()
----------------
phosek wrote:
> I don't think we can add this dependency unconditionally. If `LLVM_ENABLE_RUNTIMES` isn't set, `runtimes` list would end up empty here https://github.com/llvm/llvm-project/blob/master/llvm/runtimes/CMakeLists.txt#L32, in which case we wouldn't define `runtimes` target at all here https://github.com/llvm/llvm-project/blob/master/llvm/runtimes/CMakeLists.txt#L497.
> 
> We also cannot just check whether `LLVM_ENABLE_RUNTIMES` is set because in theory we still support the nested layout where users can place runtimes directly into `llvm/runtimes` subdirectory; we probably don't need this now that we're in monorepo, but we shouldn't break it until we officially announce the deprecation of that feature.
We're not adding this dependency unconditionally. This line is under an `if(runtimes)` block, so at this point where we're adding runtimes to clang-bootstrap-deps, the runtimes list won't be empty and the runtimes target would be defined. Additionally, if the `runtimes` target wasn't defined, then the `runtimes-configure` target which is referenced in the line above also wouldn't be defined, but since that line works fine, adding an additional dependency on `runtimes` should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71430