Opened 6 years ago
Closed 6 years ago
#18748 closed defect (fixed)
Python library to bootstrap Sage
Reported by:  vbraun  Owned by:  

Priority:  blocker  Milestone:  sage6.8 
Component:  build  Keywords:  
Cc:  ncohen  Merged in:  
Authors:  Volker Braun  Reviewers:  John Palmieri 
Report Upstream:  N/A  Work issues:  
Branch:  60a581b (Commits, GitHub, GitLab)  Commit:  60a581b42aa98a233d8b6c5a66487586f5619826 
Dependencies:  Stopgaps: 
Description (last modified by )
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 6 years ago by
 Component changed from PLEASE CHANGE to build
 Description modified (diff)
 Type changed from PLEASE CHANGE to defect
comment:2 Changed 6 years ago by
 Cc ncohen added
comment:3 Changed 6 years ago by
 Branch set to u/vbraun/python_library_to_bootstrap_sage
comment:4 Changed 6 years ago by
 Commit set to 0acf070e6d86df8f5dcc62f0751002631cbce9d8
comment:5 Changed 6 years ago by
 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 6 years ago by
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 6 years ago by
 No actual functionality changed
sagedownloadfile
code is refactored into a package Tests added for everything
 Output is explained in the README, no more if
self.verbose: printflush
hacks sagepkg
command to translate between package names and tarball filenames (and maybe query for package types in the future)
comment:8 Changed 6 years ago by
sagepkg
was the command to create oldstyle packages, now it's something completely different. Since we still support oldstyle 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 6 years ago by
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 6 years ago by
I honestly have no idea what tox
does, but I tried it anyway:
jdemeyer@tamiyo:/usr/local/src/sageconfig/src/sage_bootstrap$ uname a Linux tamiyo 3.17.7gentoo #1 SMP PREEMPT Wed Dec 31 20:06:39 CET 2014 x86_64 Intel(R) Core(TM) i72640M CPU @ 2.80GHz GenuineIntel GNU/Linux
jdemeyer@tamiyo:/usr/local/src/sageconfig/src/sage_bootstrap$ python version Python 3.3.2
jdemeyer@tamiyo:/usr/local/src/sageconfig/src/sage_bootstrap$ tox version 1.8.1 imported from /usr/lib64/python3.3/sitepackages/tox/__init__.py
jdemeyer@tamiyo:/usr/local/src/sageconfig/src/sage_bootstrap$ tox GLOB sdistmake: /usr/local/src/sageconfig/src/sage_bootstrap/setup.py py26 create: /usr/local/src/sageconfig/src/sage_bootstrap/.tox/py26 ERROR: InterpreterNotFound: python2.6 py27 instnodeps: /usr/local/src/sageconfig/src/sage_bootstrap/.tox/dist/sage_bootstrap1.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/sageconfig/src/sage_bootstrap/test/test_tarball.py", line 40, in test_checksum self.assertTrue(tarball.checksum_verifies()) File "/usr/local/src/sageconfig/src/sage_bootstrap/sage_bootstrap/tarball.py", line 83, in checksum_verifies sha1 = self._compute_sha1() File "/usr/local/src/sageconfig/src/sage_bootstrap/sage_bootstrap/tarball.py", line 73, in _compute_sha1 return self._compute_hash(hashlib.sha1()) File "/usr/local/src/sageconfig/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/sageconfig/upstream/configure99.tar.gz'  Ran 15 tests in 6.914s FAILED (errors=1) ERROR: InvocationError: '/usr/local/src/sageconfig/src/sage_bootstrap/.tox/py27/bin/python2.7 m unittest discover' py33 instnodeps: /usr/local/src/sageconfig/src/sage_bootstrap/.tox/dist/sage_bootstrap1.0.zip py33 runtests: PYTHONHASHSEED='1210919353' py33 runtests: commands[0]  python3.3 m unittest discover ..../usr/local/src/sageconfig/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/sageconfig/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 6 years ago by
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 6 years ago by
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 followup: ↓ 16 Changed 6 years ago by
Blocker because it fixes the firewall issue from #18723 and that ticket was a blocker.
comment:14 followup: ↓ 15 Changed 6 years ago by
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 newstyle package management functions via sagepkg 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 sagebootstrap only makes sense if you have the source lying around, so why pretend otherwise.
comment:15 in reply to: ↑ 14 Changed 6 years ago by
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#thefilespkgtxt
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 6 years ago by
comment:17 followup: ↓ 19 Changed 6 years ago by
THis is a rather simple ticket IMHO, it just refactors a script and adds testing.
comment:18 Changed 6 years ago by
 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 6 years ago by
Replying to vbraun:
THis is a rather simple ticket
Really?
28 files changed, 1518 insertions, 460 deletions
comment:20 Changed 6 years ago by
You have inconsistent indentation (not multiples of 4 spaces) in src/sage_bootstrap/sage_bootstrap/stdio.py
comment:21 followups: ↓ 22 ↓ 24 Changed 6 years ago by
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 sagepkg
, move it to sageoldpkg
? Or are we going to keep it forever?
comment:22 in reply to: ↑ 21 Changed 6 years ago by
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 followup: ↓ 25 Changed 6 years ago by
You didn't complain about sagedownloadfile
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 6 years ago by
comment:25 in reply to: ↑ 23 Changed 6 years ago by
Replying to vbraun:
You didn't complain about
sagedownloadfile
being insrc/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 sagespkg
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 toplevel Makefile
calling a scipt build/install
running make on build/Makefile
which then runs src/bin/sagespkg
which in turn runs build/pkgs/*/spkginstall
. That's a mess. This ticket creates an opportunity to start cleaning this up, let's do it right from the beginning.
comment:26 followup: ↓ 27 Changed 6 years ago by
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 6 years ago by
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 followups: ↓ 29 ↓ 30 ↓ 36 Changed 6 years ago by
then you end up with setup.py and Makefile in the same directory.... IMHO superconfusing.
comment:29 in reply to: ↑ 28 Changed 6 years ago by
Replying to vbraun:
then you end up with setup.py and Makefile in the same directory.... IMHO superconfusing.
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 6 years ago by
Replying to vbraun:
IMHO superconfusing.
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 followup: ↓ 35 Changed 6 years ago by
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 supereasy in git, its a matter of minutes. So why do it now when /build is not ready for it?
comment:32 Changed 6 years ago by
 Commit changed from 0acf070e6d86df8f5dcc62f0751002631cbce9d8 to e45a0b667f908f147cabbec63fe88134061ac69a
comment:33 Changed 6 years ago by
I opened #18788 to reorganize /build
comment:34 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:35 in reply to: ↑ 31 ; followup: ↓ 37 Changed 6 years ago by
Replying to vbraun:
And moving stuff is supereasy 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 6 years ago by
Replying to vbraun:
then you end up with setup.py and Makefile in the same directory.... IMHO superconfusing.
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 ; followup: ↓ 38 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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 [downloadrun: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 6 years ago by
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 6 years ago by
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 6 years ago by
The whole point of the refactoring is to make stuff testable
comment:43 Changed 6 years ago by
Why are some of the files not listed in MANIFEST
(e.g., tox.ini
, test/capture.py
)?
comment:44 Changed 6 years ago by
The manifest is autogenerated 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 finetune the inclusion rules.
comment:45 followup: ↓ 46 Changed 6 years ago by
Minor questions:
 In
package.py
, are the linesdef __eq__(self, other): return self.tarball == other.tarball
there just for testing viatox
, 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
sagepackage
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 forsagedownloadfile
, but at least the name is somewhat selfexplanatory. I think you should add a line or two there, also. Probably inREADME
, too, for example after the sentenceThis is a utility libary for dealing with thirdparty tarballs and building Sage.
(Something like "The main entry points are sagedownloadfile and sagepackage, 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. Fordownload.py
, for example, mentionDownload(...).run()
.  Add the link
https://tox.readthedocs.org
toREADME
. Maybe we should all know about tox already, but that doesn't seem to be the case.
comment:46 in reply to: ↑ 45 ; followup: ↓ 48 Changed 6 years ago by
Replying to jhpalmieri:
 In
package.py
, are the linesdef __eq__(self, other): return self.tarball == other.tarballthere just for testing viatox
, 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
sagepackage
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 6 years ago by
 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 6 years ago by
 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 6 years ago by
 Branch changed from u/vbraun/python_library_to_bootstrap_sage to u/jhpalmieri/python_library_to_bootstrap_sage
 Commit changed from e45a0b667f908f147cabbec63fe88134061ac69a to cc213dcd06581e5906f837f1a0ea079b375d3e3f
comment:50 Changed 6 years ago by
 Branch changed from u/jhpalmieri/python_library_to_bootstrap_sage to u/vbraun/python_library_to_bootstrap_sage
comment:51 Changed 6 years ago by
 Commit changed from cc213dcd06581e5906f837f1a0ea079b375d3e3f to 075228f1c38f87975b08ecefbb5414e6dc0db9de
comment:52 Changed 6 years ago by
Done, still needs review
comment:53 Changed 6 years ago by
I get an error with tox:
$ tox GLOB sdistmake: /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_bootstrap1.0.zip py26 installed: argparse==1.3.0,linecache2==1.0.0,sagebootstrap==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 6 years ago by
 Commit changed from 075228f1c38f87975b08ecefbb5414e6dc0db9de to c56991b37c05e7381ca2f27ca7183fadddb1dfe6
Branch pushed to git repo; I updated commit sha1. New commits:
c56991b  Fix Python 2.6 test error

comment:55 Changed 6 years ago by
Thanks, assertIsInstance` is Python 2.7+, fixed
comment:56 Changed 6 years ago by
 Branch changed from u/vbraun/python_library_to_bootstrap_sage to u/jhpalmieri/python_library_to_bootstrap_sage
comment:57 Changed 6 years ago by
 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 6 years ago by
 Status changed from needs_review to positive_review
comment:59 Changed 6 years ago by
 Branch changed from u/jhpalmieri/python_library_to_bootstrap_sage to 60a581b42aa98a233d8b6c5a66487586f5619826
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
add the sagepkg entrypoint
move sagedownloadfile to the sage_bootstrap library
initial implementation of the sagepkg tool