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

Python's shutil.copy2 fails on Android #2016

Merged
merged 1 commit into from Oct 7, 2016

Conversation

Projects
None yet
4 participants
@jerryasher

Python's shutil.copy2 fails on Android when copying a file's meta data (perm bits, access times) onto certain filesystems (presumably FAT). This is documented as python issue28141

These commits workaround that bug by

  • creating a new function copy_file_metadata in utils.py
  • wrapping calls to copy2 in a try/except clause that logs any errors that occur but keep execution going
  • changing the calls to shutil.copy2 to calls to the new function
@iKevinY

Thanks for reopening this PR on a new branch, @jerryasher! flake8 is complaining about a couple of things; namely, it wants the imports in generators.py to appear in alphabetical order, and since your new helper function wraps shutil.copy2, shutil doesn't need to be imported anymore.

@jerryasher

This comment has been minimized.

Show comment
Hide comment
@jerryasher

jerryasher Sep 29, 2016

Thanks, I'll attend to those.

I appreciate the output of flake8, I need to run that manually. I use emacs
and have had to turn elpy-mode off, because something about elpy keeps
rewriting a lot of the whitespace of one of the two files, and I haven't
figured out yet how to turn the rewriting off.

On Wed, Sep 28, 2016 at 6:01 PM, Kevin Yap notifications@github.com wrote:

@iKevinY requested changes on this pull request.

Thanks for reopening this PR on a new branch, @jerryasher
https://github.com/jerryasher! flake8 is complaining about a couple of
things https://travis-ci.org/getpelican/pelican/jobs/163572182#L186;
namely, it wants the imports in generators.py to appear in alphabetical
order, and since your new helper function wraps shutil.copy2, shutil
doesn't need to be imported anymore.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2016 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAosCVF34jRgnkVHetryBz5oqItVsuNoks5quw3vgaJpZM4KJcR3
.

Thanks, I'll attend to those.

I appreciate the output of flake8, I need to run that manually. I use emacs
and have had to turn elpy-mode off, because something about elpy keeps
rewriting a lot of the whitespace of one of the two files, and I haven't
figured out yet how to turn the rewriting off.

On Wed, Sep 28, 2016 at 6:01 PM, Kevin Yap notifications@github.com wrote:

@iKevinY requested changes on this pull request.

Thanks for reopening this PR on a new branch, @jerryasher
https://github.com/jerryasher! flake8 is complaining about a couple of
things https://travis-ci.org/getpelican/pelican/jobs/163572182#L186;
namely, it wants the imports in generators.py to appear in alphabetical
order, and since your new helper function wraps shutil.copy2, shutil
doesn't need to be imported anymore.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2016 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAosCVF34jRgnkVHetryBz5oqItVsuNoks5quw3vgaJpZM4KJcR3
.

@avaris

If I'm not mistaken, the block that uses shutil in generators.py can be replaced by pelican.utils.copy. With that, you don't need the extra function since you can just put the try/except in there.

Not exactly the scope of this PR, but it makes sense in terms of organizing/clean-up.

@jerryasher

This comment has been minimized.

Show comment
Hide comment
@jerryasher

jerryasher Sep 29, 2016

@avaris I see what you're saying, the new little worker, copy_file_metadata is actually used twice within utils's copy, so maybe it makes more sense to keep that utility then duplicate the try/except.

Hey, can you explain what def walk_error does within the def copy function? Is that just a vestigial piece of code or does it have some function?

@avaris I see what you're saying, the new little worker, copy_file_metadata is actually used twice within utils's copy, so maybe it makes more sense to keep that utility then duplicate the try/except.

Hey, can you explain what def walk_error does within the def copy function? Is that just a vestigial piece of code or does it have some function?

@avaris

This comment has been minimized.

Show comment
Hide comment
@avaris

avaris Sep 30, 2016

Member

Looks like walk_error was introduced in 0a48371. I don't know why it's there, I don't see it being used anywhere. Maybe it was part of the overhaul at some point and refactored out, but left with the stray definition. In any case, it should be safe to remove it.

Edit: Also, just being used twice doesn't require a separate function, IMO. Maybe you can make it a local function like it was with walk_error. Or if not, at the very least, can you move the definition closer to copy. Right now it's way too spread :).

Member

avaris commented Sep 30, 2016

Looks like walk_error was introduced in 0a48371. I don't know why it's there, I don't see it being used anywhere. Maybe it was part of the overhaul at some point and refactored out, but left with the stray definition. In any case, it should be safe to remove it.

Edit: Also, just being used twice doesn't require a separate function, IMO. Maybe you can make it a local function like it was with walk_error. Or if not, at the very least, can you move the definition closer to copy. Right now it's way too spread :).

@jerryasher

This comment has been minimized.

Show comment
Hide comment
@jerryasher

jerryasher Oct 7, 2016

Hi @justinmayer, I see the label "awaiting user feedback" was added to this pull request, but I guess I do not know who "the user" is. Are you waiting on me?

If so, are you waiting on me to comment on @avaris' comment?

I did move the function copy_file_metadata up to below the copy function. I did not remove walk_error, though I am happy to do so, as it is extraneous to what this change request requires.

Tonight I

  1. updated my master branch with the latest getpelican master by using git pull
  2. updated my copy_file_metadata branch by merging my master branch into it.
  3. pushing both of my branches onto my repository here.

This places two commits into this pull request. The first are my changes, the next is presumably the recent changes to getpelican/pelican master.

I am not sure if I needed to update my branch at all, or if I did it the right way. What do you think?

Hi @justinmayer, I see the label "awaiting user feedback" was added to this pull request, but I guess I do not know who "the user" is. Are you waiting on me?

If so, are you waiting on me to comment on @avaris' comment?

I did move the function copy_file_metadata up to below the copy function. I did not remove walk_error, though I am happy to do so, as it is extraneous to what this change request requires.

Tonight I

  1. updated my master branch with the latest getpelican master by using git pull
  2. updated my copy_file_metadata branch by merging my master branch into it.
  3. pushing both of my branches onto my repository here.

This places two commits into this pull request. The first are my changes, the next is presumably the recent changes to getpelican/pelican master.

I am not sure if I needed to update my branch at all, or if I did it the right way. What do you think?

@justinmayer

This comment has been minimized.

Show comment
Hide comment
@justinmayer

justinmayer Oct 7, 2016

Member

Yes, I was awaiting your response to the latest comments from @avaris. Now that you have made some follow-up changes, there are two things left to do:

First, as you can see by the indicator at the bottom of this pull request, there is a failing check. In this case, it is compliance with PEP 8 that needs to be addressed (see test output). You can run flake8 locally in order to display the failing check(s), and then you can manually fix the displayed problem(s):

cd pelican
pip install -r requirements/developer.pip
flake8 pelican/generators.py pelican/utils.py

Second, once flake8 checks pass, you should rebase & squash your commits into a single commit.

Member

justinmayer commented Oct 7, 2016

Yes, I was awaiting your response to the latest comments from @avaris. Now that you have made some follow-up changes, there are two things left to do:

First, as you can see by the indicator at the bottom of this pull request, there is a failing check. In this case, it is compliance with PEP 8 that needs to be addressed (see test output). You can run flake8 locally in order to display the failing check(s), and then you can manually fix the displayed problem(s):

cd pelican
pip install -r requirements/developer.pip
flake8 pelican/generators.py pelican/utils.py

Second, once flake8 checks pass, you should rebase & squash your commits into a single commit.

Jerry Asher
Python's shutil.copy2 fails on Android
Python's shutil.copy2 fails on Android when copying a file's meta data (perm bits, access times) onto certain filesystems. This is documented as python issue28141 https://bugs.python.org/issue28141

These commits workaround that bug by

+ creating a new function copy_file_metadata in utils.py
+ wrapping calls to copy2 in a try/except clause that logs any errors that occur but keep execution going
+ changing the calls to shutil.copy2 to calls to the new function
@jerryasher

This comment has been minimized.

Show comment
Hide comment
@jerryasher

jerryasher Oct 7, 2016

Okay, try this one!

Okay, try this one!

@justinmayer

This comment has been minimized.

Show comment
Hide comment
@justinmayer

justinmayer Oct 7, 2016

Member

Appreciate for your work on this, Jerry. And thanks to @iKevinY and @avaris for reviewing.

Member

justinmayer commented Oct 7, 2016

Appreciate for your work on this, Jerry. And thanks to @iKevinY and @avaris for reviewing.

@justinmayer justinmayer added this to the 3.7 milestone Oct 7, 2016

@justinmayer justinmayer merged commit b2231c4 into getpelican:master Oct 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jerryasher jerryasher deleted the jerryasher:copy_file_metadata branch Oct 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment