[CMake] fix symlink creation on Windows #6387
Conversation
|
CC @llvm-beanz I think that Chris had some fixes for the llvm macros and we should try to reuse those. |
I thought these were for executables? Not .py and .dylib files |
llvm-beanz
left a comment
llvm-beanz
left a comment
There was a problem hiding this comment.
I put inline comments below. In general I think it is best to avoid using symlinks on Windows.
lib/SwiftDemangle/CMakeLists.txt
Outdated
There was a problem hiding this comment.
Is this even needed for Windows? This is really a hack to workaround a library being renamed. I'm not even sure we actually need to create this symlink at all anymore.
There was a problem hiding this comment.
@jrose-apple I think you added this back in 2015. What do you think?
There was a problem hiding this comment.
In LLVM our normal approach is to replace "create_symlink" with "copy" on non-unix systems. I think that is way simpler, and a better approach. Especially because calling mklink on Windows requires elevated privileges, and I think it would be unfortunate if building Swift required elevated privileges. That would specifically hamper the ability of students to checkout and build Swift on University-owned machines.
There was a problem hiding this comment.
Hmm, have an account with elevated privileges. I've noticed that I get privilege errors using mklink Without any arguments. They seem to go away when I add /H or /J.
That said, i can change to be a basic copy_if_different/copy if you'd like.
There was a problem hiding this comment.
I had to do some research to understand the difference between /H, /J, and /D. Turns out /H and /J introduce other complications that are possibly undesirable in some contexts. Specifically they cannot be used to point across volume boundaries. With that context I definitely think it is better to use copy_if_different or copy.
There was a problem hiding this comment.
Aye aye, will do - thanks
|
Thanks @llvm-beanz - I've updated the PR to use copying instead of symlinking on Windows. In the second commit I removed the demangle symlinking as you suggested |
llvm-beanz
left a comment
llvm-beanz
left a comment
There was a problem hiding this comment.
One comment, otherwise LGTM.
cmake/modules/SwiftUtils.cmake
Outdated
There was a problem hiding this comment.
Rather than lots of named parameters, why not use cmake_parse_arguments with named parameters? That is much easier to read.
I'm fine with the source, dest, and target being named parameters, but is_directory should be a flag, and working_directory and comment should be single option parameters (potentially even optional).
|
@llvm-beanz I've addressed your comments, looks nicer now |
|
@hughbe LGTM! Thanks! |
|
@swift-ci Please smoke test |
4 participants
Introduce
swift_create_post_build_symlinkand a batch script to delete an existing symlink and create a new one.The reason I need a batch script is because CMake's
if(EXISTS)doesn't work for symlinks, CMakescreate_symlinkdoesn't work on Windows, andmklinkthrows errors when creating a symlink when a symlink already exists.I can't use
swift_create_post_build_symlinkinstdlib/public/SwiftShims/CMakeLists.txtbecause this is a target, not a command@compnerd @jrose-apple