Re-Fix standard_packages(), optional_packages(), and experimental_packages()

[nathanncohen]

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 old-style spkg and new-style spkg.

Nathann

Depends on #18431

CC: @nexttime @vbraun @jdemeyer

Component: distribution

Author: Nathann Cohen

Branch/Commit: b093007

Reviewer: John Palmieri

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

[nathanncohen]

Branch: u/ncohen/18456

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

4770518	trac #18431: Type file for each package+define var from pkg name
3efc9fb	trac #18431: package-specific 'dependencies' files
308ceff	trac #18431: Auto-generate make rules for standard packages
77f293f	trac #18431: auto-generated Make target "sage-standard-packages"
e114874	trac #18431: typo
766798e	trac #18431: tab->spaces
47e438d	Merge tag '6.7' into t/18441/base_packages_except_configure_should_be_standard
4de8e37	Various changes to build system
f83b0c4	Correct variable name
ebf52de	trac #18456: Re-Fix standard_packages(), optional_packages(), and experimental_packages()

[sagetrac-git]

Commit: ebf52de

[sagetrac-git]

Changed commit from ebf52de to 39cc007

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

39cc007

trac #18456: Re-Fix standard_packages(), optional_packages(), and experimental_packages()

[JohnCremona]

comment:4

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!

[nathanncohen]

comment:5

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

[JohnCremona]

comment:6

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.

[nathanncohen]

comment:7

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

[JohnCremona]

comment:8

Replying to @nathanncohen:

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

[nathanncohen]

comment:9

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

[nathanncohen]

comment:11

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

[JohnCremona]

comment:12

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!

[nathanncohen]

comment:13

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

[tscrim]

comment:14

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.

[jhpalmieri]

comment:15

Replying to @nathanncohen:

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.

[nathanncohen]

comment:16

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 command-line tool as it is done at the moment.

Nathann

[jhpalmieri]

comment:17

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

[nathanncohen]

comment:18

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 on sage --standard, or vice versa.

+1

My strong preference is for sage --standard to not rely on standard_packages, since sage --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 re-parse 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

[nathanncohen]

comment:19

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

[jhpalmieri]

comment:22

What do you think about either of these changes?

diff --git a/src/bin/sage-list-packages b/src/bin/sage-list-packages
index 5b17272..e1b0889 100755
--- a/src/bin/sage-list-packages
+++ b/src/bin/sage-list-packages
@@ -16,7 +16,12 @@ SAGE_ROOT = os.environ["SAGE_ROOT"]
 # Input parsing #
 #################
 
-parser = argparse.ArgumentParser(description="List Sage's packages")
+parser = argparse.ArgumentParser(formatter_class=argparse.RawDescriptionHelpFormatter,
+                                 description="""List Sage's packages, by default in the format
+
+  {:.<30} {}
+
+""".format("package", "version on-line (version installed)"))
 parser.add_argument('category', choices=['standard','optional','experimental', 'installed'], metavar="category",
                     help="The type of packages. Can be 'standard','optional','experimental' or 'installed'.")
 parser.add_argument('--dump', dest='dump', default=False, action='store_true',
@@ -76,6 +81,10 @@ if args['remote'] and args['category']!='installed':
 # OUTPUT #
 ##########
 
+if not args['dump']:
+    print("{:.<40} {}".format("PACKAGE", "version on-line (version installed)"))
+    print("-"*76)
+
 # installed
 if args['category'] == 'installed':
     if args['version']:

I ask because when I ran sage --standard, I wasn't sure what a line like

zeromq.................................. not found (4.0.5)

in the output meant.

[jhpalmieri]

comment:23

This change is more important.

diff --git a/src/bin/sage-list-packages b/src/bin/sage-list-packages
index 5b17272..88cbaab 100755
--- a/src/bin/sage-list-packages
+++ b/src/bin/sage-list-packages
@@ -46,12 +51,12 @@ installed = dict(pkgname_split(pkgname)
                  for pkgname in os.listdir(SAGE_SPKG_INST))
 
 # new-style packages
-local_non_filtered = os.listdir(SAGE_ROOT+"/build/pkgs/")
+local_non_filtered = os.listdir(os.path.join(SAGE_ROOT, "build", "pkgs"))
 local = {}
 for p in local_non_filtered:
-    with open(SAGE_ROOT+"/build/pkgs/"+p+"/package-version.txt") as f:
+    with open(os.path.join(SAGE_ROOT, "build", "pkgs", p, "package-version.txt")) as f:
         version = f.read().strip()
-    with open(SAGE_ROOT+"/build/pkgs/"+p+"/type") as f:
+    with open(os.path.join(SAGE_ROOT, "build", "pkgs", p, "type")) as f:
         if f.read().strip() == args['category']:
             local[p] = version
 

I can create a new branch with this change and either of the earlier ones if you want. Or you can do it.

[jdemeyer]

comment:24

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, arb-2.5.0 is installed but arb-2.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...

[sagetrac-git]

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

687e426

trac #18456: Column names

[sagetrac-git]

Changed commit from a3402c9 to 687e426

[nathanncohen]

comment:26

What do you think about either of these changes?
...
I ask because when I ran sage --standard, I wasn't sure what a line like

zeromq.................................. 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

[sagetrac-git]

Changed commit from 687e426 to d671f1b

[sagetrac-git]

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

d671f1b

trac #18456: don't use explicitly '/' in file paths

[nathanncohen]

comment:28

This change is more important.

Right. Done.

Nathann

[nathanncohen]

comment:29

There are no doctests for _package_lists_from_sage_output(..., version=True), nor does the OUTPUT 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), where installed_version is None if the package is not installed and latest_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

[jdemeyer]

comment:30

Replying to @nathanncohen:

I do not mind, you can open a ticket if you need this feature.

Done: #18581

[sagetrac-git]

Changed commit from d671f1b to 9854749

[sagetrac-git]

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

9854749

trac #18456: doc and doctest

[jhpalmieri]

comment:32

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/site-packages/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/site-packages/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:

diff --git a/src/sage/misc/package.py b/src/sage/misc/package.py
index 12b556d..91ab594 100644
--- a/src/sage/misc/package.py
+++ b/src/sage/misc/package.py
@@ -239,9 +239,7 @@ def _package_lists_from_sage_output(package_type,version=False,local=False):
         sage: installed, not_installed = _package_lists_from_sage_output('standard',local=True,version=True)
         sage: bool(not_installed)
         False
-        sage: any(x[0] == 'glpk' for x in installed)
-        True
-        sage: type(x[1]) == str and '4.55' <= x[1]
+        sage: any(x[0] == 'glpk' and type(x[1]) == str and '4.55' <= x[1] for x in installed)
         True
     """
     if package_type not in ['standard','optional','experimental']:

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b093007

trac #18456: doc and doctest

[sagetrac-git]

Changed commit from 9854749 to b093007

[nathanncohen]

comment:34

I get an error with the new doctest:

Rightrightright, thanks! I updated the latest commit.

Nathann

[jhpalmieri]

comment:35

This looks good to me. Anyone else have any issues, or can we set it to positive review?

[jdemeyer]

Reviewer: John Palmieri

[vbraun]

Changed branch from u/ncohen/18456 to b093007