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, GitHub, GitLab) | Commit: | 262ba162c108f3fd3037370f7a23bc8288fe2d93 |
Dependencies: | #18456 | Stopgaps: |
Description (last modified by )
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
- Description modified (diff)
comment:2 Changed 6 years ago by
- 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
- Branch set to u/jhpalmieri/versions
comment:4 Changed 6 years ago by
- Commit set to 17bc8fad32020a978d940694edba35b46e81389f
- Status changed from new to needs_review
comment:5 Changed 6 years ago by
- 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: ↓ 7 Changed 6 years ago by
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: ↓ 8 ↓ 9 ↓ 10 Changed 6 years ago by
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
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
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
, useNone
as latest available version.
comment:10 in reply to: ↑ 7 Changed 6 years ago by
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: ↓ 13 Changed 6 years ago by
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 readpackage_version.txt
information. For old-style packages, we don't even know about them in that case. - if
local
is False, still usepackage_version.txt
for new-style packages. For old-style packages, check the servers.
That sounds okay to me.
comment:12 follow-up: ↓ 14 Changed 6 years ago by
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
Replying to jhpalmieri:
Okay, then should we rewrite
sage --standard
(etc.) to just read data frombuild/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 readpackage_version.txt
information. For old-style packages, we don't even know about them in that case.- if
local
is False, still usepackage_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
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
- Commit changed from 17bc8fad32020a978d940694edba35b46e81389f to e9f25000c4427703a4ed53fbf68cabb9a36818c0
Branch pushed to git repo; I updated commit sha1. New commits:
e9f2500 | trac 18581: new function package_versions
|
comment:16 Changed 6 years ago by
- 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
- 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
- Commit changed from e9f25000c4427703a4ed53fbf68cabb9a36818c0 to 9178afcfa4e2b9288aa862dcfd84bc77b33ecabc
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
|
comment:20 Changed 6 years ago by
- 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
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
Instead of running sage --{} --dump
(with {} == "standard"
for example), why don't you directly run
sage-list-packages {} --dump
comment:23 Changed 6 years ago by
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
I would also like to see package_versions()
added to sage.misc.all
comment:25 Changed 6 years ago by
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
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
- Commit changed from 9178afcfa4e2b9288aa862dcfd84bc77b33ecabc to 24c0d0b008a482a3ca1d9fc42e10cd30a06436ff
Branch pushed to git repo; I updated commit sha1. New commits:
24c0d0b | trac 18581: responses to referee's comments
|
comment:28 follow-up: ↓ 31 Changed 6 years ago by
- Commit changed from 24c0d0b008a482a3ca1d9fc42e10cd30a06436ff to caa8301969391828718510a45eab8eface47a717
Branch pushed to git repo; I updated commit sha1. New commits:
caa8301 | trac 18581: fix for 'sage --optional --local'
|
comment:29 Changed 6 years ago by
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 --localdoes 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
- Status changed from needs_work to needs_review
comment:31 in reply to: ↑ 28 Changed 6 years ago by
- 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:
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) [...]
comment:32 Changed 6 years ago by
Nathann, do you have any comments about this ticket?
comment:33 Changed 6 years ago by
- Commit changed from caa8301969391828718510a45eab8eface47a717 to f7dc0cd67123e4316d39444182aebcf4d1204f88
Branch pushed to git repo; I updated commit sha1. New commits:
f7dc0cd | trac 18581: fix header for 'sage-list-packages TYPE --local'
|
comment:34 Changed 6 years ago by
- Commit changed from f7dc0cd67123e4316d39444182aebcf4d1204f88 to 61a40761c68b5b054ff86a0d8d4102556b9dcf58
Branch pushed to git repo; I updated commit sha1. New commits:
61a4076 | trac 18581: stylistic changes in sage-list-packages
|
comment:35 Changed 6 years ago by
- 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: ↓ 38 Changed 6 years ago by
- 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
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: ↓ 40 Changed 6 years ago by
Your last changes made me notice this:
pname = re.sub("(\.|-)p[0-9]+$", "", pname) # strip .p0 pname = re.sub("(\.|-)[0-9].*", "", pname) # strip version numberThat'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
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: ↓ 41 Changed 6 years ago by
Replying to ncohen:
You already claimed that on another ticket, and were answered with counter-examples: http://trac.sagemath.org/ticket/18408#comment:20
- I believe those counter-examples are gone now (but correct me if I'm wrong)
- 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: ↓ 42 Changed 6 years ago by
- 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'
- 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: ↓ 43 Changed 6 years ago by
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: ↓ 44 Changed 6 years ago by
cannot install the old-style package "
database_stein_watkins_mini.p0
" (even if you removebuild/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
comment:44 in reply to: ↑ 43 ; follow-up: ↓ 45 Changed 6 years ago by
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: ↓ 47 Changed 6 years ago by
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
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
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
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
- Commit changed from 61a40761c68b5b054ff86a0d8d4102556b9dcf58 to 262ba162c108f3fd3037370f7a23bc8288fe2d93
comment:50 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:51 follow-up: ↓ 52 Changed 6 years ago by
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: ↓ 53 Changed 6 years ago by
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
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
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
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
- Status changed from needs_review to positive_review
comment:57 Changed 6 years ago by
- Branch changed from u/jhpalmieri/versions to 262ba162c108f3fd3037370f7a23bc8288fe2d93
- Resolution set to fixed
- Status changed from positive_review to closed
The actual change is
src/sage/misc/package.py
(name,version))although the branch includes all of the changes from #18456.