Python library to bootstrap Sage #18748

vbraun · 2015-06-20T13:09:28Z

Move the logic behind package handling and I/O for Python scripts into a separate Python library for code reuse and automated testing.

CC: @nathanncohen

Component: build

Author: Volker Braun

Branch/Commit: 60a581b

Reviewer: John Palmieri

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

The text was updated successfully, but these errors were encountered:

vbraun · 2015-06-20T20:06:21Z

Branch: u/vbraun/python_library_to_bootstrap_sage

sagetrac-git · 2015-06-21T10:51:41Z

Commit: 0acf070

sagetrac-git · 2015-06-21T10:51:41Z

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

`bd61b67`	`add the sage-pkg entrypoint`
`25022c3`	`move sage-download-file to the sage_bootstrap library`
`0acf070`	`initial implementation of the sage-pkg tool`

vbraun · 2015-06-21T10:57:34Z

comment:5

Now with testing:

$ tox
[...]
  py26: commands succeeded
  py27: commands succeeded
  py33: commands succeeded
  py34: commands succeeded
  congratulations :)

vbraun · 2015-06-21T10:57:34Z

Author: Volker Braun

nathanncohen · 2015-06-21T11:00:22Z

comment:6

Can you explain in detail what you do in this branch? You make many changes, this ticket is now a blocker, and it takes a LONG time to figure it all out from looking at the diff.

vbraun · 2015-06-21T11:07:32Z

comment:7

No actual functionality changed
sage-download-file code is refactored into a package
Tests added for everything
Output is explained in the README, no more if self.verbose: printflush hacks
sage-pkg command to translate between package names and tarball filenames (and maybe query for package types in the future)

jdemeyer · 2015-06-25T08:27:17Z

comment:8

sage-pkg was the command to create old-style packages, now it's something completely different. Since we still support old-style packages, can't you rename this script?

Also: why is this a blocker? There is nothing in the description or comments justifying that.

jdemeyer · 2015-06-25T08:31:56Z

comment:9

About the directory structure: wouldn't it be simpler to keep the scripts in src/bin (they can still import sage_bootstrap) and then not use the redundant sage_bootstrap/sage_bootstrap subdirectory?

jdemeyer · 2015-06-25T08:49:37Z

comment:10

I honestly have no idea what tox does, but I tried it anyway:

jdemeyer@tamiyo:/usr/local/src/sage-config/src/sage_bootstrap$ uname -a
Linux tamiyo 3.17.7-gentoo #1 SMP PREEMPT Wed Dec 31 20:06:39 CET 2014 x86_64 Intel(R) Core(TM) i7-2640M CPU @ 2.80GHz GenuineIntel GNU/Linux

jdemeyer@tamiyo:/usr/local/src/sage-config/src/sage_bootstrap$ python --version
Python 3.3.2

jdemeyer@tamiyo:/usr/local/src/sage-config/src/sage_bootstrap$ tox --version
1.8.1 imported from /usr/lib64/python3.3/site-packages/tox/__init__.py

jdemeyer@tamiyo:/usr/local/src/sage-config/src/sage_bootstrap$ tox
GLOB sdist-make: /usr/local/src/sage-config/src/sage_bootstrap/setup.py
py26 create: /usr/local/src/sage-config/src/sage_bootstrap/.tox/py26
ERROR: InterpreterNotFound: python2.6
py27 inst-nodeps: /usr/local/src/sage-config/src/sage_bootstrap/.tox/dist/sage_bootstrap-1.0.zip
py27 runtests: PYTHONHASHSEED='1210919353'
py27 runtests: commands[0] | python2.7 -m unittest discover
.......E.......
======================================================================
ERROR: test_checksum (test.test_tarball.TarballTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/src/sage-config/src/sage_bootstrap/test/test_tarball.py", line 40, in test_checksum
    self.assertTrue(tarball.checksum_verifies())
  File "/usr/local/src/sage-config/src/sage_bootstrap/sage_bootstrap/tarball.py", line 83, in checksum_verifies
    sha1 = self._compute_sha1()
  File "/usr/local/src/sage-config/src/sage_bootstrap/sage_bootstrap/tarball.py", line 73, in _compute_sha1
    return self._compute_hash(hashlib.sha1())
  File "/usr/local/src/sage-config/src/sage_bootstrap/sage_bootstrap/tarball.py", line 63, in _compute_hash
    with open(self.upstream_fqn, 'rb') as f:
IOError: [Errno 2] No such file or directory: '/usr/local/src/sage-config/upstream/configure-99.tar.gz'

----------------------------------------------------------------------
Ran 15 tests in 6.914s

FAILED (errors=1)
ERROR: InvocationError: '/usr/local/src/sage-config/src/sage_bootstrap/.tox/py27/bin/python2.7 -m unittest discover'
py33 inst-nodeps: /usr/local/src/sage-config/src/sage_bootstrap/.tox/dist/sage_bootstrap-1.0.zip
py33 runtests: PYTHONHASHSEED='1210919353'
py33 runtests: commands[0] | python3.3 -m unittest discover
..../usr/local/src/sage-config/src/sage_bootstrap/sage_bootstrap/download.py:121: DeprecationWarning: FancyURLopener style of invoking requests is deprecated. Use newer urlopen functions/methods
  opener = urllib.FancyURLopener()
...........
----------------------------------------------------------------------
Ran 15 tests in 0.741s

OK
py34 create: /usr/local/src/sage-config/src/sage_bootstrap/.tox/py34
ERROR: InterpreterNotFound: python3.4
_______________________________________________________________________________ summary ________________________________________________________________________________
SKIPPED:  py26: InterpreterNotFound: python2.6
ERROR:   py27: commands failed
  py33: commands succeeded
SKIPPED:  py34: InterpreterNotFound: python3.4

Also, I'm a bit confused by

ERROR: InterpreterNotFound: python3.4

followed later by

SKIPPED:  py34: InterpreterNotFound: python3.4

jdemeyer · 2015-06-25T08:50:26Z

comment:11

What would it take to support Python <= 2.5 or Python <= 3.2? I don't know how common these are, but if they can be supported with minimal effort, perhaps it should be done.

vbraun · 2015-06-25T09:17:06Z

comment:12

I don't think there is any currently supported OS that ships with those python versions, so its most likely a waste of time to try.

vbraun · 2015-06-25T09:19:03Z

comment:13

Blocker because it fixes the firewall issue from #18723 and that ticket was a blocker.

vbraun · 2015-06-25T09:25:31Z

comment:14

I don't mind renaming the script, but I don't see the point of avoiding name collisions with historical Sage versions. My plan is to export all new-style package management functions via sage-pkg so its the obvious name for that.

tox (which is pretty much the standard for cross python version testing) is configured to not fail if a python version is not found (see tox.ini)

scripts in src/bin end up being installed in SAGE_LOCAL (and globally if Sage is every packaged / packageable). Build tools shouldn't be globally installed. Really sage-bootstrap only makes sense if you have the source lying around, so why pretend otherwise.

jdemeyer · 2015-06-25T09:32:36Z

comment:15

Replying to @vbraun:

I don't mind renaming the script, but I don't see the point of avoiding name collisions with historical Sage versions.

It's a name collision with current Sage versions. And sage --pkg is still currently documented in http://doc.sagemath.org/html/en/developer/packaging_old_spkgs.html#the-file-spkg-txt

tox (which is pretty much the standard for cross python version testing) is configured to not fail if a python version is not found (see tox.ini)

Sure, but it is confusing to see ERROR: InterpreterNotFound: python3.4 if it's not really an error. But if that's the way how tox works, so be it.

jdemeyer · 2015-06-25T09:36:42Z

comment:16

Replying to @vbraun:

Blocker because it fixes the firewall issue from #18723 and that ticket was a blocker.

I just don't like if blockers are complicated tickets like this. Why not apply the simple fix of #18723 as blocker and make #18748 a normal ticket?

vbraun · 2015-06-25T09:41:46Z

comment:17

THis is a rather simple ticket IMHO, it just refactors a script and adds testing.

jdemeyer · 2015-06-25T09:43:08Z

comment:18

Wouldn't it be a lot better to move sage_bootstrap to the $SAGE_ROOT/build directory? That's where the Sage packages reside, so it would make sense to move the package build system also to build.

jdemeyer · 2015-06-25T09:44:48Z

comment:19

Replying to @vbraun:

THis is a rather simple ticket

Really?

28 files changed, 1518 insertions, 460 deletions

jdemeyer · 2015-06-25T09:47:14Z

comment:20

You have inconsistent indentation (not multiples of 4 spaces) in src/sage_bootstrap/sage_bootstrap/stdio.py

vbraun · 2015-07-05T21:25:54Z

comment:42

https://tox.readthedocs.org

The whole point of the refactoring is to make stuff testable

jhpalmieri · 2015-07-06T03:06:37Z

comment:43

Why are some of the files not listed in MANIFEST (e.g., tox.ini, test/capture.py)?

vbraun · 2015-07-06T07:22:36Z

comment:44

The manifest is auto-generated by tox/distutils to build a package for testing purposes. Its contents don't really matter apart from that. The tox config file arguably doesn't belong there, so I'd count that as a plus. If we were to use the manifest for anything then we would have to add a Manifest.in to fine-tune the inclusion rules.

jhpalmieri · 2015-07-07T00:03:31Z

comment:45

Minor questions:

In package.py, are the lines

    def __eq__(self, other):
        return self.tarball == other.tarball

there just for testing via tox, or do they serve another purpose?

There are some blank spaces on several otherwise empty lines in tarball.py. Do we care?

Overall, the code is just a relocation of old code, so that's okay. I'm not sure about the whole idea of moving it. Seems okay to me, but it doesn't strike me as very high on the priority list.

Less minor questions and comments:

What does sage-package do? The file itself should contain some information; you shouldn't have to run it to get its usage message. Maybe the same could be said for sage-download-file, but at least the name is somewhat self-explanatory. I think you should add a line or two there, also. Probably in README, too, for example after the sentence

This is a utility libary for dealing with third-party tarballs and
building Sage.

(Something like "The main entry points are sage-download-file and sage-package, which are used as follows: ...". Note also that "libary" is currently misspelled.)

In addition, I think that each python file in sage_bootstrap could have a bit more documentation, illustrating how to use it. I don't really want to have to read the source code to figure out this sort of thing. For download.py, for example, mention Download(...).run().
Add the link https://tox.readthedocs.org to README. Maybe we should all know about tox already, but that doesn't seem to be the case.

vbraun · 2015-07-07T07:36:28Z

comment:46

Replying to @jhpalmieri:

In package.py, are the lines
    def __eq__(self, other):
        return self.tarball == other.tarball
there just for testing via tox, or do they serve another purpose?

They are used for testing in unittest.TestCase.assertEqual, doesn't really have anything to do with tox (which is just the test runner) besides that.

There are some blank spaces on several otherwise empty lines in tarball.py. Do we care?

I don't.

Overall, the code is just a relocation of old code, so that's okay. I'm not sure about the whole idea of moving it. Seems okay to me, but it doesn't strike me as very high on the priority list.

The old script was already too long to fit comfortably into one file, with added testing its can't fit into one file. So it has to grow into a package.

Less minor questions and comments:

What does sage-package do?

It mostly makes the release manager's job easier because you don't have to log into the server and associated tarball names to package names by hand, which wastes my preciously little available time.

I'm happy to add a line of documentation here or there but unlike the math part this is not a library for Python novices. IMHO its not too much to ask to google for "tox", thats bound to produce way more relevant and informative information than I can possibly be.

jhpalmieri · 2015-07-07T17:46:45Z

Changed branch from u/vbraun/python_library_to_bootstrap_sage to u/jhpalmieri/python_library_to_bootstrap_sage

jhpalmieri · 2015-07-07T17:48:27Z

comment:48

Replying to @vbraun:

I'm happy to add a line of documentation here or there but unlike the math part this is not a library for Python novices. IMHO its not too much to ask to google for "tox", thats bound to produce way more relevant and informative information than I can possibly be.

Here is some documentation for the main scripts. I think that few of the Python files could use a bit more information: the current descriptions for package.py and maybe tarball.py are too terse.

jhpalmieri · 2015-07-07T17:48:27Z

Changed branch from u/jhpalmieri/python_library_to_bootstrap_sage to u/vbraun/python_library_to_bootstrap_sage

jhpalmieri · 2015-07-07T17:49:21Z

New commits:

`9e2c568`	`Merge branch 'develop' into t/18748/python_library_to_bootstrap_sage`
`cc213dc`	`trac 18748: add documentation to scripts in sage_bootstrap/bin`

jhpalmieri · 2015-07-07T17:49:21Z

Changed branch from u/vbraun/python_library_to_bootstrap_sage to u/jhpalmieri/python_library_to_bootstrap_sage

jhpalmieri · 2015-07-07T17:49:21Z

Changed commit from e45a0b6 to cc213dc

vbraun · 2015-07-12T13:24:37Z

Changed branch from u/jhpalmieri/python_library_to_bootstrap_sage to u/vbraun/python_library_to_bootstrap_sage

sagetrac-git · 2015-07-12T14:27:57Z

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

`8f4fb8e`	`More docstrings`
`f9a69a5`	`Convert public attributes to properties and document`
`075228f`	`Make package.tarball a Tarball instance`

sagetrac-git · 2015-07-12T14:27:57Z

Changed commit from cc213dc to 075228f

vbraun · 2015-07-12T14:28:50Z

comment:52

Done, still needs review

jhpalmieri · 2015-07-12T16:10:02Z

comment:53

I get an error with tox:

$ tox 
GLOB sdist-make: /Users/palmieri/Desktop/Sage_stuff/git/sage/src/sage_bootstrap/setup.py
py26 create: /Users/palmieri/Desktop/Sage_stuff/git/sage/src/sage_bootstrap/.tox/py26
py26 installdeps: unittest2
py26 inst: /Users/palmieri/Desktop/Sage_stuff/git/sage/src/sage_bootstrap/.tox/dist/sage_bootstrap-1.0.zip
py26 installed: argparse==1.3.0,linecache2==1.0.0,sage-bootstrap==1.0,six==1.9.0,traceback2==1.4.0,unittest2==1.1.0,wheel==0.24.0
py26 runtests: PYTHONHASHSEED='2708481092'
py26 runtests: commands[0] | unit2 discover
............E..
======================================================================
ERROR: test_package (test.test_package.PackageTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/palmieri/Desktop/Sage_stuff/git/sage/src/sage_bootstrap/test/test_package.py", line 34, in test_package
    self.assertIsInstance(pkg.tarball, Tarball)
AttributeError: 'PackageTestCase' object has no attribute 'assertIsInstance'

----------------------------------------------------------------------
Ran 15 tests in 1.056s

FAILED (errors=1)
ERROR: InvocationError: '/Users/palmieri/Desktop/Sage_stuff/git/sage/src/sage_bootstrap/.tox/py26/bin/unit2 discover'

sagetrac-git · 2015-07-12T17:26:25Z

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

`c56991b`	`Fix Python 2.6 test error`

sagetrac-git · 2015-07-12T17:26:25Z

Changed commit from 075228f to c56991b

vbraun · 2015-07-12T17:27:01Z

comment:55

Thanks, ``assertIsInstance` is Python 2.7+, fixed

jhpalmieri · 2015-07-13T04:23:54Z

Changed branch from u/vbraun/python_library_to_bootstrap_sage to u/jhpalmieri/python_library_to_bootstrap_sage

jhpalmieri · 2015-07-13T04:25:33Z

Changed commit from c56991b to 60a581b

jhpalmieri · 2015-07-13T04:25:33Z

Reviewer: John Palmieri

jhpalmieri · 2015-07-13T04:25:33Z

comment:57

Okay, one more addition to the documentation. If you're happy with this, you can set this to positive review.

vbraun · 2015-07-13T17:58:42Z

Changed branch from u/jhpalmieri/python_library_to_bootstrap_sage to 60a581b

vbraun added this to the sage-6.8 milestone Jun 20, 2015

vbraun added the p: major / 3 label Jun 20, 2015

This comment has been minimized.

Sign in to view

vbraun added c: build labels Jun 20, 2015

vbraun added s: needs review and removed p: major / 3 labels Jun 21, 2015

jdemeyer added s: needs work and removed s: needs review labels Jun 25, 2015

vbraun added s: positive review and removed s: needs review labels Jul 13, 2015

vbraun removed the s: positive review label Jul 13, 2015

vbraun closed this as completed in 6810836 Jul 13, 2015

vbraun mentioned this issue Jul 15, 2015

Reorganize /build #18788

Closed

jdemeyer mentioned this issue Oct 14, 2015

Apply #18731 again #19349

Closed

Python library to bootstrap Sage #18748

Python library to bootstrap Sage #18748

Comments

vbraun commented Jun 20, 2015

This comment has been minimized.

vbraun commented Jun 20, 2015

sagetrac-git mannequin commented Jun 21, 2015

sagetrac-git mannequin commented Jun 21, 2015

vbraun commented Jun 21, 2015

vbraun commented Jun 21, 2015

nathanncohen mannequin commented Jun 21, 2015

vbraun commented Jun 21, 2015

jdemeyer commented Jun 25, 2015

jdemeyer commented Jun 25, 2015

jdemeyer commented Jun 25, 2015

jdemeyer commented Jun 25, 2015

vbraun commented Jun 25, 2015

vbraun commented Jun 25, 2015

vbraun commented Jun 25, 2015

jdemeyer commented Jun 25, 2015

jdemeyer commented Jun 25, 2015

vbraun commented Jun 25, 2015

jdemeyer commented Jun 25, 2015

jdemeyer commented Jun 25, 2015

jdemeyer commented Jun 25, 2015

vbraun commented Jul 5, 2015

jhpalmieri commented Jul 6, 2015

vbraun commented Jul 6, 2015

jhpalmieri commented Jul 7, 2015

vbraun commented Jul 7, 2015

jhpalmieri commented Jul 7, 2015

jhpalmieri commented Jul 7, 2015

jhpalmieri commented Jul 7, 2015

jhpalmieri commented Jul 7, 2015

jhpalmieri commented Jul 7, 2015

jhpalmieri commented Jul 7, 2015

vbraun commented Jul 12, 2015

sagetrac-git mannequin commented Jul 12, 2015

sagetrac-git mannequin commented Jul 12, 2015

vbraun commented Jul 12, 2015

jhpalmieri commented Jul 12, 2015

sagetrac-git mannequin commented Jul 12, 2015

sagetrac-git mannequin commented Jul 12, 2015

vbraun commented Jul 12, 2015

jhpalmieri commented Jul 13, 2015

jhpalmieri commented Jul 13, 2015

jhpalmieri commented Jul 13, 2015

jhpalmieri commented Jul 13, 2015

vbraun commented Jul 13, 2015