-
-
Notifications
You must be signed in to change notification settings - Fork 232
remove dir before copytree, use copy2 to keep metadata #545
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
Conversation
Can one of the admins verify this patch? |
Thanks @jasper1918 ! |
Jenkins, test this please |
cwltool/process.py
Outdated
@@ -279,9 +279,11 @@ def moveIt(src, dst): | |||
if src != dst: | |||
_logger.debug("Copying %s to %s", src, dst) | |||
if os.path.isdir(src): | |||
if os.path.exists(dst): | |||
shutil.rmtree(dst) |
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.
Could you add a check if dst
is a directory. If it isn't a directory, it should use os.unlink() instead. (If you try to use shutil.rmtree
on a regular file it will raise an exception).
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.
Thanks @tetron. The isdir function is prob more appropriate in this case anyway. Not sure I understand the scenario where a src dir could equate to a dst file. The copy2 func will allow overwrites so no need to unlink in the else block.
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.
If src
is a directory but dst
is a file, copytree()
will fail. I believe the problem you're trying to solve is correctly overwriting files in the final directory.
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 fair. Committed for completeness. ;)
Jenkins, test this please |
Relates to #544
This PR adds: