[PATCH] D58537: lib/Header: Simplify CMakeLists.txt

[Tom Stellard via Phabricator via cfe-commits]

tstellar marked an inline comment as done.
tstellar added a comment.

Can you test D61054 <https://reviews.llvm.org/D61054>?



================
Comment at: cfe/trunk/lib/Headers/CMakeLists.txt:168
 install(
-  FILES ${cuda_wrapper_files}
-  COMPONENT clang-headers
-  PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
-  DESTINATION lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include/cuda_wrappers)
+  DIRECTORY ${output_dir}
+  DESTINATION ${header_install_dir}
----------------
vzakhari wrote:
> tstellar wrote:
> > vzakhari wrote:
> > > This change results in a use of CMAKE_CFG_INTDIR, which expands to $(Configuration) inside cmake_install.cmake, when using Visual Studio generator. CMake cannot reasonably expand $(Configuration), so Visual Studio builds are broken for me after this change-set.
> > > 
> > > Can we revert to installation from FILES relative to the current source dir?  This will require keeping a separate list of source files in copy_header_to_output_dir(), and using this list in the install() command.
> > > 
> > > I do understand that the intention was, probably, to copy headers files into output_dir along with creating some structure inside output_dir, and then installing the whole output_dir verbatim to the install dir.  It will be hard to achieve this with the change I suggest, but can we fix Visual Studio builds in any other way?
> > > 
> > > FWIW, vcproj invokes the installation with the following command: cmake -DBUILD_TYPE=$(Configuration) -P cmake_install.cmake
> > > CMake could have expanded CMAKE_CFG_INTDIR as ${BUILD_TYPE} instead of $(Configuration) inside cmake_install.cmake.
> > > This change results in a use of CMAKE_CFG_INTDIR, which expands to $(Configuration) inside cmake_install.cmake, when using Visual Studio generator. CMake cannot reasonably expand $(Configuration), so Visual Studio builds are broken for me after this change-set.
> > 
> > Prior to this change we were installing the arm generated files from ${CMAKE_CURRENT_BINARY_DIR}.  Do we really need to use ${LLVM_LIBRARY_OUTPUT_INTDIR} as the base for output_dir?  Could we use ${CMAKE_CURRENT_BINARY_DIR} instead?  That seems like it would fix this issue.
> > 
> > > FWIW, vcproj invokes the installation with the following command: cmake -DBUILD_TYPE=$(Configuration) -P cmake_install.cmake
> > CMake could have expanded CMAKE_CFG_INTDIR as ${BUILD_TYPE} instead of $(Configuration) inside cmake_install.cmake.
> > 
> > Are the vc project files part of the LLVM source tree?
> > 
> > 
> > 
> > 
> > 
> As I understand, ${CMAKE_CURRENT_BINARY_DIR} is the same directory for all (Debug, Release) builds for the multiconfiguration builders.  I am not sure if it is possible to run Debug and Release builds in parallel, which would imply parallel access to the generated files.  Maybe it is a potential problem, but we have it even now inside clang_generate_header() function.
> 
> VC project files are generated by CMake, that is why I said CMake could have used ${BUILD_TYPE} in the cmake_install.cmake files instead of $(Configuration).
> 
> I guess just using ${CMAKE_CURRENT_BINARY_DIR} as the output_dir will solve the current problem.  It should work as long as Debug and Release versions of the header files are functionally equivalent, and noone runs Debug and Release builds in parallel.
> I guess just using ${CMAKE_CURRENT_BINARY_DIR} as the output_dir will solve the current problem. It should work as long as Debug and Release versions of the header files are functionally equivalent, and noone runs Debug and Release builds in parallel.

Ok, the generated files were already installing from ${CMAKE_CURRENT_BINARY_DIR}, so if that is a problem than this was already broken before this patch.  The header files should be equivalent for all build types anyway.




Repository:
  rL LLVM

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

https://reviews.llvm.org/D58537