Restore Python 2.6+ compatibility

[vbraun]

As of Sage 7.3.beta6, building Sage requires a system Python >= 2.7 because sage-uncompress-spkg uses argparse.

While we already ship a copy of argparse.py, sage-uncompress-spkg currently doesn't try to import it from where it's located in the Sage tree.

Depends on #19984
Depends on #20871

CC: @dimpase @embray @jdemeyer

Component: build

Keywords: argparse sage-uncompress-spkg

Author: Volker Braun

Branch/Commit: 775a3d3

Reviewer: Erik Bray, Leif Leonhardy, Dima Pasechnik

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

[vbraun]

Branch: u/vbraun/sage_requires_python_2_7_

[vbraun]

New commits:

c323a93

Sage requires Python 2.7+

[vbraun]

Commit: c323a93

[vbraun]

Author: Volker Braun

[jhpalmieri]

comment:3

Just a few months ago, people were complaining about a change (fixed in #20252) which prevented Sage from building with Python 2.6. So I think this is a dangerous change to implement: we will lose some of our user base. See discussion in sage-devel. What is the motivation?

[vbraun]

comment:4

The fix has been broken in #20721

I've advocated bundling argparse (which is imho a no-brainer) but the 0.1% size increase was deemed unacceptable in #19984.

[embray]

comment:5

Is use of argparse the only thing that currently breaks compatibility for Python 2.6? If so I think bundling it is a no-brainer--I've done it in a dozen other projects (though most projects are finally droppping support for Python 2.6).

That said we either support Python 2.6 or we don't. If Sage isn't tested against Python 2.6 it doesn't support it as far as anyone should be concerned. Same goes for any other Python version. If it's not tested it's not supported.

[dimpase]

comment:6

Replying to @embray:

Is use of argparse the only thing that currently breaks compatibility for Python 2.6? If so I think bundling it is a no-brainer--I've done it in a dozen other projects (though most projects are finally droppping support for Python 2.6).

That said we either support Python 2.6 or we don't. If Sage isn't tested against Python 2.6 it doesn't support it as far as anyone should be concerned. Same goes for any other Python version. If it's not tested it's not supported.

there is demand for Python 2.6 support. It should be easy to set up a VM with one of these LTS Linux systems which are using 2.6 as the default. The question is who will do this, and on what system. There is spare capacity on the UW cluster, I think, to do this.

[dimpase]

comment:7

So, what should be done to reenable 2.6?

[embray]

comment:8

I guess also I was confused about using Python as a build dependency versus as a runtime dependency. As a runtime dependency only Python 2.7 is supported I guess. But for the build tools 2.6+ makes sense (but should be tested if we're to claim to support it).

[vbraun]

Changed commit from c323a93 to none

[vbraun]

Dependencies: #19984

[vbraun]

Changed branch from u/vbraun/sage_requires_python_2_7_ to none

[vbraun]

comment:10

There is testing via tox for multiple Python versions. Though you need to write tests, obviously. Its not currently tested on the buildbot, thats a todo for the buildbot configuration.

[embray]

comment:11

I'm still a little confused here. What specifically is being tested via tox is there are no tests (and what are we talking about tests for)?

For example, what command(s) are being run by tox? Who's running tox, if not the build bot?

[vbraun]

comment:12

I don't understand what you mean; Tests are in build/tests and you can run them right now via cd build && tox. Developers are supposed to run the tests before making commits, not just the buildbot ;-)

[embray]

comment:13

Ah, I think that's relatively new. Wasn't there a few days ago.

[nexttime]

comment:14

Ping.

Do we need more than, say

diff --git a/build/bin/sage-uncompress-spkg b/build/bin/sage-uncompress-spkg
index 9b72878..e7339ad 100755
--- a/build/bin/sage-uncompress-spkg
+++ b/build/bin/sage-uncompress-spkg
@@ -28,10 +28,22 @@ printing the SPKG.txt file from an old-style spkg.)
 
 from __future__ import print_function
 
-import argparse
-import copy
 import os
 import sys
+try:
+    import argparse
+except ImportError:
+    # Python 2.6 doesn't have it
+    SAGE_ROOT = os.environ.get("SAGE_ROOT")
+    if not SAGE_ROOT:
+        sys.stderr.write(
+            "Error: SAGE_ROOT not set, needed to import Sage's argparse module.\n")
+        sys.exit(1)
+    sys.path.append(os.path.join(SAGE_ROOT, "build", "sage_bootstrap", "compat"))
+    # we could also first check the presence of Sage's argparse.py here
+    import argparse
+    # bail out if that also fails (or suggest something else?)
+import copy
 import tarfile
 import zipfile
 

?

[vbraun]

Author: Volker Braun

[vbraun]

New commits:

775a3d3

Refactor sage-uncompress-spkg, add tests

[vbraun]

Commit: 775a3d3

[nexttime]

comment:32

FWIW, I intentionally did not import the whole sage_bootstrap package just because sage-uncompress-spkg in case of Python 2.6 needs the argparse module (and only that) from .../compat/ in my patch.

---

Could you add a .contains_garbage() property in order to not pass the whole list of all files in a tarball to .extractall() if it returns False (which is the normal case -- it should IMHO error out unless given some option to override that otherwise, or at least give a warning).

As is, it essentially does

tar xf $TARBALL $( tar tf $TARBALL | cat )

or, worse (and more explicit),

gunzip -c $TARBALL | tar xf - $( gunzip -c $TARBALL | tar tf - | cat )

in the normal (non-error) case, where gunzip will be bunzip2 if $TARBALL ends with .bz2 instead of .gz; since you're also building a dictionary, cat here doesn't even properly reflect the no-op processing of all filenames in the archive (it would have to be some more complex identity function).

You could in addition rename .names() to .valid_names() say, and add another function .invalid_names() (or .names() returning all names) or whatever (such that it is possible to obtain the ones filtered out, or at least see whether any files got filtered out, but if we give an error or warning message, that should contain which files are considered garbage).

[vbraun]

comment:33

IHMO adding subdirectories of packages to sys.path is to be avoided as it can cause confusion.

I don't mind beautifying the file handling logic, but that doesn't belong to this ticket.

[nexttime]

comment:34

P.S.: To those unfamiliar with the tar (tape archive) file format:

In contrast to e.g. a zip archive, the tar header does not contain a "table of contents"; to get a list of all files contained in a tarball (or to see whether it contains some file(s)), the whole archive has to get read. (This originates from the sequential access of tapes, and has other advantages.)

[nexttime]

comment:35

Replying to @vbraun:

IHMO adding subdirectories of packages to sys.path is to be avoided as it can cause confusion.

Sure, but I didn't put argparse there, which is unrelated to and independent of all the other stuff there.

I don't mind beautifying the file handling logic, but that doesn't belong to this ticket.

But completely rewriting it (or changing its structure, and adding tests etc.) does?

That's really completely unrelated to Python 2.6 support; if you're referring to the title, all we need here is to pick up the argparse module. And it's currently a blocker ticket.

I'd suggest to put your restructuring etc. on a follow-up, where we can improve other things as well (to not say remove further regressions introduced previously).

If the latter still makes it into Sage 7.3, fine, otherwise it doesn't hurt because the main issue has already been fixed here -- quickly and safe.

[vbraun]

comment:36

Replying to @nexttime:

But completely rewriting it (or changing its structure, and adding tests etc.) does?

Yes, especially adding tests that you can run with tox on a variety of Python versions is IMHO crucially important.

[nexttime]

comment:37

Then also add tests with "garbage" in the archive.

And a warning message is still missing, which is even more important.

[nexttime]

comment:38

P.S.: If we want to continue Python 2.6 support, at least one buildbot should have (and use) it anyway.

[vbraun]

comment:39

Those are good suggestions but not really on scope for this ticket.

[nexttime]

comment:40

Replying to @vbraun:

Those are good suggestions but not really on scope for this ticket.

https://en.wikipedia.org/wiki/Procrastination

[embray]

comment:41

filter_os_files.py seems a bit overly-specialized. I might just call that utils.py. But up to you. LGTM otherwise.

[dimpase]

comment:42

there is [comment:ticket:20870:4] about getting directory permissions right in sage-uncompress-spkg.

Is it indeed the case it is fixed here, or was always fixed anyway?

---

EDIT: Cross-ticket comment reference is [comment:ticket:TTTTT:CCC].

[vbraun]

comment:43

I didn't change anything, but SageTarFile.chmod (which I didn't change) suggests that permissions are sanitized.

[dimpase]

comment:44

what are these errors:

sage -t build/test/capture.py  # ImportError in doctesting framework
...

Are these files meant to be tested in some other way?

[nexttime]

comment:45

Replying to @dimpase:

what are these errors:

sage -t build/test/capture.py  # ImportError in doctesting framework
...

Are these files meant to be tested in some other way?

At least they're not directly tested by ptestlong (which passed for me):

$ ./sage -t --long build/test/
...
----------------------------------------------------------------------
sage -t --long build/test/test_package.py  # ImportError in doctesting framework
sage -t --long build/test/test_is_url.py  # ImportError in doctesting framework
sage -t --long build/test/test_download.py  # ValueError in doctesting framework
sage -t --long build/test/runnable.py  # ImportError in doctesting framework
sage -t --long build/test/test_download_cmdline.py  # ImportError in doctesting framework
sage -t --long build/test/test_config.py  # ImportError in doctesting framework
sage -t --long build/test/capture.py  # ImportError in doctesting framework
sage -t --long build/test/test_mirror_list.py  # ValueError in doctesting framework
sage -t --long build/test/test_cksum.py  # ImportError in doctesting framework
sage -t --long build/test/test_tarball.py  # ImportError in doctesting framework
sage -t --long build/test/test_uncompress.py  # ImportError in doctesting framework
sage -t --long build/test/test_logger.py  # ValueError in doctesting framework
sage -t --long build/test/test_package_cmdline.py  # ImportError in doctesting framework
----------------------------------------------------------------------

[vbraun]

comment:46

No, they aren't meant to be run with the Sage doctest runner. They are meant to execute under tox, which is also ideally suited to run against multiple python versions (see tox.ini):

cd build
tox

[dimpase]

Reviewer: Erik Bray, Leif Leonhardy, Dima Pasechnik

[dimpase]

comment:47

LGTM

[dimpase]

comment:48

PS. Perhaps for another ticket:

py34 runtests: commands[0] | python3.4 -m unittest discover
...../home/dima/software/sage/build/sage_bootstrap/download/transfer.py:123: DeprecationWarning: FancyURLopener style of invoking requests is deprecated. Use newer urlopen functions/methods
  opener = urllib.FancyURLopener()

[nexttime]

comment:49

OMG, fancy is deprecated?!

[vbraun]

Changed branch from u/vbraun/restore_python_2_6__compatibility to 775a3d3