[PATCH] D109593: WIP: [libcxx] Add a CI configuration for standalone building in llvm-project/runtimes

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: runtimes/CMakeLists.txt:77-82
 # This variable makes sure that e.g. llvm-lit is found.
-set(LLVM_MAIN_SRC_DIR ${LLVM_BUILD_MAIN_SRC_DIR})
+set(LLVM_MAIN_SRC_DIR ${CMAKE_CURRENT_SOURCE_DIR}/../llvm)
 set(LLVM_CMAKE_PATH ${LLVM_MAIN_SRC_DIR}/cmake/modules)
 
 # This variable is used by individual runtimes to locate LLVM files.
+set(LLVM_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../llvm)
----------------
phosek wrote:
> Can we land this change as a separate commit?
Sure


================
Comment at: runtimes/CMakeLists.txt:95
-if (NOT MSVC)
-  set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -nostdinc++ -nostdlib++")
-endif()
----------------
phosek wrote:
> phosek wrote:
> > Rather than removing this altogether, could we either check if the compiler supports these flags or if the compiler is Clang?
> `-nostdinc++` should be support by GCC.
I don't remove it altogether, I move setting it to llvm/runtimes/CMakeLists.txt (where it's only used by the currently-built clang, so it's fine to use it unconditionally).

But keeping them here with detection also sounds fine to me. FWIW they're pretty much duplicate and unnecessary right now though, as at least libunwind, libcxxabi and libcxx all try to detect and add these flags themselves.


================
Comment at: runtimes/CMakeLists.txt:100-103
+if (NOT LLVM_DEFAULT_TARGET_TRIPLE)
+  include(GetHostTriple)
+  get_host_triple(LLVM_DEFAULT_TARGET_TRIPLE)
+endif()
----------------
phosek wrote:
> Can we land this change as a separate commit?
Sure, can do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109593