GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-25711: Rewrite zipimport in pure Python. by serhiy-storchaka · Pull Request #6809 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented May 14, 2018 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue25711

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zware, alex, and gst reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement DO-NOT-MERGE labels May 14, 2018
@serhiy-storchaka
serhiy-storchaka requested a review from a team May 14, 2018 16:20
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 14, 2018
@bedevere-bot bedevere-bot added the awaiting merge label May 14, 2018
@gpshead
gpshead requested review from Yhg1s and brettcannon May 14, 2018 16:26
serhiy-storchaka and others added 2 commits July 8, 2018 10:33
@serhiy-storchaka
bpo-25711: Rewrite zipimport in pure Python.
c090211
@gpshead @serhiy-storchaka
typo
fb93cdd
@serhiy-storchaka
serhiy-storchaka force-pushed the zipimport branch from fcf8f37 to fb93cdd Compare July 8, 2018 07:38
serhiy-storchaka added 4 commits July 8, 2018 13:08
@serhiy-storchaka
Update Python/importlib_zipimport.h.
561e14a
@serhiy-storchaka
Fix importlib.resources.
8d3e66d
@serhiy-storchaka
Fix tests.
c8a2cde
@serhiy-storchaka
Remove zipimport.c from VC project.
0265548
@serhiy-storchaka
serhiy-storchaka requested a review from a team as a code owner July 8, 2018 17:21
@serhiy-storchaka
Remove references to PyInit_zipimport().
9269d84
brettcannon
brettcannon reviewed Aug 17, 2018
View reviewed changes
Comment thread Lib/zipimport.py Outdated

path = _get_module_path(self, fullname)
if mi:
fullpath = path + path_sep + '__init__.py'

@brettcannon brettcannon Aug 17, 2018

Copy link
Copy Markdown
Member

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.

bootstrap_external._path_join()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Aug 17, 2018

Copy link
Copy Markdown
Member Author

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.

What is the benefit of using bootstrap_external._path_join()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brettcannon brettcannon Aug 24, 2018

Copy link
Copy Markdown
Member

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.

Readability. When I read path + path_sep + '__init__.py' I have to mentally piece together that this is constructing a file path, while _path_join(path, '__init__.py)` gives me that context implicitly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py Outdated
if mi:
fullpath = path + path_sep + '__init__.py'
else:
fullpath = path + '.py'

@brettcannon brettcannon Aug 17, 2018

Copy link
Copy Markdown
Member

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.

Might as well use an f-string.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Aug 17, 2018

Copy link
Copy Markdown
Member Author

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.

What is the benefit of using an f-string instead of + for concatenating two strings?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brettcannon brettcannon Aug 24, 2018

Copy link
Copy Markdown
Member

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.

I find it more readable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py Outdated
# add __path__ to the module *before* the code gets
# executed
path = _get_module_path(self, fullname)
fullpath = f'{self.archive}{path_sep}{path}'

@brettcannon brettcannon Aug 17, 2018

Copy link
Copy Markdown
Member

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.

_bootstrap_external._path_join()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py Outdated

# implementation

def _unpack_uint32(data):

@brettcannon brettcannon Aug 17, 2018

Copy link
Copy Markdown
Member

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.

_bootstrap_external._r_long()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Aug 17, 2018

Copy link
Copy Markdown
Member Author

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.

What to do with _unpack_uint16? _unpack_uint32 looks to me more descriptive than _r_long.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brettcannon brettcannon Aug 24, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

You can keep _unpack_uint16, I just don't think we need to have two implementations to do the exact same thing of reading in the bytes of a little-endian 4 bytes int. If you don't like the name then feel free to rename it in importlib.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py Outdated
name = name.decode('latin1').translate(cp437_table)

name = name.replace('/', path_sep)
path = f'{archive}{path_sep}{name}'

@brettcannon brettcannon Aug 17, 2018

Copy link
Copy Markdown
Member

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.

bootstrap_external._path_join()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@brettcannon
brettcannon requested a review from warsaw August 17, 2018 18:03
serhiy-storchaka added 3 commits August 17, 2018 22:29
@serhiy-storchaka
Merge branch 'master' into zipimport
d34d754
@serhiy-storchaka
Try to fix a build on Windows.
a683768
@serhiy-storchaka
Fix tests on Windows.
3928725
berkerpeksag
berkerpeksag reviewed Aug 19, 2018
View reviewed changes
Comment thread Lib/zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/zipimport.py Outdated

def _unpack_uint16(data):
"""Convert 2 bytes in little-endian to an integer."""
assert len(data) == 2

@berkerpeksag berkerpeksag Aug 19, 2018

Copy link
Copy Markdown
Member

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.

Nit: Wouldn't be better if we raised ValueError here and in _unpack_uint32()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Aug 19, 2018

Copy link
Copy Markdown
Member Author

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.

In all cases the length of the data is checked before calling _unpack_uint*(). The assertion condition is always true.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
serhiy-storchaka added 2 commits August 25, 2018 09:21
@serhiy-storchaka
Merge branch 'master' into zipimport
3ea8986
@serhiy-storchaka
Address Brett's comments.
6a78b4c
brettcannon
brettcannon approved these changes Aug 31, 2018
View reviewed changes
warsaw
warsaw reviewed Sep 11, 2018
View reviewed changes

@warsaw warsaw left a comment

Copy link
Copy Markdown
Member

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.

Comments just based on reading the code. I'm going to test this branch with importlib_resources (standalone) next.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_bdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/importlib/_bootstrap_external.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/zipimport.py Outdated
@@ -0,0 +1,640 @@
'''zipimport provides support for importing Python modules from Zip archives.

@warsaw warsaw Sep 11, 2018

Copy link
Copy Markdown
Member

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.

Style nit: this should be a """ string.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Sep 11, 2018

Copy link
Copy Markdown
Member Author

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.

Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/zipimport.py Outdated
try:
raw_data = fp.read(data_size)
except OSError:
raise OSError("zipimport: can't read data")

@warsaw warsaw Sep 11, 2018

Copy link
Copy Markdown
Member

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.

Is this just here to change the error message? I guess that's for compatibility with the message raised by the C implementation? I'm not sure how useful that is, and it does kind of hide the original exception (which might be useful for debugging). My inclination is to preserve the original OSError rather than produce one with a different message, but if you feel otherwise, why not use a raise from here too?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Sep 11, 2018

Copy link
Copy Markdown
Member Author

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.

Removed the try block.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py
try:
decompress = _get_decompress_func()
except:
raise ZipImportError("can't decompress data; zlib not available")

@warsaw warsaw Sep 11, 2018

Copy link
Copy Markdown
Member

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.

except Exception and raise from

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Sep 11, 2018

Copy link
Copy Markdown
Member Author

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.

Added except Exception.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
warsaw
warsaw requested changes Sep 11, 2018
View reviewed changes

@warsaw warsaw left a comment

Copy link
Copy Markdown
Member

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.

The branch does work with importlib_resources AFAICT.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Sep 11, 2018

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Sep 11, 2018
@serhiy-storchaka
Merge branch 'master' into zipimport
14c8601
@python python deleted a comment from warsaw Sep 11, 2018
@serhiy-storchaka
Addressed commit reviews.
5fed013
serhiy-storchaka
serhiy-storchaka commented Sep 11, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member Author

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.

Thank you for your review @warsaw. I have addressed most of your comments. But AFAIK from error is not needed in the Python code. Exceptions is chained by default. You need to add from None for suppressing chaining.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/importlib/_bootstrap_external.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_bdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/zipimport.py Outdated
'PQRSTUVWXYZ[\\]^_'
'`abcdefghijklmno'
'pqrstuvwxyz{|}~\x7f'

@serhiy-storchaka serhiy-storchaka Sep 11, 2018

Copy link
Copy Markdown
Member Author

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.

Just for readability. The above part is ASCII, and 16 chars per row, the below part is non-ASCII, and 8 chars per row. What comment should I add?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py
from zlib import decompress
except:
_bootstrap._verbose_message('zipimport: zlib UNAVAILABLE')
raise ZipImportError("can't decompress data; zlib not available")

@serhiy-storchaka serhiy-storchaka Sep 11, 2018

Copy link
Copy Markdown
Member Author

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.

The C implementation catches all exceptions. I agree, that this is evil and will change this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py Outdated
try:
raw_data = fp.read(data_size)
except OSError:
raise OSError("zipimport: can't read data")

@serhiy-storchaka serhiy-storchaka Sep 11, 2018

Copy link
Copy Markdown
Member Author

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.

Removed the try block.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py
try:
decompress = _get_decompress_func()
except:
raise ZipImportError("can't decompress data; zlib not available")

@serhiy-storchaka serhiy-storchaka Sep 11, 2018

Copy link
Copy Markdown
Member Author

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.

Added except Exception.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka added 2 commits September 17, 2018 13:53
@serhiy-storchaka
Merge branch 'master' into zipimport
3984d25
@serhiy-storchaka
Add additional comments for the cp437 table.
33fa5a4
serhiy-storchaka
serhiy-storchaka commented Sep 17, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member Author

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.

I prefer to make behavior changing clean up and remove unneeded tests in separate PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/zipimport.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/zipimport.py
try:
toc_entry = self._files[key]
except KeyError:
raise OSError(0, '', key)

@serhiy-storchaka serhiy-storchaka Sep 17, 2018

Copy link
Copy Markdown
Member Author

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.

For raising a proper FileNotFoundError you need to specify the corresponding errno.

import errno
raise FileNotFoundError(errno.ENOENT, 'No such file or directory', key)

(and maybe using pathname is more proper than key).

I prefer to defer this change to later. It is nontrivial and may need additional discussion and tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
warsaw
warsaw requested changes Sep 17, 2018
View reviewed changes

@warsaw warsaw left a comment

Copy link
Copy Markdown
Member

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.

I think we're down to one issue/question for this branch, a merge conflict, and deferring cleanups to a follow on branch.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
Merge branch 'master' into zipimport
ec110a0
warsaw
warsaw approved these changes Sep 18, 2018
View reviewed changes

@warsaw warsaw left a comment

Copy link
Copy Markdown
Member

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 for doing this - I think it will lead to great improvements in zipimport in the future. My only suggestion would be to open a bpo issue for the subsequent clean up branch, since there are things that we know can be improved on the next pass. I think this is a good change to land.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Sep 18, 2018
@serhiy-storchaka
serhiy-storchaka merged commit 79d1c2e into python:master Sep 18, 2018
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 18, 2018
@serhiy-storchaka
serhiy-storchaka deleted the zipimport branch September 18, 2018 19:22
@serhiy-storchaka

serhiy-storchaka commented Sep 18, 2018 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Thank you for your review @warsaw. I have created two following PRs (#9404 and #9406) and one issue (bpo-34726). Will open more. I still don't know what to do with exceptions.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tirkarthi tirkarthi mentioned this pull request May 14, 2019
Reference zipimport source code from docs #13310
Merged
@marcelotduarte marcelotduarte mentioned this pull request Mar 1, 2020
correctly set __package__ in a module in a zip file marcelotduarte/cx_Freeze#609
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag left review comments
@benjaminp benjaminp benjaminp left review comments
@brettcannon brettcannon brettcannon approved these changes
@warsaw warsaw warsaw approved these changes
@Yhg1s Yhg1s Awaiting requested review from Yhg1s

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

DO-NOT-MERGE type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@serhiy-storchaka @bedevere-bot @berkerpeksag @brettcannon @warsaw @benjaminp @the-knights-who-say-ni @gpshead
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.