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

'tar_extract' function fails if there is a linked folder in workingdir that matches one in the tar file #4965

Merged
merged 3 commits into from Apr 29, 2019

Conversation

Projects
None yet
4 participants
@jgsogo
Copy link
Member

commented Apr 12, 2019

Changelog: Bugfix: 'tar_extract' function was failing if there was a linked folder in the working dir that matches one inside the tar file. Now we use the destination_dir as base directory to check this condition.
Docs: omit

closes #4959

@ghost ghost assigned jgsogo Apr 12, 2019

@ghost ghost added the stage: review label Apr 12, 2019

jgsogo added some commits Apr 12, 2019

@jgsogo jgsogo marked this pull request as ready for review Apr 12, 2019

@jgsogo jgsogo requested a review from memsharded Apr 12, 2019

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Not for a minor, it is almost a corner-case, but I would add it to the next release.

@klimkin

This comment has been minimized.

Copy link

commented Apr 12, 2019

Unlikely, but what if the dest dir has a similar symlink? Will it be safer creating an empty temporary directory?

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

The destination directory could also have the symlink issue, which in turn opens the question of what to do with existing folders and files with the same name as those in the tar file (I'm not sure what Conan is doing in such cases).

With this change, it won't happen with packages (we clean the target directory before extracting) but can still happen with utilities from the tools module that make use of the same code. In those cases, the user is extracting to a local folder that could be not-empty. If we output a warning for these conflicting files (#4966) I think that the user will be able to handle it.

A different approach, as you suggest, would be to extract the zip to a temporary folder and then copy it to the destination dir, but this additional copy could be slow for big packages (or packages with thousands of small files).

@lasote lasote assigned memsharded and unassigned lasote and jgsogo Apr 16, 2019

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@klimkin @jgsogo Irrespective of the possibility of colliding with a previously existing file, I am merging this now, as it is already an improvement and is fixing a bug that didn't make sense.

@memsharded memsharded merged commit 82cdb0a into conan-io:develop Apr 29, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Apr 29, 2019

@memsharded memsharded added this to the 1.15 milestone Apr 29, 2019

@jgsogo jgsogo deleted the jgsogo:bug/badpath-use-destdir branch Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.