-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for symlinked install on windows #6
base: rolling
Are you sure you want to change the base?
Conversation
I tried running the build with
And I got the following error:
|
Can you please look at the specific install command (ament_cmake_symlink_install/ament_cmake_symlink_install.cmake:287), run it in a shell and check what it does / output / returncode? |
You need elevated permissions to create symlinks on windows as well, see: https://docs.python.org/3.4/library/os.html#os.symlink (note about Windows and |
Sure, but we could detect if the permission is available or not based on the error code. We can always fall back to:
But this needs to be debugged on Windows to gather the necessary information:
|
d805ce0
to
3f22de3
Compare
I just rebased and force pushed this branch so it has the latest changes. |
I'm running as Administrator so don't have the permissions issues. |
When running without "Run as administrator..." I get this:
I'm trying with admin now. |
With admin:
|
The IS_SYMLINK check in CMake doesn't appear to work on Windows, so I've disable the shortcut check on Windows. Also, I've changed the error checking on Windows and changed the way mklink is executed, instead executing it with cmd /C.
This still doesn't work because the generator expressions are not expanded in time to be used by execute_process. We might try using add_custom_command instead which supposedly works with generator expressions.
endif() | ||
else() | ||
string(REPLACE "/" "\\" symlink "${symlink}") | ||
string(REPLACE "/" "\\" absolute_file "${absolute_file}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use file(TO_NATIVE_PATH
for these kind of conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, but those lines may not end up being needed. This pull request isn't really ready for review since it isn't working yet.
This still doesn't work because the generator expressions are not expanded in time to be used by However, this is not high priority so we're pushing it back to the backlog. |
d9277b7
to
85924cc
Compare
@wjwwood @dirk-thomas what is the resolution on this? that symlink installation is not possible on Windows without admin privileges and that this PR should be closed? |
Imo this could still be implemented as described. When the user has the privilege create symlinks, otherwise either fail with a reasonable error message or fall back to not use symlinks. |
I agree, so in light of that I think my previous comment still applies: currently doesn't work, might be fixed with another alternative, not high enough priority to try for now. |
sounds good thanks for the follow-up |
@dirk-thomas is this still a valid PR? |
It would still be a good feature to have on Windows. I am not planning to spend any time on it though. |
@wjwwood Whenever you have time to try it.