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.

Sign up for GitHub

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

Jump to bottom

bpo-31291: fix an assertion failure in zipimport.zipimporter.get_data() #3226

Merged
merged 3 commits into from
Aug 29, 2017
Merged

bpo-31291: fix an assertion failure in zipimport.zipimporter.get_data() #3226

merged 3 commits into from
Aug 29, 2017

Conversation

orenmn
Copy link
Contributor

@orenmn orenmn commented Aug 28, 2017 •
edited by bedevere-bot
Loading

  • in zipimport.c: replace the C equivalent of pathname.replace('/', '\\') with
    str.replace(pathname, '/', '\\').
  • in test_zipimport.py: add a test to verify the assertion failure is no more.

https://bugs.python.org/issue31291

@orenmn orenmn requested a review from a team August 28, 2017 09:22
orenmn
orenmn commented Aug 28, 2017
View reviewed changes
Modules/zipimport.c
@@ -651,7 +651,8 @@ zipimport_zipimporter_get_data_impl(ZipImporter *self, PyObject *path)
Py_ssize_t path_start, path_len, len;

#ifdef ALTSEP
path = _PyObject_CallMethodId(path, &PyId_replace, "CC", ALTSEP, SEP);
path = _PyObject_CallMethodId((PyObject *)&PyUnicode_Type, &PyId_replace,
"OCC", path, ALTSEP, SEP);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about this.
would it be better to simply check whether the return value of pathname.replace('/', '\') is a str?

Copy link
Member

Choose a reason for hiding this comment

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

The current solution LGTM.

@brettcannon brettcannon mentioned this pull request Aug 28, 2017
Closed
@brettcannon
Copy link
Member

I just rejected the issue, so closing the PR as well. Any discussion about the closure should happen on the issue.

serhiy-storchaka
serhiy-storchaka reviewed Aug 29, 2017
View reviewed changes
Misc/NEWS.d/next/Core and Builtins/2017-08-28-11-51-29.bpo-31291.t8QggK.rst Outdated
@@ -0,0 +1,3 @@
Fix an assertion failure in `zipimport.zipimporter.get_data` on Windows,
when the return value of pathname.replace('/','\\') isn't a string. Patch by
Copy link
Member

Choose a reason for hiding this comment

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

Format pathname.replace('/','\\') as a literal code (surround it with double backquotes). Otherwise \\ will be interpreted in reST.

Lib/test/test_zipimport.py Outdated
@@ -517,6 +517,12 @@ def testGetData(self):
zi = zipimport.zipimporter(TEMP_ZIP)
self.assertEqual(data, zi.get_data(name))
self.assertIn('zipimporter object', repr(zi))
# Issue #31291: There shouldn't be an assertion failure in
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to add a separate method for this test.

Modules/zipimport.c
@@ -651,7 +651,8 @@ zipimport_zipimporter_get_data_impl(ZipImporter *self, PyObject *path)
Py_ssize_t path_start, path_len, len;

#ifdef ALTSEP
path = _PyObject_CallMethodId(path, &PyId_replace, "CC", ALTSEP, SEP);
path = _PyObject_CallMethodId((PyObject *)&PyUnicode_Type, &PyId_replace,
"OCC", path, ALTSEP, SEP);
Copy link
Member

Choose a reason for hiding this comment

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

The current solution LGTM.

@serhiy-storchaka serhiy-storchaka added needs backport to 3.6 type-bug An unexpected behavior, bug, or error labels Aug 29, 2017
brettcannon
brettcannon approved these changes Aug 29, 2017
View reviewed changes
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Serhiy's arguments on the issue tracker convinced me (plus I think he knows this module better than I do now 😉 ).

@brettcannon
Copy link
Member

BTW, I don't know if I will have time to commit this between now and taking holiday, but if no one merges this by September 15, @orenmn , just leave a comment messaging me.

@serhiy-storchaka serhiy-storchaka merged commit 631fdee into python:master Aug 29, 2017
orenmn added a commit to orenmn/cpython that referenced this pull request Aug 30, 2017
@orenmn
[3.6] bpo-31291: Fixed an assertion failure in zipimport.zipimporter.…
8547b2d 
…get_data() (pythonGH-3226)

if pathname.replace('/', '\\') returns non-string.
(cherry picked from commit 631fdee)
@orenmn orenmn mentioned this pull request Aug 30, 2017
Merged
@bedevere-bot
Copy link

GH-3243 is a backport of this pull request to the 3.6 branch.

serhiy-storchaka pushed a commit that referenced this pull request Aug 30, 2017
@orenmn @serhiy-storchaka
[3.6] bpo-31291: Fixed an assertion failure in zipimport.zipimporter.…
095a421 
…get_data() (GH-3226) (#3243)

if pathname.replace('/', '\\') returns non-string.
(cherry picked from commit 631fdee)
GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017
@orenmn @GadgetSteve
bpo-31291: Fixed an assertion failure in zipimport.zipimporter.get_da…
dc4e207 
…ta() (python#3226)

if pathname.replace('/', '\\') returns non-string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brettcannon brettcannon brettcannon approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@orenmn @brettcannon @bedevere-bot @serhiy-storchaka @Mariatta @the-knights-who-say-ni