[CMake] Fix bad dependency in symlink_clang_headers #5034
Conversation
The problem here is a bit complicated. The symlink_clang_headers target creates two symlinks to clang's headers, one under the build directory at lib/swift/clang, and the other under a temporary path. The one under the temporary path is a bad symlink, and it is only created during the build so that it can be installed. It isn't actually used by the build. Ninja treats the bad symlink as a non-existing file, and since the build rule that creates it has the restat property on it this results in the commands to symlink the clang headers directory running over and over and over again during the build. This patch prevents that by not generating the bad symlink during the build. Instead we generate it at install time using the LLVMInstallSymlink script that is vended as part of LLVM's distribution.
|
@swift-ci please smoke test |
|
Seems reasonable to me, but I'd want to make sure it doesn't break our existing install process. @shahmishal, @zisko? |
|
@swift-ci Please test |
|
Build failed |
|
Huh... That error doesn't seem related to my change. @swift-ci please test platform linux |
|
@swift-ci please test linux platform |
|
LGTM |
| function(swift_install_symlink_component component) | ||
| cmake_parse_arguments( | ||
| ARG # prefix | ||
| "" "LINK_NAME;TARGET;DESTINATION" "" ${ARGN}) |
There was a problem hiding this comment.
Chris, one CMake utility that would be useful is a wrapper around cmake_parse_arguments that is self documenting. I.e.:
swift_parse_arguments(
PREFIX Arg
FLAGS ""
OPTIONS "LINK_NAME;TARGET;DESTINATION"
MULTIVALUE_OPTIONS ""
ARGS ${ARGN}).
Thoughts?
There was a problem hiding this comment.
Better yet, we should suggest that syntax to the CMake developers and see if they'll run with it.
That said, I generally think that the bar for providing a wrapper around built-in CMake functionality should be pretty high. CMake has excellent online documentation, and providing custom wrappers for built-in functionality makes the code less approachable from someone outside the project. I do like the syntax you're suggesting, but IMO the benefit isn't substantial enough to break from convention.
There was a problem hiding this comment.
@llvm-beanz I disagree with you here. We are already in many parts of swift performing these annotations in the comments. It would be better to just make the code self document rather than rely on comments. OR we should strip out all of the comments. I would prefer that we be consistent here. Notice how you left in one of the comments (for the prefix), but removed the one for options, etc. We should be consistent here.
There was a problem hiding this comment.
I'm happy to add the full set of comments in. Also, I realized why CMake doesn't support the behavior you suggested, and I think it would be very difficult for us to support it.
Take for example this situation:
swift_parse_args(PREFIX FOO MULTIVALUE_OPTIONS OPTIONS)
What is the expected behavior of that? It could be specifying a multi-value option named "OPTIONS", or defining MULTIVALUE_OPTIONS and OPTIONS as empty.
There was a problem hiding this comment.
Ok. Please just add in the comments. Thanks!
| return() | ||
| endif() | ||
|
|
||
| foreach(path ${CMAKE_MODULE_PATH}) |
There was a problem hiding this comment.
Rather than iterating over the cmake module path, I wonder if LLVM can just tell us its module path via the exports. I think we have that, no?
There was a problem hiding this comment.
There is no variable defined in the config files and in-tree builds, so this solution is probably the most portable.
There was a problem hiding this comment.
For the in tree builds, we know the location of this file. For the out of tree builds, we have this:
set(LLVM_CMAKE_DIR "@LLVM_CONFIG_CMAKE_DIR@")
There was a problem hiding this comment.
And again that value is already exported. So for instance in my local build:
set(LLVM_CMAKE_DIR "/Volumes/Files/work/solon/llvm/cmake/modules")
The file you are looking for is in that folder.
| DESTINATION "lib/swift") | ||
| swift_install_symlink_component(clang-resource-dir-symlink | ||
| LINK_NAME clang | ||
| TARGET ../clang/${CLANG_VERSION} |
There was a problem hiding this comment.
Can you use an absolute path here?
There was a problem hiding this comment.
No. The target is the path the symlink points to, which is intentionally relative to where it gets installed to. That makes it so the install directory can be moved around or relocated without breaking.
The problem here is a bit complicated. The symlink_clang_headers target creates two symlinks to clang's headers, one under the build directory at lib/swift/clang, and the other under a temporary path.
The one under the temporary path is a bad symlink, and it is only created during the build so that it can be installed. It isn't actually used by the build. Ninja treats the bad symlink as a non-existing file, and since the build rule that creates it has the restat property on it this results in the commands to symlink the clang headers directory running over and over and over again during the build.
This patch prevents that by not generating the bad symlink during the build. Instead we generate it at install time using the LLVMInstallSymlink script that is vended as part of LLVM's distribution.