Opened 4 years ago

Closed 4 years ago

#18456 closed enhancement (fixed)

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

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.8
Component: distribution Keywords:
Cc: leif, vbraun, jdemeyer Merged in:
Authors: Nathann Cohen Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: b093007 (Commits) Commit: b09300783d2dffe3b3fa6f4f643861909457e34f
Dependencies: #18431 Stopgaps:

Description (last modified by ncohen)

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

Change History (37)

comment:1 Changed 4 years ago by ncohen

  • Branch set to u/ncohen/18456
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by git

  • Commit set to ebf52de4e4f6e7cb16d1372648617ff401b59d53

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

4770518trac #18431: Type file for each package+define var from pkg name
3efc9fbtrac #18431: package-specific 'dependencies' files
308cefftrac #18431: Auto-generate make rules for standard packages
77f293ftrac #18431: auto-generated Make target "sage-standard-packages"
e114874trac #18431: typo
766798etrac #18431: tab->spaces
47e438dMerge tag '6.7' into t/18441/base_packages_except_configure_should_be_standard
4de8e37Various changes to build system
f83b0c4Correct variable name
ebf52detrac #18456: Re-Fix standard_packages(), optional_packages(), and experimental_packages()

comment:3 Changed 4 years ago by git

  • Commit changed from ebf52de4e4f6e7cb16d1372648617ff401b59d53 to 39cc007aaaf83a6ff0fbd99536c1561486fbfead

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

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

comment:4 follow-up: Changed 4 years ago by cremona

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!

comment:5 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by ncohen

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

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

comment:6 follow-up: Changed 4 years ago by cremona

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.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 4 years ago by ncohen

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

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by cremona

  • Status changed from needs_review to needs_work

Replying to ncohen:

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

comment:9 in reply to: ↑ 8 Changed 4 years ago by ncohen

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

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

comment:10 Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:11 Changed 4 years ago by ncohen

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

comment:12 follow-up: Changed 4 years ago by cremona

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!

comment:13 in reply to: ↑ 12 Changed 4 years ago by ncohen

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

comment:14 Changed 4 years ago by tscrim

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.

comment:15 in reply to: ↑ 5 Changed 4 years ago by jhpalmieri

Replying to ncohen:

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.

comment:16 Changed 4 years ago by ncohen

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

comment:17 follow-up: Changed 4 years ago by jhpalmieri

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

comment:18 in reply to: ↑ 17 Changed 4 years ago by ncohen

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

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

comment:19 follow-up: Changed 4 years ago by ncohen

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

comment:20 Changed 4 years ago by git

  • Commit changed from 39cc007aaaf83a6ff0fbd99536c1561486fbfead to a3402c959a7d74ede7a58ae1d75b17faba0ac52d

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

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

comment:21 in reply to: ↑ 19 Changed 4 years ago by ncohen

add a cool feature: automatic test of (installed) optional packages.

Done at #18558

Nathann

comment:22 follow-up: Changed 4 years ago by jhpalmieri

What do you think about either of these changes?

  • src/bin/sage-list-packages

    diff --git a/src/bin/sage-list-packages b/src/bin/sage-list-packages
    index 5b17272..e1b0889 100755
    a b SAGE_ROOT = os.environ["SAGE_ROOT"] 
    1616# Input parsing #
    1717#################
    1818
    19 parser = argparse.ArgumentParser(description="List Sage's packages")
     19parser = argparse.ArgumentParser(formatter_class=argparse.RawDescriptionHelpFormatter,
     20                                 description="""List Sage's packages, by default in the format
     21
     22  {:.<30} {}
     23
     24""".format("package", "version on-line (version installed)"))
    2025parser.add_argument('category', choices=['standard','optional','experimental', 'installed'], metavar="category",
    2126                    help="The type of packages. Can be 'standard','optional','experimental' or 'installed'.")
    2227parser.add_argument('--dump', dest='dump', default=False, action='store_true',
    if args['remote'] and args['category']!='installed': 
    7681# OUTPUT #
    7782##########
    7883
     84if not args['dump']:
     85    print("{:.<40} {}".format("PACKAGE", "version on-line (version installed)"))
     86    print("-"*76)
     87
    7988# installed
    8089if args['category'] == 'installed':
    8190    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.

Last edited 4 years ago by jhpalmieri (previous) (diff)

comment:23 follow-up: Changed 4 years ago by jhpalmieri

This change is more important.

  • src/bin/sage-list-packages

    diff --git a/src/bin/sage-list-packages b/src/bin/sage-list-packages
    index 5b17272..88cbaab 100755
    a b installed = dict(pkgname_split(pkgname) 
    4651                 for pkgname in os.listdir(SAGE_SPKG_INST))
    4752
    4853# new-style packages
    49 local_non_filtered = os.listdir(SAGE_ROOT+"/build/pkgs/")
     54local_non_filtered = os.listdir(os.path.join(SAGE_ROOT, "build", "pkgs"))
    5055local = {}
    5156for p in local_non_filtered:
    52     with open(SAGE_ROOT+"/build/pkgs/"+p+"/package-version.txt") as f:
     57    with open(os.path.join(SAGE_ROOT, "build", "pkgs", p, "package-version.txt")) as f:
    5358        version = f.read().strip()
    54     with open(SAGE_ROOT+"/build/pkgs/"+p+"/type") as f:
     59    with open(os.path.join(SAGE_ROOT, "build", "pkgs", p, "type")) as f:
    5560        if f.read().strip() == args['category']:
    5661            local[p] = version
    5762

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

Last edited 4 years ago by jhpalmieri (previous) (diff)

comment:24 follow-up: Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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

comment:25 Changed 4 years ago by git

  • Commit changed from a3402c959a7d74ede7a58ae1d75b17faba0ac52d to 687e4261edc550d7521d99bfef9839e401ef522e

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

687e426trac #18456: Column names

comment:26 in reply to: ↑ 22 Changed 4 years ago by ncohen

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

comment:27 Changed 4 years ago by git

  • Commit changed from 687e4261edc550d7521d99bfef9839e401ef522e to d671f1ba7545507e710099a94cb1f07f5ba2047d

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

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

comment:28 in reply to: ↑ 23 Changed 4 years ago by ncohen

This change is more important.

Right. Done.

Nathann

comment:29 in reply to: ↑ 24 ; follow-up: Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

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

comment:30 in reply to: ↑ 29 Changed 4 years ago by jdemeyer

Replying to ncohen:

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

Done: #18581

comment:31 Changed 4 years ago by git

  • Commit changed from d671f1ba7545507e710099a94cb1f07f5ba2047d to 985474957e3f7327e635c7aa13dbf70f510c727f

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

9854749trac #18456: doc and doctest

comment:32 follow-up: Changed 4 years ago by jhpalmieri

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:

  • src/sage/misc/package.py

    diff --git a/src/sage/misc/package.py b/src/sage/misc/package.py
    index 12b556d..91ab594 100644
    a b def _package_lists_from_sage_output(package_type,version=False,local=False): 
    239239        sage: installed, not_installed = _package_lists_from_sage_output('standard',local=True,version=True)
    240240        sage: bool(not_installed)
    241241        False
    242         sage: any(x[0] == 'glpk' for x in installed)
    243         True
    244         sage: type(x[1]) == str and '4.55' <= x[1]
     242        sage: any(x[0] == 'glpk' and type(x[1]) == str and '4.55' <= x[1] for x in installed)
    245243        True
    246244    """
    247245    if package_type not in ['standard','optional','experimental']:

comment:33 Changed 4 years ago by git

  • Commit changed from 985474957e3f7327e635c7aa13dbf70f510c727f to b09300783d2dffe3b3fa6f4f643861909457e34f

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

b093007trac #18456: doc and doctest

comment:34 in reply to: ↑ 32 Changed 4 years ago by ncohen

I get an error with the new doctest:

Rightrightright, thanks! I updated the latest commit.

Nathann

comment:35 Changed 4 years ago by jhpalmieri

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

comment:36 Changed 4 years ago by jdemeyer

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

comment:37 Changed 4 years ago by vbraun

  • Branch changed from u/ncohen/18456 to b09300783d2dffe3b3fa6f4f643861909457e34f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.