Page MenuHomePhabricator

[llvm] [cmake] Add additional headers only if they exist
ClosedPublic

Authored by mgorny on Wed, Mar 20, 11:18 PM.

Details

Summary

Modify the add_header_files_for_glob() function to only add files
that do exist, rather than all matches of the glob. This fixes CMake
error when one of the include directories (which happen to include
/usr/include) contain broken symlinks.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Wed, Mar 20, 11:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Mar 20, 11:18 PM
smeenai added a subscriber: smeenai.Wed, Apr 3, 9:40 AM
smeenai added inline comments.
llvm/cmake/modules/LLVMProcessSources.cmake
35 ↗(On Diff #191637)

I'd add a comment explaining this is for broken symlinks, otherwise I'd be wondering why you were checking for the existence of a file you just globbed...

Actually, is EXISTS resolving symlinks documented anywhere? It's unclear if it would return the existence of the symlink itself or the target of the symlink.

mgorny marked an inline comment as done.Wed, Apr 3, 10:05 AM
mgorny added inline comments.
llvm/cmake/modules/LLVMProcessSources.cmake
35 ↗(On Diff #191637)

I'd add a comment explaining this is for broken symlinks, otherwise I'd be wondering why you were checking for the existence of a file you just globbed...

Good idea.

Actually, is EXISTS resolving symlinks documented anywhere? It's unclear if it would return the existence of the symlink itself or the target of the symlink.

To be honest, I don't see it in CMake docs. I think I presumed that behavior from shell's -f operator but there's indeed some potential ambiguity here.

smeenai accepted this revision.Wed, Apr 3, 5:06 PM

LGTM with the comment added.

llvm/cmake/modules/LLVMProcessSources.cmake
35 ↗(On Diff #191637)

Looks like it just calls access, which should resolve symlinks.

This revision is now accepted and ready to land.Wed, Apr 3, 5:06 PM
smeenai added inline comments.Wed, Apr 3, 5:29 PM
llvm/cmake/modules/LLVMProcessSources.cmake
35 ↗(On Diff #191637)
mgorny marked an inline comment as done.Wed, Apr 3, 11:28 PM
mgorny added inline comments.
llvm/cmake/modules/LLVMProcessSources.cmake
35 ↗(On Diff #191637)

Thank you. I'll add the comment later today and push it.

This revision was automatically updated to reflect the committed changes.