This is an archive of the discontinued LLVM Phabricator instance.

lib/Header: Simplify CMakeLists.txt
ClosedPublic

Authored by tstellar on Feb 21 2019, 6:37 PM.

Details

Summary

Replace cut and pasted code with cmake macros and reduce the number of
install commands. This fixes an issue where the headers were being
installed twice.

This clean up should also make future modifications easier, like
adding a cmake option to install header files into a custom resource
directory.

Event Timeline

tstellar created this revision.Feb 21 2019, 6:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 6:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
smeenai accepted this revision.Feb 21 2019, 8:13 PM
smeenai added reviewers: beanz, phosek.

LGTM, though I generally prefer functions to macros.

clang/lib/Headers/CMakeLists.txt
127–130

I'd prefer this be converted to a function and the append to out_files be handled explicitly via PARENT_SCOPE (you could either hardcode the name or take in a parameter for the name of the list to append to). Macros pollute variables and their parameters behave weirdly.

138

This one can trivially be a function instead of a macro, right?

This revision is now accepted and ready to land.Feb 21 2019, 8:13 PM
tstellar updated this revision to Diff 188500.Feb 26 2019, 9:32 PM

Use macros instead of functions.

Use macros instead of functions.

I mean "Use functions instead of macros."

smeenai accepted this revision.Feb 27 2019, 1:47 PM

LGTM, thanks!

clang/lib/Headers/CMakeLists.txt
136

I don't think this comment is necessary – this is a pretty common CMake pattern, and I think the PARENT_SCOPE is enough of a search hint for people who aren't familiar with it.

tstellar updated this revision to Diff 188749.Feb 28 2019, 9:04 AM

Fix an issue with the generated arm headers that I discovered after
doing some more testing. Also, remove comment.

@tstellar are you planning to land this soon? It'll conflict with D58791, but I'm not planning to land that for another few days, so I can rebase on top of this one.

@tstellar are you planning to land this soon? It'll conflict with D58791, but I'm not planning to land that for another few days, so I can rebase on top of this one.

Yes, I'll push this later today.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2019, 4:49 PM
vzakhari added inline comments.
cfe/trunk/lib/Headers/CMakeLists.txt
168 ↗(On Diff #189010)

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.

@tstellar ping. Someone appears to be running into this on the CMake mailing list too: https://cmake.org/pipermail/cmake/2019-April/069359.html

@tstellar ping. Someone appears to be running into this on the CMake mailing list too: https://cmake.org/pipermail/cmake/2019-April/069359.html

Sorry, I missed this. I will take a look.

tstellar marked an inline comment as done.Apr 22 2019, 4:27 PM
tstellar added inline comments.
cfe/trunk/lib/Headers/CMakeLists.txt
168 ↗(On Diff #189010)

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?

vzakhari added inline comments.Apr 23 2019, 10:29 AM
cfe/trunk/lib/Headers/CMakeLists.txt
168 ↗(On Diff #189010)

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.

tstellar marked an inline comment as done.Apr 23 2019, 7:35 PM

Can you test D61054?

cfe/trunk/lib/Headers/CMakeLists.txt
168 ↗(On Diff #189010)

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.

Can you test D61054?

I will do it ASAP, but I am currently having problems with my build system. I guess it is OK to proceed with this change. I will send an update, when I try it. Thank you for fixing this!