[libcxx] Use joined format for include flag on Windows

[phosek]

Summary

Without this, CMake deduplicates the /I flag breaking the build. See https://cmake.org/cmake/help/v3.13/command/target_compile_options.html for more details on why this is needed.

[mstorsjo]

Can you explain the issue, including the circumstances where/when the issue occurs, in a bit more detail, I don't quite follow. (We do have CI for windows, in an msvc setting, so it works in some configs at least.)

[phosek]

In D101550#2726176, @mstorsjo wrote:

Can you explain the issue, including the circumstances where/when the issue occurs, in a bit more detail, I don't quite follow. (We do have CI for windows, in an msvc setting, so it works in some configs at least.)

See the last paragraph on https://cmake.org/cmake/help/v3.13/command/target_compile_options.html, I'll add it to the description as well.

[mstorsjo]

In D101550#2726200, @phosek wrote:

In D101550#2726176, @mstorsjo wrote:

Can you explain the issue, including the circumstances where/when the issue occurs, in a bit more detail, I don't quite follow. (We do have CI for windows, in an msvc setting, so it works in some configs at least.)

See the last paragraph on https://cmake.org/cmake/help/v3.13/command/target_compile_options.html, I'll add it to the description as well.

Thanks, that explains the "what". (FTR, for both the -D options in the example, and the -I options here, there's also the possibility to write them joined.)

What I'm still wondering though, is, in which cases does this crop up, as it builds fine in all configs I know of? Is this to avoid issues with some other patch that adds more include dirs?

[phosek]

In D101550#2726379, @mstorsjo wrote:

Thanks, that explains the "what". (FTR, for both the -D options in the example, and the -I options here, there's also the possibility to write them joined.)

What I'm still wondering though, is, in which cases does this crop up, as it builds fine in all configs I know of? Is this to avoid issues with some other patch that adds more include dirs?

I'm trying to use the runtimes build on Windows and include libc++ in the build which is where I ran into this issue.

Prior to ea12d779bc238c387511fe7462020f4ecf4a8246 there was only one /I and that change only landed yesterday which likely explains why we haven't seen this earlier?

I tried using the joined form but that didn't work but that may have to be because of the quote placement. We need to use quotes around the path because of spaces (we ran into that issue before) but I'm not sure if the correct joined form is /I"${LIBCXX_GENERATED_INCLUDE_DIR}" or "/I${LIBCXX_GENERATED_INCLUDE_DIR}", do you know?

[mstorsjo]

In D101550#2726500, @phosek wrote:

I'm trying to use the runtimes build on Windows and include libc++ in the build which is where I ran into this issue.

Prior to ea12d779bc238c387511fe7462020f4ecf4a8246 there was only one /I and that change only landed yesterday which likely explains why we haven't seen this earlier?

Ah, indeed, that would probably explain it.

I tried using the joined form but that didn't work but that may have to be because of the quote placement. We need to use quotes around the path because of spaces (we ran into that issue before) but I'm not sure if the correct joined form is /I"${LIBCXX_GENERATED_INCLUDE_DIR}" or "/I${LIBCXX_GENERATED_INCLUDE_DIR}", do you know?

On the windows command line quoting level, I think both of them are equally ok, but I'm not sure how this translates from the cmake level here to the actual final command line.

I'm fine with this solution in any case as it seems to be what cmake recommends. But it's a bit weird, because - doesn't this practically pass things as one single arg, containing /I<space>\path\to\dir? If that works with the compilers in practice I guess that's ok even though it's weird.

[phosek]

I tested it a bit more and looks like the joined form without quotes is working so I'll go with that since it better matches what we do on other platforms.

[mstorsjo]

LGTM. Thanks, this feels less weird.

libcxx/include/CMakeLists.txt

 endforeach()
 
 add_custom_target(generate-cxx-headers ALL DEPENDS ${_all_includes})
 
 add_library(cxx-headers INTERFACE)
 add_dependencies(cxx-headers generate-cxx-headers ${LIBCXX_CXX_ABI_HEADER_TARGET})
 # TODO: Use target_include_directories once we figure out why that breaks the runtimes build
 if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC")
-  target_compile_options(cxx-headers INTERFACE /I "${LIBCXX_GENERATED_INCLUDE_DIR}"
-                                     INTERFACE /I "${LIBCXX_GENERATED_INCLUDE_TARGET_DIR}")
+  target_compile_options(cxx-headers INTERFACE /I${LIBCXX_GENERATED_INCLUDE_DIR}
+                                     INTERFACE /I${LIBCXX_GENERATED_INCLUDE_TARGET_DIR})
 else()
   target_compile_options(cxx-headers INTERFACE -I${LIBCXX_GENERATED_INCLUDE_DIR}
                                      INTERFACE -I${LIBCXX_GENERATED_INCLUDE_TARGET_DIR})
 endif()
 
 if (LIBCXX_INSTALL_HEADERS)
   foreach(file ${files})
     get_filename_component(dir ${file} DIRECTORY)