_package_lists_from_sage_output() should output installed and available versions

[jdemeyer]

Currently, the function _package_lists_from_sage_output only gives the latest available version of a package, not the version which is installed.

I propose to define a new function package_versions(local=False) which returns a dictionary with key:value pairs of the form

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 key:value pair would be

'arb': ('2.5.0', '2.6.0')

If arb is not installed, this would be:

'arb': (None, '2.6.0')

Depends on #18456

CC: @nathanncohen

Component: misc

Author: John Palmieri

Branch/Commit: 262ba16

Reviewer: Jeroen Demeyer

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

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-Currently, the function `standard_packages()` only gives the latest available version of a package, not the version which is installed. I propose to actually return a list of triples
+Currently, the function `_package_lists_from_sage_output` only gives the latest available version of a package, not the version which is installed. I propose to actually return a list of triples

(package_name, installed_version, latest_version)

[jhpalmieri]

Branch: u/jhpalmieri/versions

[jhpalmieri]

comment:4

The actual change is

diff --git a/src/sage/misc/package.py b/src/sage/misc/package.py
index 91ab594..6a9f33d 100644
--- a/src/sage/misc/package.py
+++ b/src/sage/misc/package.py
@@ -216,9 +216,13 @@ def _package_lists_from_sage_output(package_type,version=False,local=False):
     When ``version=False``, the function returns a pair of list
     ``(installed,not_installed)`` with the corresponding packages' name.

-    When ``version=True``, the elements of each list are not package names but
-    pairs ``(package_name,package_version)`` where ``version`` is a string
-    representing the version of each package that is installed.
+    When ``version=True``, the elements of each list are not package
+    names but either pairs ``(package_name, installed_version)`` or
+    triples ``(package_name, latest_version, installed_version)``,
+    depending on whether ``local`` is ``True`` or ``False``. Here,
+    ``latest_version`` is a string representing the version of the
+    package available on the Sage package server, while
+    ``installed_version`` is the version that is actually installed.

     EXAMPLE::

@@ -257,11 +261,10 @@ def _package_lists_from_sage_output(package_type,version=False,local=False):
     if version:
         for line in X:
             line = line.split(' ')
-            name,version = line[0],line[1]
-            if is_package_installed(name):
-                installed.append((name,version))
+            if is_package_installed(line[0]):
+                installed.append(tuple(line))
             else:
-                not_installed.append((name,version))
+                not_installed.append(tuple(line))
     else:
         for name in X:
             if is_package_installed(name):

although the branch includes all of the changes from #18456.

[jhpalmieri]

Commit: 17bc8fa

[jhpalmieri]

Author: John Palmieri

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1,18 +1,20 @@
-Currently, the function `_package_lists_from_sage_output` only gives the latest available version of a package, not the version which is installed. I propose to actually return a list of triples
+Currently, the function `_package_lists_from_sage_output` only gives the latest available version of a package, not the version which is installed.
+
+I propose to define a new function `package_versions(local=False)` which returns a dictionary with key:value pairs of the form

-(package_name, installed_version, latest_version) +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
+If, for example, arb-2.5.0 is installed but arb-2.6.0 is availble, then one such key:value pair would be

-('arb', '2.5.0', '2.6.0') +'arb': ('2.5.0', '2.6.0')

 If arb is not installed, this would be:

-('arb', None, '2.6.0') +'arb': (None, '2.6.0')

[jdemeyer]

comment:5

I don't want a tuple of lists of tuples, that's just too complicated. I meant just one list of tuples.

Actually thinking a bit more about this, a dictionary makes more sense: the keys are the package names and the values are tuples (installed_version, latest_version).

And I certainly want the output format to be the same for local=True and local=False.

I also propose to make this a separate function package_versions() to avoid the many if version branches you will need. And then implement the old _package_lists_from_sage_output() as a very short function using this new package_versions() function.

[jdemeyer]

comment:6

Note: everything I said in the last comment should be treated as a suggestion, not a demand.

Except for the fact that the output for local=True should also include the latest available version, that's really important for #18558.

[jhpalmieri]

comment:7

Replying to @jdemeyer:

Note: everything I said in the last comment should be treated as a suggestion, not a demand.

Except for the fact that the output for local=True should also include the latest available version, that's really important for #18558.

Then I don't understand what you want. With local=True, you don't use the internet, but to find the latest available version, you need the internet. So I'm puzzled.

Regarding your previous comment, if we implement package_versions(), then do you care much what sort of output _package_lists_from_sage_output produces? Should I just revert it back to the output it had in #18456?

[jhpalmieri]

comment:8

Replying to @jhpalmieri:

Then I don't understand what you want. With local=True, you don't use the internet, but to find the latest available version, you need the internet. So I'm puzzled.

Or at least, that's how "latest available version" is defined in the earlier versions. Do you just want to read the data from build/pkgs/benzene/package_version.txt, etc.? What do you do with old style packages?

[jdemeyer]

comment:9

Replying to @jhpalmieri:

Then I don't understand what you want. With local=True, you don't use the internet, but to find the latest available version, you need the internet. So I'm puzzled.

You're right, I was only thinking about new-style packages. I propose the following:

• for new-style packages, read the latest available version from buikd/pkgs/foo/package_version.txt
• for old-style packages with local=True, use None as latest available version.

[jdemeyer]

comment:10

Replying to @jhpalmieri:

Regarding your previous comment, if we implement package_versions(), then do you care much what sort of output _package_lists_from_sage_output produces?

If we do implement package_versions(), then _package_lists_from_sage_output shouldn't have a version argument.

[jhpalmieri]

comment:11

Okay, then should we rewrite sage --standard (etc.) to just read data from build/pkgs/.../package_version.txt? I think you're saying this:

• if local is True, just read package_version.txt information. For old-style packages, we don't even know about them in that case.
• if local is False, still use package_version.txt for new-style packages. For old-style packages, check the servers.

That sounds okay to me.

[jhpalmieri]

comment:12

I suppose to make this work well, when a package is installed, we should record its type in its record in local/var/lib/sage/installed. I'm not sure I want to bother with that. As it is, if someone installs an old-style package, we don't know what type it is without checking the servers (and the servers are not 100% accurate: e.g., zeromq is listed as both standard and optional on the servers).

Currently, sage --optional (etc.) ignores the files in var/lib/sage/installed when compiling a list of packages to print: it only checks build/pkgs and the servers. (It does check those files to see what version of a package was installed.) I'm not sure of a better way to proceed, especially if local is True.

[jdemeyer]

comment:13

Replying to @jhpalmieri:

Okay, then should we rewrite sage --standard (etc.) to just read data from build/pkgs/.../package_version.txt?

I don't know, really. What would you find easier, changing sage --standard or changing the Sage library code? Nathann, do you have an opinion on this?

I think you're saying this:

• if local is True, just read package_version.txt information. For old-style packages, we don't even know about them in that case.
• if local is False, still use package_version.txt for new-style packages. For old-style packages, check the servers.

Yes, indeed.

[jdemeyer]

comment:14

Replying to @jhpalmieri:

I suppose to make this work well, when a package is installed, we should record its type in its record in local/var/lib/sage/installed.

I don't see why we need to change that. I think the current way of determining packages is good enough, especially since we're having fewer and fewer old-style packages. So, I would focus on getting new-style package right.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 17bc8fa to e9f2500

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

e9f2500

trac 18581: new function package_versions

[jhpalmieri]

comment:16

It's pretty easy to change sage --standard: all of the information is there, it's just a matter of how things are printed out. I might even consider the old way a bug. Here's a new version.

[jdemeyer]

comment:17

Can you replace the string "not_installed" in package_versions() by None? It's just because checking x == "not_installed" is a lot stranger and harder to remember than checking x is None.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from e9f2500 to 9178afc

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

9178afc

trac 18581: return None rather than 'not_installed' for version of uninstalled packages

[jhpalmieri]

comment:19

Okay.

[jdemeyer]

comment:20

Doesn't this need to be changed also?

if versions[p][0] != 'not_installed'

[jdemeyer]

comment:21

For is_package_installed(), can you add as doctest

sage: is_package_installed('pari')
True

just because sage is special (it doesn't have a version number)

[jdemeyer]

comment:22

Instead of running sage --{} --dump (with {} == "standard" for example), why don't you directly run

sage-list-packages {} --dump

(this is significantly faster since it avoids all the shell script overhead like sage-env)

[jdemeyer]

comment:23

Instead of this strange test

std['glpk'][0] >= '4.55'

couldn't you just show the actual result of something like

std['zn_poly']

(a package which hasn't been updated in a long time in Sage and whose upstream is dead, so changes in the version number are unlikely)

[jdemeyer]

comment:24

I would also like to see package_versions() added to sage.misc.all

[jdemeyer]

comment:25

This sentence is missing a final period: If ``local`` is False, then Sage's servers are queried for package information

[jdemeyer]

comment:26

Is there a reason why

./sage --optional --local

does not contain the lastest available version? (with --dump, it does)

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

24c0d0b

trac 18581: responses to referee's comments

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 9178afc to 24c0d0b

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 24c0d0b to caa8301

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

caa8301

trac 18581: fix for 'sage --optional --local'

[jhpalmieri]

comment:29

The zn_poly doctest makes me a little nervous, in case the version does change, but I added a note to SPKG.txt about it, just in case.

Replying to @jdemeyer:

Is there a reason why

./sage --optional --local

does not contain the lastest available version? (with --dump, it does)

Sorry, I missed that case in my changes to sage-list-packages.

[jdemeyer]

comment:31

Replying to @sagetrac-git:

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

caa8301

trac 18581: fix for 'sage --optional --local'

You also need to fix the header:

jdemeyer@tamiyo:/usr/local/src/sage-git$ ./sage --optional --local
[package] .............................. [installed version]
4ti2.................................... 1.6.5 (1.6.5)
arb..................................... 2.5.0 (2.6.0)
autotools............................... 20141105 (not_installed)
benzene................................. 20130630 (not_installed)
bliss................................... 0.72.p1 (not_installed)
[...]

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:32

Nathann, do you have any comments about this ticket?

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from caa8301 to f7dc0cd

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

f7dc0cd

trac 18581: fix header for 'sage-list-packages TYPE --local'

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

61a4076

trac 18581: stylistic changes in sage-list-packages

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from f7dc0cd to 61a4076

[jhpalmieri]

comment:35

I fixed the header. I also made some stylistic changes, for example replacing "foo,bar" with "foo, bar" in various places, and cutting most of the lines to fewer than 80 characters.

[jdemeyer]

comment:36

Your last changes made me notice this:

pname = re.sub("(\.|-)p[0-9]+$", "", pname) # strip .p0
pname = re.sub("(\.|-)[0-9].*", "", pname) # strip version number

That's actually too complicated and also wrong. As far as I know, all other places in Sage handling packages just split on the first - and that should be done here also.

[jdemeyer]

comment:37

What's the point of this complication?

urlexec=os.path.join(os.path.dirname(sys.argv[0]), "sage-download-file")
MIRROR_URL = os.popen(urlexec+" --print-fastest-mirror").read().strip()

Why not simply

MIRROR_URL = os.popen("sage-download-file --print-fastest-mirror").read().strip()

[6bdad4c1-1e26-4f2f-a442-a01a2292c181]

comment:38

Your last changes made me notice this:

pname = re.sub("(\.|-)p[0-9]+$", "", pname) # strip .p0
pname = re.sub("(\.|-)[0-9].*", "", pname) # strip version number

That's actually too complicated and also wrong. As far as I know, all other places in Sage handling packages just split on the first - and that should be done here also.

You already claimed that on another ticket, and were answered with counter-examples: #18408 comment:20

Nathann

[jdemeyer]

comment:39

Instead of

local_non_filtered = os.listdir(SAGE_PKGS)
local = {}
for p in local_non_filtered:

the following is easier to understand:

local = {}
for p in os.listdir(SAGE_PKGS):

[jdemeyer]

comment:40

Replying to @nathanncohen:

You already claimed that on another ticket, and were answered with counter-examples: #18408 comment:20

1. I believe those counter-examples are gone now (but correct me if I'm wrong)
2. Splitting package names from versions should be done consistently, it's not that every script can independently decide how to do that. In particular, I find it an obvious requirement that the package name reported by ./sage --optional should be accepted by ./sage -i.

[6bdad4c1-1e26-4f2f-a442-a01a2292c181]

comment:41

1. I believe those counter-examples are gone now (but correct me if I'm wrong)

http://mirror.switch.ch/mirror/sagemath//spkg/optional/list

Look for 'stein'

2. Splitting package names from versions should be done consistently, it's not that every script can independently decide how to do that. In particular, I find it an obvious requirement that the package name reported by ./sage --optional should be accepted by ./sage -i.

I agree, but that's not my point. My point is that you try to enforce a standard which is not respected by Sage. If you wish for all packages to be easily split into [name,version] by splitting the first '-' (which also means no '-' should appear in their name), I totally agree with you. But it should be done this way: 1) Fix the names 2) Add a consistency check somewhere to make sure it does not break in the future 3) Change the regexp

Nathann

[jdemeyer]

comment:42

Replying to @nathanncohen:

If you wish for all packages to be easily split into [name,version] by splitting the first '-'

That's not my wish, that how sage-fix-pkg-checksums and ./sage -i already handle packages.

Note that

./sage -i database_jones_numfield

does try to install the package database_jones_numfield-v4, so you cannot claim that the name of the package is "database_jones_numfield-v4" without a version number.

Conversely,

./sage -i database_stein_watkins_mini

cannot install the old-style package "database_stein_watkins_mini.p0" (even if you remove build/pkgs/database_stein_watkins_mini) because the .p0 is not seen as a version number (and I don't think it should!)