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

fix: Gracefully handle read-only sdists #1140

Merged
merged 1 commit into from Jun 2, 2020

Conversation

epage
Copy link
Contributor

@epage epage commented May 30, 2019

For a package without dependencies in pypi's database, like p4python,
the sdist is required. The problem is p4python was developed in
perforce where all files are read-only by default and deleting the temp
directory fails.

So we need to use the custom-built temp directory and specially handle
read-only files to be able to use p4python in poetry.

Fixes #520

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

  • Added tests for changed code. Unsure what is best route for testing this
  • Updated documentation for changed code. N/A

Note: If your Pull Request introduces a new feature or changes the current behavior, it should be based
on the develop branch. If it's a bug fix or only a documentation update, it should be based on the master branch.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

poetry/utils/helpers.py Outdated Show resolved Hide resolved
@@ -27,19 +27,18 @@ def normalize_version(version): # type: (str) -> str
return str(Version(version))


def del_ro(action, name, exc):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a check that making the file writable will fix the error raised by rmtree (or that the error was raised because of being nonwriteable) wouldn't hurt. In case different error ocurred, making a file writable might be unwanted? But that's just me guessing.

Copy link
Contributor Author

@epage epage Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering this is being applied to a temp directory, specifically one being used for unpacking sdist's, I doubt making something writeable before deletion would hurt anything.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, my comment really comes from worrying that the code isn't self-documenting in this case and might benefit from a comment explaining why there's the `onerror, what is it supposed to handle... But that's just my personal feeling.

Copy link
Contributor Author

@epage epage Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I feel that the "what" is clear (delete read-only files), the problem is the "why" which is that some sdist's contain read-only files.

The question is whether that kind of "why" lives better in a comment or in a commit message. Personally, this feels more like the type of historical information that is best left in a commit message but I can also see the case for why it could be a comment.

So I'm fine changing this if its what is needed to get this PR accepted.

@epage
Copy link
Contributor Author

@epage epage commented Jun 6, 2019

Hmm, should this have been against master rather than develop?

@brycedrennan brycedrennan added the Bug Something isn't working as expected label Aug 17, 2019
@Lego3
Copy link

@Lego3 Lego3 commented Oct 31, 2019

@iscre4m , @epage I would really benefit from this change as well. What would be the next step to get this included?

@thejcannon
Copy link
Contributor

@thejcannon thejcannon commented Jan 27, 2020

Is there anything blocking this from being reviewed (and approved)? It's a simple change, but without it is a huge snag.

@RoscoP
Copy link

@RoscoP RoscoP commented Feb 12, 2020

@iscre4m sorry to ping, but I'm hoping to see this PR get in if it is acceptable. If not, can you leave actionable feedback and hopefully someone picks it up?

@djetelina
Copy link

@djetelina djetelina commented Feb 12, 2020

@RoscoP I wish I could help, but I’m just a random guy who wanted to help Poetry a bit and randomly reviewed this PR.

@finswimmer finswimmer requested a review from Feb 12, 2020
@epage epage changed the base branch from develop to master Feb 29, 2020
@epage
Copy link
Contributor Author

@epage epage commented Feb 29, 2020

This change is now rebased and targeting master since I'm assuming that is more correct? Would be nice to have guidance

Copy link
Member

@finswimmer finswimmer left a comment

Looks good to me. Thanks a lot 👍

I wonder why the tests haven't started. So could you please commit any small change to force rerunning the tests?

fin swimmer

@epage
Copy link
Contributor Author

@epage epage commented Mar 13, 2020

Rebase and repushed which queued the tests back up

@epage
Copy link
Contributor Author

@epage epage commented Mar 13, 2020

Tests passed. Looks like this is ready @finswimmer

finswimmer
finswimmer previously approved these changes Mar 14, 2020
Copy link
Member

@finswimmer finswimmer left a comment

Thanks a lot 👍

Looks good to me!

@epage
Copy link
Contributor Author

@epage epage commented Mar 19, 2020

@finswimmer with this being approved, what is the next step for merging?

@finswimmer finswimmer requested a review from sdispater Mar 20, 2020
@thejcannon
Copy link
Contributor

@thejcannon thejcannon commented Mar 25, 2020

Any love for this PR? It's the only thing holding us back from adopting poetry at a much wider level (we use p4python a lot)

@sebastien-savard
Copy link

@sebastien-savard sebastien-savard commented Apr 7, 2020

I would love to see this fix get in, just ran into this issue with p4python yesterday.

@irwand
Copy link

@irwand irwand commented Apr 9, 2020

Can we get this in please? This would help us to adopt poetry in our company. Thank you!!

@sbethur
Copy link

@sbethur sbethur commented Apr 17, 2020

I too ran into this issue. Can the maintainers please accept the fix. Thanks!

@RoscoP
Copy link

@RoscoP RoscoP commented May 26, 2020

@sdispater - Sorry to pester, hoping this PR can make it in soon or maybe tied to a milestone.

@finswimmer
Copy link
Member

@finswimmer finswimmer commented May 31, 2020

Hello @epage,

sorry for ping you again. As the checks have changed and so not all necessary test are completed, one more push to force rerunning the test is needed :( I promise to merge this PR than just in time.

Thanks a lot!

fin swimmer

For a package without dependencies in pypi's database, like p4python,
the sdist is required.  The problem is p4python was developed in
perforce where all files are read-only by default and deleting the temp
directory fails.

So we need to use the custom-built temp directory and specially handle
read-only files to be able to use p4python in poetry.

Fixes python-poetry#520
@epage
Copy link
Contributor Author

@epage epage commented Jun 2, 2020

@finswimmer just did a new push

Copy link
Member

@finswimmer finswimmer left a comment

Thanks a lot!

@finswimmer finswimmer merged commit 54c6734 into python-poetry:master Jun 2, 2020
16 checks passed
@sdispater sdispater mentioned this pull request Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants