Opened 6 years ago
Closed 6 years ago
#18456 closed enhancement (fixed)
ReFix standard_packages(), optional_packages(), and experimental_packages()
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.8 
Component:  distribution  Keywords:  
Cc:  leif, vbraun, jdemeyer  Merged in:  
Authors:  Nathann Cohen  Reviewers:  John Palmieri 
Report Upstream:  N/A  Work issues:  
Branch:  b093007 (Commits)  Commit:  b09300783d2dffe3b3fa6f4f643861909457e34f 
Dependencies:  #18431  Stopgaps: 
Description (last modified by )
What was fixed in #18407 (one week ago) has been broken since :P
That's because the website went mostly down. Also, that new version handles both oldstyle spkg and newstyle spkg.
Nathann
Change History (37)
comment:1 Changed 6 years ago by
 Branch set to u/ncohen/18456
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Commit set to ebf52de4e4f6e7cb16d1372648617ff401b59d53
comment:3 Changed 6 years ago by
 Commit changed from ebf52de4e4f6e7cb16d1372648617ff401b59d53 to 39cc007aaaf83a6ff0fbd99536c1561486fbfead
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
39cc007  trac #18456: ReFix standard_packages(), optional_packages(), and experimental_packages()

comment:4 followup: ↓ 5 Changed 6 years ago by
I insstalled the branch and did "make" which rebuilt a very large amount. Then
./sage standard
gave
Using Sage Server http://www.sagemath.org/spkg HTTP Error 404: Not Found ******************************************************************************** Error contacting http://www.sagemath.org/spkg/standard/list. Try using an alternative server. For example, from the bash prompt try typing export SAGE_SERVER=http://sage.scipy.org/sage/ then try again.
which also failed. SO I don't exactly know what it is that I am supposed to be testing!
comment:5 in reply to: ↑ 4 ; followup: ↓ 15 Changed 6 years ago by
Hello !
I insstalled the branch and did "make" which rebuilt a very large amount.
This is suspicious. As you can see from the branch's diff, it only touches one file (i.e. sage/misc/package.py) which is not a dependency of anything as far as I know. I just did a checkout on the branch, and no recompilation was triggered at all.
which also failed. SO I don't exactly know what it is that I am supposed to be testing!
This branch fixes the three functions standard_packages
, optional_packages
, and experimental_packages
defined in sage/misc/package.py
. The script sage standard
script does not call them.
In order to fix sage standard
, it makes more sense to make it call a Sage function that does the job. Previously, standard_packages
ran sage standard
and parsed its output: it would be better to have sage standard
call standard_packages
(without any parsing).
That would require changing the output or standard_packages
, as currently it only returns the list of packages without including the version numbers.
Nathann
comment:6 followup: ↓ 7 Changed 6 years ago by
You are right to be suspicious. I was starting from 6.8.beta0, while I think that the branch branched off from 6.7  right? That would explain the extra rebuilding.
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 6 years ago by
You are right to be suspicious. I was starting from 6.8.beta0, while I think that the branch branched off from 6.7  right? That would explain the extra rebuilding.
Yep, totally. If it can be of interest to you, I use a short bash script that downloads a branch for me, and merges it with my current beta. This way, I never go back to an earlier version of sage and I avoid these problems.
About sage standard
: I added a commit at public/18456v2 that makes it work again, though without the version number.
I can add it to this ticket if you want.
Nathann
comment:8 in reply to: ↑ 7 ; followup: ↓ 9 Changed 6 years ago by
 Status changed from needs_review to needs_work
Replying to ncohen:
You are right to be suspicious. I was starting from 6.8.beta0, while I think that the branch branched off from 6.7  right? That would explain the extra rebuilding.
Yep, totally. If it can be of interest to you, I use a short bash script that downloads a branch for me, and merges it with my current beta. This way, I never go back to an earlier version of sage and I avoid these problems.
About
sage standard
: I added a commit at public/18456v2 that makes it work again, though without the version number.I can add it to this ticket if you want.
Well, if it doesn't work as is then it cannot get a positive review anyway, so I have changed it to "needs work" and can try again after you have added what is needed to make it work!
Nathann
comment:9 in reply to: ↑ 8 Changed 6 years ago by
Well, if it doesn't work as is then it cannot get a positive review anyway, so I have changed it to "needs work" and can try again after you have added what is needed to make it work!
You got me wrong: it does not work in the latest beta either. None of the two work. And this ticket fixes one of the two.
Nathann
comment:10 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:11 Changed 6 years ago by
Again: if you want I can add this other commit. It makes 'sage standard' call the functions I fix in this branch and display the list of packages (though without the version number).
Nathann
comment:12 followup: ↓ 13 Changed 6 years ago by
Sorry, I ran out of time on this and have to go to give an exam (and grade 257 scripts) now. For whoever comes next to the ticket, you will have to explain better why something known not to work is still on "needs review" since I don't understand!
comment:13 in reply to: ↑ 12 Changed 6 years ago by
For whoever comes next to the ticket, you will have to explain better why something known not to work is still on "needs review" since I don't understand!
I do not know where you got the idea that this branch breaks anything. If you tell me what, I can surely explain. This branch fixes three functions, as advertised in the ticket's title.
Nathann
comment:14 Changed 6 years ago by
I believe we should fix sage standard
and similar on this ticket. I think what John is confused about is that this currently fixes the interactive Sage functions (i.e., run from within Sage), not those you run on the shell.
I'm okay with the output on public/18456v2
, but I'd feel more comfortable if someone more experienced with (Sage's) bash scripts (and those who might have an opinion on this) took a look at it before giving this a positive review.
comment:15 in reply to: ↑ 5 Changed 6 years ago by
Replying to ncohen:
In order to fix
sage standard
, it makes more sense to make it call a Sage function that does the job.
I don't agree with this: ideally, sage standard
, sage optional
, etc., should work without building Sage, or with a partially built Sage.
comment:16 Changed 6 years ago by
Hello guys,
I wrote this fix for sage standard
because that is what everybody seems attracted to, but really my goal here is only to fix standard_packages
(and others) so that I can use them in #18408. And then, in turn, update #18405.
Right now, everything is broken. And I personally prefer by far to have the *_package
functions get ther info directly from the server than parse the output of a commandline tool as it is done at the moment.
Nathann
comment:17 followup: ↓ 18 Changed 6 years ago by
Just to make sure I understand: the original purpose of this ticket was to fix standard_packages
(etc.), but not sage standard
, right? That's fine with me.
For ease of maintenance (and for cleanliness of code, which isn't quite as important), it would be best to not duplicate the work, so it would be best to have standard_packages
rely on sage standard
, or vice versa. My strong preference is for sage standard
to not rely on standard_packages
, since sage standard
should work without having built Sage.
That's my opinion, but I don't feel that strongly about the code duplication, so I personally won't object if this ticket only fixes standard_packages
(etc.) by having it talk directly to the server. I will object if it also fixes sage standard
in such a way that it won't work right after unpacking a fresh tarball.
(In my brief tests, by the way, this does fix the Sage functions as intended. But I haven't looked at the actual changes to see if they make sense.)
comment:18 in reply to: ↑ 17 Changed 6 years ago by
Hello !
For ease of maintenance (and for cleanliness of code, which isn't quite as important), it would be best to not duplicate the work, so it would be best to have
standard_packages
rely onsage standard
, or vice versa.
+1
My strong preference is for
sage standard
to not rely onstandard_packages
, sincesage standard
should work without having built Sage.
That's what I tried first, i.e. fix sage standard
. But then you have some problems, like undefined environment variables, and you must reparse everything in standard_packages
again.
I thought a bit about making this script dependent on sage, and I actually ended up wondering whether .... we need those sage standard
scripts. Right now they are totally broken, and... Well, if we do not need them then perhaps we should stop maintaining them. They returned outdated information long before they broke.
(In my brief tests, by the way, this does fix the Sage functions as intended. But I haven't looked at the actual changes to see if they make sense.)
I hope that they do ;)
Nathann
comment:19 followup: ↓ 21 Changed 6 years ago by
Okay. Third fix.
This one fixes the three functions AND the sage standard
commands (which do not need Sage to be compiled). The executable now has a 'dump' options so that there is not much parsing left to do on Sage's side when running that script to get some info.
When it will be reviewed, I will be able to add a cool feature: automatic test of (installed) optional packages.
Needs review again,
Nathann
comment:20 Changed 6 years ago by
 Commit changed from 39cc007aaaf83a6ff0fbd99536c1561486fbfead to a3402c959a7d74ede7a58ae1d75b17faba0ac52d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a3402c9  trac #18456: ReFix standard_packages(), optional_packages(), and experimental_packages()

comment:21 in reply to: ↑ 19 Changed 6 years ago by
comment:22 followup: ↓ 26 Changed 6 years ago by
What do you think about either of these changes?

src/bin/sagelistpackages
diff git a/src/bin/sagelistpackages b/src/bin/sagelistpackages index 5b17272..e1b0889 100755
a b SAGE_ROOT = os.environ["SAGE_ROOT"] 16 16 # Input parsing # 17 17 ################# 18 18 19 parser = argparse.ArgumentParser(description="List Sage's packages") 19 parser = argparse.ArgumentParser(formatter_class=argparse.RawDescriptionHelpFormatter, 20 description="""List Sage's packages, by default in the format 21 22 {:.<30} {} 23 24 """.format("package", "version online (version installed)")) 20 25 parser.add_argument('category', choices=['standard','optional','experimental', 'installed'], metavar="category", 21 26 help="The type of packages. Can be 'standard','optional','experimental' or 'installed'.") 22 27 parser.add_argument('dump', dest='dump', default=False, action='store_true', … … if args['remote'] and args['category']!='installed': 76 81 # OUTPUT # 77 82 ########## 78 83 84 if not args['dump']: 85 print("{:.<40} {}".format("PACKAGE", "version online (version installed)")) 86 print(""*76) 87 79 88 # installed 80 89 if args['category'] == 'installed': 81 90 if args['version']:
comment:23 followup: ↓ 28 Changed 6 years ago by
This change is more important.

src/bin/sagelistpackages
diff git a/src/bin/sagelistpackages b/src/bin/sagelistpackages index 5b17272..88cbaab 100755
a b installed = dict(pkgname_split(pkgname) 46 51 for pkgname in os.listdir(SAGE_SPKG_INST)) 47 52 48 53 # newstyle packages 49 local_non_filtered = os.listdir( SAGE_ROOT+"/build/pkgs/")54 local_non_filtered = os.listdir(os.path.join(SAGE_ROOT, "build", "pkgs")) 50 55 local = {} 51 56 for p in local_non_filtered: 52 with open( SAGE_ROOT+"/build/pkgs/"+p+"/packageversion.txt") as f:57 with open(os.path.join(SAGE_ROOT, "build", "pkgs", p, "packageversion.txt")) as f: 53 58 version = f.read().strip() 54 with open( SAGE_ROOT+"/build/pkgs/"+p+"/type") as f:59 with open(os.path.join(SAGE_ROOT, "build", "pkgs", p, "type")) as f: 55 60 if f.read().strip() == args['category']: 56 61 local[p] = version 57 62
I can create a new branch with this change and either of the earlier ones if you want. Or you can do it.
comment:24 followup: ↓ 29 Changed 6 years ago by
 Status changed from needs_review to needs_work
There are no doctests for _package_lists_from_sage_output(..., version=True)
, nor does the OUTPUT
block explain the output in that case.
For version=True
, I would actually propose to change the output format to a list of triples (package_name, installed_version, latest_version)
, where installed_version
is None
if the package is not installed and latest_version
is the newest available version. If, for example, arb2.5.0
is installed but arb2.6.0
is availble, then one such triple would be
('arb', '2.5.0', '2.6.0')
If arb
is not installed:
('arb', None, '2.6.0')
Since this function would be quite different from _package_lists_from_sage_output
, perhaps there should be two functions: _package_lists_from_sage_output
and _package_versions_from_sage_output
or something...
comment:25 Changed 6 years ago by
 Commit changed from a3402c959a7d74ede7a58ae1d75b17faba0ac52d to 687e4261edc550d7521d99bfef9839e401ef522e
Branch pushed to git repo; I updated commit sha1. New commits:
687e426  trac #18456: Column names

comment:26 in reply to: ↑ 22 Changed 6 years ago by
What do you think about either of these changes? ... I ask because when I ran
sage standard
, I wasn't sure what a line likezeromq.................................. not found (4.0.5)in the output meant.
I added a commit that displays column names when 'dump' is not used. The columns change a lot depending on which flags are used, and so this code is not exactly the one you proposed.
Nathann
comment:27 Changed 6 years ago by
 Commit changed from 687e4261edc550d7521d99bfef9839e401ef522e to d671f1ba7545507e710099a94cb1f07f5ba2047d
Branch pushed to git repo; I updated commit sha1. New commits:
d671f1b  trac #18456: don't use explicitly '/' in file paths

comment:28 in reply to: ↑ 23 Changed 6 years ago by
This change is more important.
Right. Done.
Nathann
comment:29 in reply to: ↑ 24 ; followup: ↓ 30 Changed 6 years ago by
 Status changed from needs_work to needs_review
There are no doctests for
_package_lists_from_sage_output(..., version=True)
, nor does theOUTPUT
block explain the output in that case.
Fixed.
For
version=True
, I would actually propose to change the output format to a list of triples(package_name, installed_version, latest_version)
, whereinstalled_version
isNone
if the package is not installed andlatest_version
is the newest available version.
I do not mind, you can open a ticket if you need this feature.
Since this function would be quite different from
_package_lists_from_sage_output
, perhaps there should be two functions:_package_lists_from_sage_output
and_package_versions_from_sage_output
or something...
Two private functions that do almost the same job? Let's keep things simple, one is enough.
Nathann
comment:30 in reply to: ↑ 29 Changed 6 years ago by
comment:31 Changed 6 years ago by
 Commit changed from d671f1ba7545507e710099a94cb1f07f5ba2047d to 985474957e3f7327e635c7aa13dbf70f510c727f
Branch pushed to git repo; I updated commit sha1. New commits:
9854749  trac #18456: doc and doctest

comment:32 followup: ↓ 34 Changed 6 years ago by
I get an error with the new doctest:
sage t long src/sage/misc/package.py ********************************************************************** File "src/sage/misc/package.py", line 244, in sage.misc.package._package_lists_from_sage_output Failed example: type(x[1]) == str and '4.55' <= x[1] Exception raised: Traceback (most recent call last): File "/Users/jpalmier/Desktop/Sage_stuff/git/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 496, in _run self.compile_and_execute(example, compiler, test.globs) File "/Users/jpalmier/Desktop/Sage_stuff/git/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 858, in compile_and_execute exec(compiled, globs) File "<doctest sage.misc.package._package_lists_from_sage_output[8]>", line 1, in <module> type(x[Integer(1)]) == str and '4.55' <= x[Integer(1)] TypeError: 'sage.symbolic.expression.Expression' object does not support indexing **********************************************************************
The problem is that x
does not get set by the expression any(x[0] ...)
. You could maybe use this instead:

src/sage/misc/package.py
diff git a/src/sage/misc/package.py b/src/sage/misc/package.py index 12b556d..91ab594 100644
a b def _package_lists_from_sage_output(package_type,version=False,local=False): 239 239 sage: installed, not_installed = _package_lists_from_sage_output('standard',local=True,version=True) 240 240 sage: bool(not_installed) 241 241 False 242 sage: any(x[0] == 'glpk' for x in installed) 243 True 244 sage: type(x[1]) == str and '4.55' <= x[1] 242 sage: any(x[0] == 'glpk' and type(x[1]) == str and '4.55' <= x[1] for x in installed) 245 243 True 246 244 """ 247 245 if package_type not in ['standard','optional','experimental']:
comment:33 Changed 6 years ago by
 Commit changed from 985474957e3f7327e635c7aa13dbf70f510c727f to b09300783d2dffe3b3fa6f4f643861909457e34f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b093007  trac #18456: doc and doctest

comment:34 in reply to: ↑ 32 Changed 6 years ago by
I get an error with the new doctest:
Rightrightright, thanks! I updated the latest commit.
Nathann
comment:35 Changed 6 years ago by
This looks good to me. Anyone else have any issues, or can we set it to positive review?
comment:36 Changed 6 years ago by
 Reviewers set to John Palmieri
 Status changed from needs_review to positive_review
comment:37 Changed 6 years ago by
 Branch changed from u/ncohen/18456 to b09300783d2dffe3b3fa6f4f643861909457e34f
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
trac #18431: Type file for each package+define var from pkg name
trac #18431: packagespecific 'dependencies' files
trac #18431: Autogenerate make rules for standard packages
trac #18431: autogenerated Make target "sagestandardpackages"
trac #18431: typo
trac #18431: tab>spaces
Merge tag '6.7' into t/18441/base_packages_except_configure_should_be_standard
Various changes to build system
Correct variable name
trac #18456: ReFix standard_packages(), optional_packages(), and experimental_packages()