Opened 6 years ago

Closed 6 years ago

#18581 closed enhancement (fixed)

_package_lists_from_sage_output() should output installed and available versions

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.8
Component: misc Keywords:
Cc: ncohen Merged in:
Authors: John Palmieri Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 262ba16 (Commits) Commit: 262ba162c108f3fd3037370f7a23bc8288fe2d93
Dependencies: #18456 Stopgaps:

Description (last modified by 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')

Change History (57)

comment:1 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 6 years ago by jdemeyer

  • Summary changed from standard_packages()... should output installed and available versions to _package_lists_from_sage_output() should output installed and available versions

comment:3 Changed 6 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/versions

comment:4 Changed 6 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Commit set to 17bc8fad32020a978d940694edba35b46e81389f
  • Status changed from new to needs_review

The actual change is

  • src/sage/misc/package.py

    diff --git a/src/sage/misc/package.py b/src/sage/misc/package.py
    index 91ab594..6a9f33d 100644
    a b def _package_lists_from_sage_output(package_type,version=False,local=False): 
    216216    When ``version=False``, the function returns a pair of list
    217217    ``(installed,not_installed)`` with the corresponding packages' name.
    218218
    219     When ``version=True``, the elements of each list are not package names but
    220     pairs ``(package_name,package_version)`` where ``version`` is a string
    221     representing the version of each package that is installed.
     219    When ``version=True``, the elements of each list are not package
     220    names but either pairs ``(package_name, installed_version)`` or
     221    triples ``(package_name, latest_version, installed_version)``,
     222    depending on whether ``local`` is ``True`` or ``False``. Here,
     223    ``latest_version`` is a string representing the version of the
     224    package available on the Sage package server, while
     225    ``installed_version`` is the version that is actually installed.
    222226
    223227    EXAMPLE::
    224228
    def _package_lists_from_sage_output(package_type,version=False,local=False): 
    257261    if version:
    258262        for line in X:
    259263            line = line.split(' ')
    260             name,version = line[0],line[1]
    261             if is_package_installed(name):
    262                 installed.append((name,version))
     264            if is_package_installed(line[0]):
     265                installed.append(tuple(line))
    263266            else:
    264                 not_installed.append((name,version))
     267                not_installed.append(tuple(line))
    265268    else:
    266269        for name in X:
    267270            if is_package_installed(name):

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

comment:5 Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to needs_work

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.

comment:6 follow-up: Changed 6 years ago by 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.

comment:7 in reply to: ↑ 6 ; follow-ups: Changed 6 years ago by 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?

comment:8 in reply to: ↑ 7 Changed 6 years ago by 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?

comment:9 in reply to: ↑ 7 Changed 6 years ago by 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.

comment:10 in reply to: ↑ 7 Changed 6 years ago by 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.

comment:11 follow-up: Changed 6 years ago by 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.

comment:12 follow-up: Changed 6 years ago by 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.

comment:13 in reply to: ↑ 11 Changed 6 years ago by 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.

comment:14 in reply to: ↑ 12 Changed 6 years ago by 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.

comment:15 Changed 6 years ago by git

  • Commit changed from 17bc8fad32020a978d940694edba35b46e81389f to e9f25000c4427703a4ed53fbf68cabb9a36818c0

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

e9f2500trac 18581: new function package_versions

comment:16 Changed 6 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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.

comment:17 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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.

comment:18 Changed 6 years ago by git

  • Commit changed from e9f25000c4427703a4ed53fbf68cabb9a36818c0 to 9178afcfa4e2b9288aa862dcfd84bc77b33ecabc

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

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

comment:19 Changed 6 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

Okay.

comment:20 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Doesn't this need to be changed also?

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

comment:21 Changed 6 years ago by 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)

comment:22 Changed 6 years ago by jdemeyer

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)

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:23 Changed 6 years ago by 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)

comment:24 Changed 6 years ago by jdemeyer

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

comment:25 Changed 6 years ago by jdemeyer

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

comment:26 Changed 6 years ago by jdemeyer

Is there a reason why

./sage --optional --local

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

comment:27 Changed 6 years ago by git

  • Commit changed from 9178afcfa4e2b9288aa862dcfd84bc77b33ecabc to 24c0d0b008a482a3ca1d9fc42e10cd30a06436ff

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

24c0d0btrac 18581: responses to referee's comments

comment:28 follow-up: Changed 6 years ago by git

  • Commit changed from 24c0d0b008a482a3ca1d9fc42e10cd30a06436ff to caa8301969391828718510a45eab8eface47a717

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

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

comment:29 Changed 6 years ago by 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.

comment:30 Changed 6 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

comment:31 in reply to: ↑ 28 Changed 6 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

Replying to git:

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

caa8301trac 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)
[...]

comment:32 Changed 6 years ago by jdemeyer

Nathann, do you have any comments about this ticket?

comment:33 Changed 6 years ago by git

  • Commit changed from caa8301969391828718510a45eab8eface47a717 to f7dc0cd67123e4316d39444182aebcf4d1204f88

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

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

comment:34 Changed 6 years ago by git

  • Commit changed from f7dc0cd67123e4316d39444182aebcf4d1204f88 to 61a40761c68b5b054ff86a0d8d4102556b9dcf58

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

61a4076trac 18581: stylistic changes in sage-list-packages

comment:35 Changed 6 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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.

comment:36 follow-up: Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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.

comment:37 Changed 6 years ago by 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()

comment:38 in reply to: ↑ 36 ; follow-up: Changed 6 years ago by 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

comment:39 Changed 6 years ago by 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):

comment:40 in reply to: ↑ 38 ; follow-up: Changed 6 years ago by 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.

comment:41 in reply to: ↑ 40 ; follow-up: Changed 6 years ago by 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'

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

comment:42 in reply to: ↑ 41 ; follow-up: Changed 6 years ago by 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!)

comment:43 in reply to: ↑ 42 ; follow-up: Changed 6 years ago by 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

Last edited 6 years ago by ncohen (previous) (diff)

comment:44 in reply to: ↑ 43 ; follow-up: Changed 6 years ago by 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.

comment:45 in reply to: ↑ 44 ; follow-up: Changed 6 years ago by 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

comment:46 Changed 6 years ago by 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

comment:47 in reply to: ↑ 45 Changed 6 years ago by 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.

comment:48 Changed 6 years ago by 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.

comment:49 Changed 6 years ago by git

  • Commit changed from 61a40761c68b5b054ff86a0d8d4102556b9dcf58 to 262ba162c108f3fd3037370f7a23bc8288fe2d93

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

dca165ctrac 18581: minor clean up in sage-list-packages
262ba16trac 18581: determine version number for package by splitting at first hyphen

comment:50 Changed 6 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

comment:51 follow-up: Changed 6 years ago by jdemeyer

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

comment:52 in reply to: ↑ 51 ; follow-up: Changed 6 years ago by 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

comment:53 in reply to: ↑ 52 Changed 6 years ago by 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.

comment:54 Changed 6 years ago by ncohen

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

comment:55 Changed 6 years ago by 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.

comment:56 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:57 Changed 6 years ago by vbraun

  • Branch changed from u/jhpalmieri/versions to 262ba162c108f3fd3037370f7a23bc8288fe2d93
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.