Skip to content
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

Raise when symlinks use different drive in Windows #6201

Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Dec 9, 2019

Changelog: Fix: Raise error for symlinks in Windows that point to a different unit.
Docs: Omit

Close #6197

Note this is almost impossible to test: it requires Administrator permissions to create a unit X: and then mklink to it to reproduce

Copy link

@guban guban left a comment

To see whether swallowing the error is appropriate, consider two scenarios:

  1. Scenario A: The user intentionally wants to export a symlink.
  2. Scenario B: The user does not export the symlink.

Also, keep in mind that the symlink is not necessarily committed to the version control: it could be made just for a local use and added to .gitignore.

In scenario A, we should fail, telling the user that exporting the symlink is a bad idea and is not supported for the user's own good. Swallowing the error in this scenario is not good.

In scenario B, we can swallow the error because the symlink is not exported, so it won't cause any problems for conan export command. In this scenario, though, we shouldn't even touch that symlink: the processing should be applied only to the files that are actually exported.

Considering that

  1. both scenarios are unusual and rare, and
  2. implementing scenario B is probably quite expensive,

a good solution is to always fail with an error (i.e., treating all scenarios as scenario A), and print the symlink path as part of error message, so that users in scenario B could easily find the culprit.

@lasote
Copy link
Contributor

@lasote lasote commented Jan 2, 2020

a good solution is to always fail with an error (i.e., treating all scenarios as scenario A), and print the symlink path as part of error message, so that users in scenario B could easily find the culprit.

+1

@memsharded
Copy link
Member Author

@memsharded memsharded commented Jan 2, 2020

Understood, made it fails always with an informative message.

@memsharded memsharded changed the title skip different drive errors Raise when symlinks use different drive in Windows Jan 14, 2020
@memsharded memsharded requested a review from jgsogo Jan 14, 2020
jgsogo
jgsogo approved these changes Jan 14, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Easy solution. Totally agree this is the best we can do 👍

@memsharded memsharded merged commit 4a69273 into conan-io:develop Jan 14, 2020
2 checks passed
@memsharded memsharded deleted the feature/symlinks_win_relative_error branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants