This is an archive of the discontinued LLVM Phabricator instance.

Differential D73525

[CMake][libcxx] Don't wrap __config_site path in quotes on Windows
ClosedPublic

Authored by phosek on Jan 27 2020, 7:32 PM.

Details

Reviewers
ldionne
EricWF
mclow.lists
Commits
rG7f49dc496682: [CMake][libcxx] Don't wrap __config_site path in quotes on Windows
Summary

This is failing to compile on Windows because clang-cl is trying to
use the path with quotes, dropping them resolves the issue.

Diff Detail

Event Timeline

phosek created this revision.Jan 27 2020, 7:32 PM
Herald added a reviewer: EricWF. · View Herald TranscriptJan 27 2020, 7:32 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: libcxx-commits, dexonsmith, christof, mgorny. · View Herald Transcript
smeenai added a subscriber: smeenai.Jan 27 2020, 7:47 PM
smeenai added inline comments.
libcxx/CMakeLists.txt
881
target_compile_options(${target} PUBLIC /FI "${site_config_path}")

is more consistent with the below case, if it works.

Herald added a reviewer: mclow.lists. · View Herald TranscriptJan 27 2020, 7:47 PM
ldionne accepted this revision.Jan 29 2020, 5:12 PM
ldionne added inline comments.
libcxx/CMakeLists.txt
881

Either way works for me. @phosek Please try using this suggestion, otherwise commit as-is.

This revision is now accepted and ready to land.Jan 29 2020, 5:12 PM
phosek updated this revision to Diff 241339.Jan 29 2020, 7:36 PM
phosek marked 3 inline comments as done.
phosek added inline comments.
libcxx/CMakeLists.txt
881

I tested it locally and it seems to be working so I'll use the suggested version.

Closed by commit rG7f49dc496682: [CMake][libcxx] Don't wrap __config_site path in quotes on Windows (authored by phosek). · Explain WhyJan 29 2020, 7:41 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
libcxx/
2 lines

Diff 241340

libcxx/CMakeLists.txt

Show First 20 Lines • Show All 872 Lines • ▼ Show 20 Lines
elseif(EXISTS "${site_config_path}") elseif(EXISTS "${site_config_path}")
message(STATUS "Removing stale site configuration ${site_config_path}") message(STATUS "Removing stale site configuration ${site_config_path}")
file(REMOVE "${site_config_path}") file(REMOVE "${site_config_path}")
endif() endif()
function(cxx_add_config_site target) function(cxx_add_config_site target)
if (LIBCXX_NEEDS_SITE_CONFIG) if (LIBCXX_NEEDS_SITE_CONFIG)
if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC") if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC" OR "${CMAKE_CXX_SIMULATE_ID}" STREQUAL "MSVC")
target_compile_options(${target} PUBLIC "/FI\"${site_config_path}\"") target_compile_options(${target} PUBLIC /FI "${site_config_path}")
smeenaiUnsubmitted
Done
ReplyInline Actions
target_compile_options(${target} PUBLIC /FI "${site_config_path}")

is more consistent with the below case, if it works.

smeenai: ``` target_compile_options(${target} PUBLIC /FI "${site_config_path}") ``` is more…
ldionneUnsubmitted
Done
ReplyInline Actions

Either way works for me. @phosek Please try using this suggestion, otherwise commit as-is.

ldionne: Either way works for me. @phosek Please try using this suggestion, otherwise commit as-is.
phosekAuthorUnsubmitted
Done
ReplyInline Actions

I tested it locally and it seems to be working so I'll use the suggested version.

phosek: I tested it locally and it seems to be working so I'll use the suggested version.
else() else()
target_compile_options(${target} PUBLIC -include "${site_config_path}") target_compile_options(${target} PUBLIC -include "${site_config_path}")
endif() endif()
endif() endif()
endfunction() endfunction()
# Setup all common build flags ================================================= # Setup all common build flags =================================================
function(cxx_add_common_build_flags target) function(cxx_add_common_build_flags target)
▲ Show 20 Lines • Show All 58 Lines • Show Last 20 Lines