[PATCH] D65270: [CMake] Fix source path generation for install in multi-config (MSBuild)

2020-01-28 Thread Richard Musil via Phabricator via cfe-commits
risa2000 added a comment.

I wonder if someone could explain what happened and if this has been dropped 
because of the comment from @beanz? I am not using XCode so I cannot comment on 
its requirements nor fix the code for it.

Someone has subscribed to this bug on 
https://bugs.llvm.org/show_bug.cgi?id=41030 so I got an urge to check on the 
bug again and possibly get to some conclusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65270



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D65270: [CMake] Fix source path generation for install in multi-config (MSBuild)

2019-07-27 Thread Richard Musil via Phabricator via cfe-commits
risa2000 added inline comments.



Comment at: clang/lib/Headers/CMakeLists.txt:190
 
+if(CMAKE_CONFIGURATION_TYPES)
+  string(REPLACE "${CMAKE_CFG_INTDIR}" "$" output_dir "${output_dir}")

beanz wrote:
> I think this will break Xcode
I guess I put it in since it seemed to be already used around:

```
$ grep -r "if(CMAKE_CONFIGURATION_TYPES)" *
compiler-rt/cmake/Modules/AddCompilerRT.cmake:  if(CMAKE_CONFIGURATION_TYPES)
compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake:
if(CMAKE_CONFIGURATION_TYPES)
compiler-rt/CMakeLists.txt:if(CMAKE_CONFIGURATION_TYPES)
lldb/utils/lldb-dotest/CMakeLists.txt:  if(CMAKE_CONFIGURATION_TYPES)
llvm/cmake/modules/AddLLVM.cmake:if(CMAKE_CONFIGURATION_TYPES)
llvm/cmake/modules/AddLLVM.cmake:if(CMAKE_CONFIGURATION_TYPES)
llvm/cmake/modules/CrossCompile.cmake:  if(CMAKE_CONFIGURATION_TYPES)

```
but technically, the generator expression should work equally for single and 
multi-config generators, so the condition is not necessary. What exactly will 
break Xcode?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65270



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D65270: [CMake] Fix source path generation for install in multi-config (MSBuild)

2019-07-25 Thread Richard Musil via Phabricator via cfe-commits
risa2000 created this revision.
risa2000 added a reviewer: phosek.
risa2000 added a project: LLVM.
Herald added subscribers: llvm-commits, Sanitizers, cfe-commits, mgorny.
Herald added projects: clang, Sanitizers.

The folder expansion in install scripts created by CMake for "Visual Studio" 
generator, which uses multi config builds (MSBuild), requires using "expression 
generator", in order to correctly expand the target paths.

This is a mirror of the bug (https://bugs.llvm.org/show_bug.cgi?id=41030), 
which comes also with a lengthy description and some reasoning for the change.

The short version is:

The current scripts (throughout the LLVM project) use extensively CMake 
variable `${Configuration}` to identify the current build type (e.g. Debug, 
Release, etc.), and expect that this variable gets expanded correctly when the 
runtime script is generated. This approach (in general) does not work for 
multi-config generators (e.g. MSBuild), which need all the possible build type 
script created in advance.

Some CMake scripts are working around this by using "specialized" (by build 
type) target properties (e.g. `ARCHIVE_OUTPUT_DIRECTORY_`) and 
basically do the expansion manually, some do not do that and those fail. This 
patch provides a fix for install path generations and also shows, how this 
topic should be handled in LLVM in the broader scope.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65270

Files:
  clang/lib/Headers/CMakeLists.txt
  compiler-rt/cmake/Modules/AddCompilerRT.cmake


Index: compiler-rt/cmake/Modules/AddCompilerRT.cmake
===
--- compiler-rt/cmake/Modules/AddCompilerRT.cmake
+++ compiler-rt/cmake/Modules/AddCompilerRT.cmake
@@ -2,25 +2,15 @@
 include(CompilerRTUtils)
 
 function(set_target_output_directories target output_dir)
-  # For RUNTIME_OUTPUT_DIRECTORY variable, Multi-configuration generators
-  # append a per-configuration subdirectory to the specified directory.
-  # To avoid the appended folder, the configuration specific variable must be
-  # set 'RUNTIME_OUTPUT_DIRECTORY_${CONF}':
-  # RUNTIME_OUTPUT_DIRECTORY_DEBUG, RUNTIME_OUTPUT_DIRECTORY_RELEASE, ...
+  # If we are using multi-configuration generator ensure proper folder
+  # expansion by using $ generator expression
   if(CMAKE_CONFIGURATION_TYPES)
-foreach(build_mode ${CMAKE_CONFIGURATION_TYPES})
-  string(TOUPPER "${build_mode}" CONFIG_SUFFIX)
-  set_target_properties("${target}" PROPERTIES
-  "ARCHIVE_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${output_dir}
-  "LIBRARY_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${output_dir}
-  "RUNTIME_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${output_dir})
-endforeach()
-  else()
-set_target_properties("${target}" PROPERTIES
-ARCHIVE_OUTPUT_DIRECTORY ${output_dir}
-LIBRARY_OUTPUT_DIRECTORY ${output_dir}
-RUNTIME_OUTPUT_DIRECTORY ${output_dir})
+string(REPLACE "${CMAKE_CFG_INTDIR}" "$" output_dir 
"${output_dir}")
   endif()
+  set_target_properties("${target}" PROPERTIES
+  ARCHIVE_OUTPUT_DIRECTORY ${output_dir}
+  LIBRARY_OUTPUT_DIRECTORY ${output_dir}
+  RUNTIME_OUTPUT_DIRECTORY ${output_dir})
 endfunction()
 
 # Tries to add an "object library" target for a given list of OSs and/or
Index: clang/lib/Headers/CMakeLists.txt
===
--- clang/lib/Headers/CMakeLists.txt
+++ clang/lib/Headers/CMakeLists.txt
@@ -187,6 +187,10 @@
 
 set(header_install_dir lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include)
 
+if(CMAKE_CONFIGURATION_TYPES)
+  string(REPLACE "${CMAKE_CFG_INTDIR}" "$" output_dir "${output_dir}")
+endif()
+
 install(
   FILES ${files} ${generated_files}
   DESTINATION ${header_install_dir}


Index: compiler-rt/cmake/Modules/AddCompilerRT.cmake
===
--- compiler-rt/cmake/Modules/AddCompilerRT.cmake
+++ compiler-rt/cmake/Modules/AddCompilerRT.cmake
@@ -2,25 +2,15 @@
 include(CompilerRTUtils)
 
 function(set_target_output_directories target output_dir)
-  # For RUNTIME_OUTPUT_DIRECTORY variable, Multi-configuration generators
-  # append a per-configuration subdirectory to the specified directory.
-  # To avoid the appended folder, the configuration specific variable must be
-  # set 'RUNTIME_OUTPUT_DIRECTORY_${CONF}':
-  # RUNTIME_OUTPUT_DIRECTORY_DEBUG, RUNTIME_OUTPUT_DIRECTORY_RELEASE, ...
+  # If we are using multi-configuration generator ensure proper folder
+  # expansion by using $ generator expression
   if(CMAKE_CONFIGURATION_TYPES)
-foreach(build_mode ${CMAKE_CONFIGURATION_TYPES})
-  string(TOUPPER "${build_mode}" CONFIG_SUFFIX)
-  set_target_properties("${target}" PROPERTIES
-  "ARCHIVE_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${output_dir}
-  "LIBRARY_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}" ${output_dir}
-  "RUNTIME_OUTPUT_DIRECTORY_${CONFIG_SUFFIX}"