_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')

[jhpalmieri]

The actual change is

src/sage/misc/package.py

@@ -216 +216 @@
     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 +261 @@
     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.

[jdemeyer]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[git]

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

​e9f2500

trac 18581: new function package_versions

[jhpalmieri]

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]

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.

[git]

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]

Okay.

[jdemeyer]

Doesn't this need to be changed also?

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

[jdemeyer]

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]

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

sage-list-packages {} --dump

[jdemeyer]

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]

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

[jdemeyer]

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

[jdemeyer]

Is there a reason why

./sage --optional --local

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

[git]

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

​24c0d0b

trac 18581: responses to referee's comments

[git]

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

​caa8301

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

[jhpalmieri]

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]

Replying to 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]

Nathann, do you have any comments about this ticket?

[git]

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

​f7dc0cd

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

[git]

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

​61a4076

trac 18581: stylistic changes in sage-list-packages

[jhpalmieri]

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]

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]

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

[ncohen]

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: ​http://trac.sagemath.org/ticket/18408#comment:20

Nathann

[jdemeyer]

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]

Replying to ncohen:

You already claimed that on another ticket, and were answered with counter-examples: ​http://trac.sagemath.org/ticket/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.

[ncohen]

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]

Replying to ncohen:

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

[ncohen]

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

Okay, so you are just telling me that "only what follows a '-' character is a version number" precisely because that's what the script handle (which is a bug). And to prove it, you explain to me that "p0" is clearly not a version number.

Can we talk about the list of packages online? You set my patch to 'needs_work' because 'extcode' is not a standard package according to you. But of course, the official list of Sage's package had to be what is listed on the mirrors, don't you think? So how can you claim that 'extcode' is not a standard package when the opposite is stated on our website?

Do you see what's happening here? Double standards. You don't want to fix this problem because you are lazy, yet you expect me to magically 'update the online list of packages' despite knowing that I have absolutely no way to make that happen.

Nathann

[jdemeyer]

Nathann, in #18456 you decided to change

def split_pkgname(name):
    try:
        basename, version = name.split('-', 1)
    except ValueError:
        # Some packages such as currently "csage" aren't versioned, or
        # have non-standard versioning, e.g. "database_stein_watkins_mini.p0".
        basename = name
        version = ""
    return basename, version

to

def pkgname_split(name):
    pname = name.split(' ')[0]
    pname = re.sub("(\.|-)p[0-9]+$","",pname) # strip .p0
    pname = re.sub("(\.|-)[0-9].*","",pname) # strip version number
    return (pname,name[len(pname)+1:])

without justification or giving a reason why the latter is correct (where did those regexes come from?)

Unless you can give me a good reason why you changed that, I'm proposing to undo that change.

[ncohen]

Unless you can give me a good reason why you changed that, I'm proposing to undo that change.

Case-by-case analysis of all packages. It works, for all pacakges that we have. In a name that ends by .p0, there is no denying that .p0 is not part of the package's name but indicates a version. Do you think that it will lead you anywhere to deny something like that?

Nathann

[ncohen]

​http://mirror.switch.ch/mirror/sagemath/spkg/experimental/

Example: mayavi_2.2.1.spkg with your parsing, the package's name is 'mayavi_2.2.1' and there is no version number.

Nathann

[jdemeyer]

Replying to ncohen:

Unless you can give me a good reason why you changed that, I'm proposing to undo that change.

Case-by-case analysis of all packages. It works, for all packages that we have. In a name that ends by .p0, there is no denying that .p0 is not part of the package's name but indicates a version. Do you think that it will lead you anywhere to deny something like that?

It's not a matter of denial, it's more a matter of consistency. What's the point of listing a package FOO in the output of ./sage --optional if ./sage -i FOO cannot install that package?

Instead of making up contrived regular expressions to deal with wrong package names, let's just fix the packages instead. The package database_stein_watkins_mini.p0 is already fixed by a new-style package and we should just remove the wrongly-named package.

[jhpalmieri]

Some data: if I just split on the first hyphen, I think the only packages where the versions come out differently are

database_jones_numfield-v4   is 'v4' the version number?
database_stein_watkins_mini  already fixed
mayavi_2.2.1                 is it mayavi, v 2.2.1, or mayavi_2.2.1, no version?
reallib3-linux-20060728      is 'linux' part of the version number?

The Jones database hasn't been updated since 2007, MayaVi2 hasn't been updated since 2008, and reallib3-linux-20060728 since 2006. Based on this, I find Jeroen's argument more convincing. (I mean, I find the argument for consistency in version-checking convincing to start with, and the fact that there are only a few old exceptions makes Nathann's counterargument less persuasive.)

There is a separate issue that the servers seem to be a mess. Moving packages from old-style to new-style will help make that less of a problem.

[git]

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

​dca165c	trac 18581: minor clean up in sage-list-packages
​262ba16	trac 18581: determine version number for package by splitting at first hyphen

[jdemeyer]

This is good for me, but I would like to give Nathann a chance to give his opinion.

[ncohen]

This is good for me, but I would like to give Nathann a chance to give his opinion.

I believe that you are breaking something that works. If you want to set a new standard (it is not currently enforced), fix the package's name first and then write some code to enforce the standard.

Nathann

[jdemeyer]

Replying to ncohen:

This is good for me, but I would like to give Nathann a chance to give his opinion.

I believe that you are breaking something that works.

What worked before that I am breaking?

If you want to set a new standard (it is not currently enforced), fix the package's name first and then write some code to enforce the standard.

The current standard is that the full package name is split into a package-version pair by the first hyphen. This is probably not documented anywhere, but that's the current practice. This is the standard followed by ./sage -i (*), by sage-fix-pkg-checksums and by sage-list-packages (until recently when you changed it).

I am pretty sure that no script ever considered mayavi_2.2.1 as the package mayavi_2 with version 2.1, so I'm wondering why you made it that way in #18456.

(*) To be more precise, ./sage -i splits by any hyphen, not just the first hyphen.

[ncohen]

I am spending a nice evening with a nice girl, and I don't want to have this conversation again. Have fun, Nathann

[jhpalmieri]

The naming conventions for old-style spkgs are described ​here, and they insist that there is a hyphen separating the package name from the version.