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

Python 3 bug in zipfile #26301

Closed
embray opened this issue Sep 17, 2018 · 11 comments
Closed

Python 3 bug in zipfile #26301

embray opened this issue Sep 17, 2018 · 11 comments

Comments

@embray
Copy link
Contributor

embray commented Sep 17, 2018

There's a doctest that invokes this bug in Python 3.6's zipfile module:

sage: from sage.plot.plot3d.shapes import Cylinder
sage: C = Cylinder(3, 1, closed=False)
sage: C.jmol_repr(C.testing_render_params())
Exception ignored in: <bound method ZipFile.__del__ of <zipfile.ZipFile [closed]>>
Traceback (most recent call last):
  File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/zipfile.py", line 1663, in __del__
    self.close()
  File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/zipfile.py", line 1681, in close
    self._write_end_record()
  File "/home/embray/src/sagemath/sage-python3/local/lib/python3.6/zipfile.py", line 1783, in _write_end_record
    centDirSize, centDirOffset, len(self._comment))
struct.error: argument out of range
['pmesh obj_1 "obj_1.pmesh"\ncolor pmesh  [102,102,255]']

The Graphics3d.testing_render_params() opens a ZipFile in write mode to /dev/null, so the bug seems to have to do with writing a zipfile to /dev/null (probably to do with a tell() call somewhere).

For the purposes of testing there's no reason this needs to go to '/dev/null'. It can just as well use a temp file, so we can implement that workaround. But we should also open an upstream bug report for the zipfile problem, if it hasn't already been reported.

Upstream bug report: https://bugs.python.org/issue34904

Upstream: Reported upstream. No feedback yet.

Component: python3

Author: Frédéric Chapoton

Branch/Commit: 60fb5bf

Reviewer: Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/26301

@embray embray added this to the sage-8.4 milestone Sep 17, 2018
@fchapoton
Copy link
Contributor

Author: Frédéric Chapoton

@fchapoton
Copy link
Contributor

Branch: u/chapoton/26301

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

60fb5bftrac 26301 replace dev/null by temporary file

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 4, 2018

Commit: 60fb5bf

@embray
Copy link
Contributor Author

embray commented Oct 5, 2018

comment:3

Sure, looks good to me. I still need to make sure to make an upstream bug report (but this is pretty minor, and after fixing it in Sage I doubt we'll revisit the issue).

@embray
Copy link
Contributor Author

embray commented Oct 5, 2018

comment:4

Interestingly, I can't reproduce the bug on Cygwin, only on Linux. I was trying to see if it was still broken in master, and at first I thought maybe it was fixed. But no, it just doesn't happen on Cygwin that I can tell. Maybe some difference in how /dev/null works; not sure.

@embray
Copy link
Contributor Author

embray commented Oct 5, 2018

Reviewer: Erik Bray

@embray
Copy link
Contributor Author

embray commented Oct 5, 2018

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

@embray
Copy link
Contributor Author

embray commented Oct 5, 2018

comment:5

Ok, upstream bug report submitted. LGTM.

@embray

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Oct 6, 2018

Changed branch from u/chapoton/26301 to 60fb5bf

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

No branches or pull requests

3 participants