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 #512 - Isolate directories for generating hashes #556

Closed
wants to merge 3 commits into from

Conversation

mete0r
Copy link
Contributor

@mete0r mete0r commented Sep 1, 2017

Some distributions have incompatible type of files with same name on the root directory. For example, matplotlib-2.0.2.tar.gz has a directory named "LICENSE" on the root, which may conflict with many other distributions, causing errors like:

IOError: [Errno 20] Not a directory: u'/tmp/tmpz/LICENSE/LICENSE_STIX'

So we need to isolate unpacking directory of each distributions.

Contributor checklist
  • Provided the tests for the changes
  • Added the changes to CHANGELOG.md
  • Requested (or received) a review from another contributor

mete0r added 3 commits September 2, 2017 02:42
Some distributions have incompatible type of files with same name
on the root directory. For example, matplotlib-2.0.2.tar.gz has a
directory named "LICENSE" on the root, which may conflict many
other distributions, causing errors like:

  IOError: [Errno 20] Not a directory: u'/tmp/tmpz/LICENSE/LICENSE_STIX'

So we need to isolate unpacking directory of each distributions.
@suutari
Copy link
Contributor

suutari commented Sep 2, 2017

So this makes it possible to unpack distribution packages with conflicting files inside them.

I'm just thinking that why the code unpacks the downloaded packages in the first place, since it only needs to hash the byte contents of the package. I think the code should be simplified to not do any unpacking, but rather just hash the file contents. This should fix the bug and also make it faster and smarter since unnecessary unpacking is avoided.

I already tested this with Prequ. I'll create a PR for this to pip-tools too.

@suutari
Copy link
Contributor

suutari commented Sep 2, 2017

I have now created the PR for doing the hashing without unpacking as mentioned. See PR #557.

@mete0r: I also included your test case for this. Thanks! And it seems to pass too. :)

@mete0r
Copy link
Contributor Author

mete0r commented Sep 3, 2017

@suutari yeah, you are right and I considered that too, but just wanted a quick patch to have minimal impact on the status quo, i.e. depending on pip's internal API.

For pip >= 10.0.0 (pypa/pip@95bcf8c), the whole implementation of piptools.repository.pypi should be rewritten from scratch anyway, along with appropriate tests.

@suutari-ai
Copy link
Contributor

This should probably be closed now that #557 is merged. Right?

@vphilippon
Copy link
Member

Indeed, thanks for the reminder.

@vphilippon vphilippon closed this Sep 27, 2017
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