Opened 4 years ago

Closed 4 years ago

#18748 closed defect (fixed)

Python library to bootstrap Sage

Reported by: vbraun Owned by:
Priority: blocker Milestone: sage-6.8
Component: build Keywords:
Cc: ncohen Merged in:
Authors: Volker Braun Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: 60a581b (Commits) Commit: 60a581b42aa98a233d8b6c5a66487586f5619826
Dependencies: Stopgaps:

Description (last modified by vbraun)

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

Change History (59)

comment:1 Changed 4 years ago by vbraun

  • Component changed from PLEASE CHANGE to build
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 4 years ago by ncohen

  • Cc ncohen added

comment:3 Changed 4 years ago by vbraun

  • Branch set to u/vbraun/python_library_to_bootstrap_sage

comment:4 Changed 4 years ago by git

  • Commit set to 0acf070e6d86df8f5dcc62f0751002631cbce9d8

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

bd61b67add the sage-pkg entrypoint
25022c3move sage-download-file to the sage_bootstrap library
0acf070initial implementation of the sage-pkg tool

comment:5 Changed 4 years ago by vbraun

  • Authors set to Volker Braun
  • Priority changed from major to blocker
  • Status changed from new to needs_review

Now with testing:

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

comment:6 Changed 4 years ago by ncohen

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.

comment:7 Changed 4 years ago by vbraun

  • 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)

comment:8 Changed 4 years ago by jdemeyer

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.

comment:9 Changed 4 years ago by jdemeyer

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?

comment:10 Changed 4 years ago by jdemeyer

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

comment:11 Changed 4 years ago by jdemeyer

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.

comment:12 Changed 4 years ago by vbraun

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.

comment:13 follow-up: Changed 4 years ago by vbraun

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

comment:14 follow-up: Changed 4 years ago by vbraun

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.

comment:15 in reply to: ↑ 14 Changed 4 years ago by jdemeyer

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.

comment:16 in reply to: ↑ 13 Changed 4 years ago by jdemeyer

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?

comment:17 follow-up: Changed 4 years ago by vbraun

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

comment:18 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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.

comment:19 in reply to: ↑ 17 Changed 4 years ago by jdemeyer

Replying to vbraun:

THis is a rather simple ticket

Really?

28 files changed, 1518 insertions, 460 deletions

comment:20 Changed 4 years ago by jdemeyer

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

comment:21 follow-ups: Changed 4 years ago by vbraun

Well thats because a lot of testing was added. Really its an obvious improvement.

I don't think source code should reside in SAGE_ROOT/build

What about the old sage-pkg, move it to sage-old-pkg? Or are we going to keep it forever?

comment:22 in reply to: ↑ 21 Changed 4 years ago by jdemeyer

Replying to vbraun:

I don't think source code should reside in SAGE_ROOT/build

First of all, we already have source code there ($SAGE_ROOT/build/install). You will probably argue that this is a build script, not source code. But then your module is really also a build script, not source code (especially given that it's not installed).

Currently $SAGE_ROOT/src is essentially the Sage library, i.e. the Python package sage. I don't see how build scripts for Sage packages belong there.

comment:23 follow-up: Changed 4 years ago by vbraun

You didn't complain about sage-download-file being in src/bin/, so why complain now? There is much more than the Sage Python library in src.

comment:24 in reply to: ↑ 21 Changed 4 years ago by jdemeyer

Replying to vbraun:

Or are we going to keep it forever?

It's not even deprecated, so I would keep it for now. There are people still creating new old-style packages, see #18514 for example.

You could call your new script sage-packages or something...

comment:25 in reply to: ↑ 23 Changed 4 years ago by jdemeyer

Replying to vbraun:

You didn't complain about sage-download-file being in src/bin/, so why complain now?

We currently have only 1 place to put scripts and that's in $SAGE_ROOT/src/bin. You're creating a second place, so it makes to think what is the best location. After this ticket is merged, we should move more scripts to sage_bootstrap/bin, in particular sage-spkg and maybe even $SAGE_ROOT/build/install and $SAGE_ROOT/build/pipestatus. If sage_bootstrap would be in $SAGE_ROOT/build, that would lead to a clean separation of the complete build system for packages in $SAGE_ROOT/build.

I always found it awkward that we have a top-level Makefile calling a scipt build/install running make on build/Makefile which then runs src/bin/sage-spkg which in turn runs build/pkgs/*/spkg-install. That's a mess. This ticket creates an opportunity to start cleaning this up, let's do it right from the beginning.

Version 0, edited 4 years ago by jdemeyer (next)

comment:26 follow-up: Changed 4 years ago by vbraun

I'm happy to reorganize /build but I don't think we should do it in this ticket. IMHO the makefile stuff should first move to /build/make (or so) to make space for build/sage_bootstrap, build/setup.py, build/tox.ini, .... Otherwise you end up with /build/src/sage_bootstrap which is just duplicating src. Feel free to open another ticket.

comment:27 in reply to: ↑ 26 Changed 4 years ago by jdemeyer

Replying to vbraun:

I'm happy to reorganize /build but I don't think we should do it in this ticket.

+1

IMHO the makefile stuff should first move to /build/make (or so) to make space for build/sage_bootstrap

What's wrong with creating build/sage_bootstrap without moving the "makefile stuff" first? Independently of whether build/install and friends should be moved, what influence does it have on the present ticket?

I think it's stupid to first create $SAGE_ROOT/src/sage_bootstrap on this ticket and then immediately move it to a different place. Just put it in the correct place from the beginning...

comment:28 follow-ups: Changed 4 years ago by vbraun

then you end up with setup.py and Makefile in the same directory.... IMHO super-confusing.

comment:29 in reply to: ↑ 28 Changed 4 years ago by jdemeyer

Replying to vbraun:

then you end up with setup.py and Makefile in the same directory.... IMHO super-confusing.

I see your point, but I don't find it strong enough as an argument for why $SAGE_ROOT/src/sage_bootstrap/sage_bootstrap is better than $SAGE_ROOT/build/sage_bootstrap.

Can we get rid of that setup.py? Move it? Rename it?

comment:30 in reply to: ↑ 28 Changed 4 years ago by jdemeyer

Replying to vbraun:

IMHO super-confusing.

That can be solved with a simple comment like

#
# This setup.py file is only used for tox testing of the
# sage_bootstrap package, do not actually execute it!
#
# For building Sage packages, see "install", "deps" and "Makefile".
#

comment:31 follow-up: Changed 4 years ago by vbraun

Afaik setup.py can't have a different name. It certainly shouldn't be renamed. I'm fine with making /build a Python package, but then everything should have a sane directory layout. And moving stuff is super-easy in git, its a matter of minutes. So why do it now when /build is not ready for it?

comment:32 Changed 4 years ago by git

  • Commit changed from 0acf070e6d86df8f5dcc62f0751002631cbce9d8 to e45a0b667f908f147cabbec63fe88134061ac69a

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

52d0126rename sage-pkg -> sage-package
e4aff9afix indentation
e45a0b6Do not assume that the confball has been downloaded before in test

comment:33 Changed 4 years ago by vbraun

I opened #18788 to reorganize /build

comment:34 Changed 4 years ago by vbraun

  • Status changed from needs_work to needs_review

comment:35 in reply to: ↑ 31 ; follow-up: Changed 4 years ago by jdemeyer

Replying to vbraun:

And moving stuff is super-easy in git, its a matter of minutes.

Moving the files physically, sure. But that's only a very small part of the effort of moving files. All the code referring to those files needs to be changed, everything has to be tested again...

So why do it now when /build is not ready for it?

First of all, I don't agree with the premise that "build is not ready for it". I really don't see the problem. Second, like I already said: I would avoid having to move things in the future.

comment:36 in reply to: ↑ 28 Changed 4 years ago by jdemeyer

Replying to vbraun:

then you end up with setup.py and Makefile in the same directory.... IMHO super-confusing.

Let me remind you that we have had this in $SAGE_ROOT/src for a while and nobody ever complained...

comment:37 in reply to: ↑ 35 ; follow-up: Changed 4 years ago by vbraun

Replying to jdemeyer:

All the code referring to those files needs to be changed

The scripts are found via the PATH, thats the only thing that needs to be changed.

Second, like I already said: I would avoid having to move things in the future.

I wasn't the one who complained that this ticket does too much at once ;-)

comment:38 in reply to: ↑ 37 Changed 4 years ago by jdemeyer

Replying to vbraun:

I wasn't the one who complained that this ticket does too much at once ;-)

I'm not asking to do more in this ticket. I'm only asking to do it in such a way that we don't need a second ticket to move sage_bootstrap.

comment:39 Changed 4 years ago by dimpase

I don't see much change from the old setup:

$ sage -f hg
Attempting to download package hg
>>> Checking online list of optional packages.
>>> Checking online list of experimental packages.
>>> Checking online list of standard packages.
>>> Checking online list of huge packages.
>>> Checking online list of archive packages.
ERROR [download|run:133]: [Errno 404] Not Found: '//www.mirrorservice.org/sites/www.sagemath.org//spkg/archive/list'
sed: can't read /home/dima/.sage//archive.list: No such file or directory
Error: could not find a package matching hg on http://www.mirrorservice.org/sites/www.sagemath.org//spkg

should we have sage -reset_mirrors ?

comment:40 Changed 4 years ago by vbraun

I should have said: This ticket is about standard packages. Since hg is optional it is unrelated. But if you can't build "standard" Sage then this ticket might help.

comment:41 Changed 4 years ago by dimpase

what is tox and what is it doing? Perhaps it should go to another ticket, as it looks totally orthogonal to the stated purpose of refactoring what we have.

comment:42 Changed 4 years ago by vbraun

https://tox.readthedocs.org

The whole point of the refactoring is to make stuff testable

comment:43 Changed 4 years ago by jhpalmieri

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

comment:44 Changed 4 years ago by vbraun

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.

comment:45 follow-up: Changed 4 years ago by jhpalmieri

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.

comment:46 in reply to: ↑ 45 ; follow-up: Changed 4 years ago by vbraun

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.

comment:47 Changed 4 years ago by jhpalmieri

  • Branch changed from u/vbraun/python_library_to_bootstrap_sage to u/jhpalmieri/python_library_to_bootstrap_sage

comment:48 in reply to: ↑ 46 Changed 4 years ago by jhpalmieri

  • Branch changed from u/jhpalmieri/python_library_to_bootstrap_sage to u/vbraun/python_library_to_bootstrap_sage

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.

comment:49 Changed 4 years ago by jhpalmieri

  • Branch changed from u/vbraun/python_library_to_bootstrap_sage to u/jhpalmieri/python_library_to_bootstrap_sage
  • Commit changed from e45a0b667f908f147cabbec63fe88134061ac69a to cc213dcd06581e5906f837f1a0ea079b375d3e3f

New commits:

9e2c568Merge branch 'develop' into t/18748/python_library_to_bootstrap_sage
cc213dctrac 18748: add documentation to scripts in sage_bootstrap/bin

comment:50 Changed 4 years ago by vbraun

  • Branch changed from u/jhpalmieri/python_library_to_bootstrap_sage to u/vbraun/python_library_to_bootstrap_sage

comment:51 Changed 4 years ago by git

  • Commit changed from cc213dcd06581e5906f837f1a0ea079b375d3e3f to 075228f1c38f87975b08ecefbb5414e6dc0db9de

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

8f4fb8eMore docstrings
f9a69a5Convert public attributes to properties and document
075228fMake package.tarball a Tarball instance

comment:52 Changed 4 years ago by vbraun

Done, still needs review

comment:53 Changed 4 years ago by jhpalmieri

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'

comment:54 Changed 4 years ago by git

  • Commit changed from 075228f1c38f87975b08ecefbb5414e6dc0db9de to c56991b37c05e7381ca2f27ca7183fadddb1dfe6

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

c56991bFix Python 2.6 test error

comment:55 Changed 4 years ago by vbraun

Thanks, assertIsInstance` is Python 2.7+, fixed

comment:56 Changed 4 years ago by jhpalmieri

  • Branch changed from u/vbraun/python_library_to_bootstrap_sage to u/jhpalmieri/python_library_to_bootstrap_sage

comment:57 Changed 4 years ago by jhpalmieri

  • Commit changed from c56991b37c05e7381ca2f27ca7183fadddb1dfe6 to 60a581b42aa98a233d8b6c5a66487586f5619826
  • Reviewers set to John Palmieri

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

comment:58 Changed 4 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:59 Changed 4 years ago by vbraun

  • Branch changed from u/jhpalmieri/python_library_to_bootstrap_sage to 60a581b42aa98a233d8b6c5a66487586f5619826
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.