Opened 5 years ago

Closed 5 years ago

#17155 closed enhancement (fixed)

add "sage -installed" and "sage -pip" commands

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-6.4
Component: scripts Keywords:
Cc: tmonteil Merged in:
Authors: Vincent Delecroix Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 9810c8c (Commits) Commit: 9810c8cbf207320cf607c9769b713c29de678112
Dependencies: Stopgaps:

Description

Add two commands to sage:

  • sage -installed to get the list of installed packages
  • sage -pip to run the Python package manager

Change History (20)

comment:1 Changed 5 years ago by vdelecroix

  • Branch set to u/vdelecroix/17155
  • Commit set to 4bf23308777288b5a829b18b153367ea0b7a2250
  • Status changed from new to needs_review

New commits:

4bf2330trac #17155: add "sage -installed" and "sage -pip" commands

comment:2 Changed 5 years ago by kcrisman

Huh - and the pip would be quite useful. But even in the chunk you show,

if [ "$1" = '-i' ]; then
     shift
     # If there are no further arguments, simply list all installed

This should even be documented somewhere, but maybe it hasn't been advertised well lately? Not that it would be horrible to add this alias, though I don't know if some syntax warriors will complain because you allow -installed while --installed is the current vogue...

comment:3 Changed 5 years ago by vdelecroix

Hello,

Thanks for having a look.

I do not complain that sage -i has not been advertised. But I do that it is documented nowhere (neither in src/bin/sage nor in src/bin/sage-pkg)!

Currently we have the options -optional, -standard and -experimental. I am just adding an additional -installed. We can be smarter and more coherent by allowing multiple arguments and force to use the -i option like in

    $ sage -i --installed --optional --standard
    ... would list all installed package that are either ...
    ... optional or standard                             ...
    $ sage -i --not-installed
    ... would list all non installed packages ...

I am in favour of a more coherent scheme of options, but then we should forbid the older syntax (and instead write a deprecation message that explains the new syntax).

And I definitely do not understand why we do have two scripts (src/bin/sage-spkg and src/bin/sage-list-packages) for dealing with packages. Morever the current state is that sage-spkg is used to list the installed packages while sage-list-packages is the one to list the optional packages...

EDIT: and I just noticed that the sage -i starts with a useless line 'Currently installed packages:'. It is especially bad if you want to use it in other scripts.

Vincent

Last edited 5 years ago by vdelecroix (previous) (diff)

comment:4 Changed 5 years ago by kcrisman

And I definitely do not understand why we do have two scripts

Alas, I can only point things out when it comes to these scripts - I am reluctant to tread on such long-standing traditions. I do hope some others come to look at this, though, since at the very least it is not bad to add -installed.

comment:5 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Use spaces, not TABs for indentation.

Instead of [ ! $(sage -installed | grep -c '^pip-') -eq 1 ], I would go with [ -x "$SAGE_LOCAL/bin/pip" ]

And why not replace echo "Pip is not installed. Run \"sage -i pip\" to install it." by sage -i pip || exit $??

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

comment:6 Changed 5 years ago by git

  • Commit changed from 4bf23308777288b5a829b18b153367ea0b7a2250 to 6ae094b15c7e966a38ca92a280b7974b83c530c7

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

6ae094btrac #17155: reviewer comments

comment:7 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:8 in reply to: ↑ 5 Changed 5 years ago by vdelecroix

Replying to jdemeyer:

Use spaces, not TABs for indentation.

Instead of [ ! $(sage -installed | grep -c '^pip-') -eq 1 ], I would go with [ -x "$SAGE_LOCAL/bin/pip" ]

done. It is much simpler, thanks.

And why not replace echo "Pip is not installed. Run \"sage -i pip\" to install it." by sage -i pip || exit $??

I do not like to do things in a script that users did not ask for.

Vincent

comment:9 Changed 5 years ago by git

  • Commit changed from 6ae094b15c7e966a38ca92a280b7974b83c530c7 to 0ded194dd551dc9a29b1991e0b782e53faa770da

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

0ded194trac #17155: reviewer comments

comment:10 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

If you deprecate something, it should still work (i.e. don't remove the old code, just add the echo statements).

Also, please use echo >&2 for these warnings.

comment:11 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

In --help, you write -installed while only --installed works. I think you should stick to the de facto convention that both single and double dashes should work.

comment:12 Changed 5 years ago by git

  • Commit changed from 0ded194dd551dc9a29b1991e0b782e53faa770da to 31c2fec6204031e9882d278eb2ecd79bbdf74569

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

31c2fectrac #17155: nicer deprecation

comment:13 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:14 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

In sage-list-packages:

  • "standard -> "standard"
  • except A, B -> except A as B
  • The except clause should abort the program otherwise one gets
    Traceback (most recent call last):
      File "/usr/local/src/sage-config/src/bin/sage-list-packages", line 80, in <module>
        available_version = dict([ split_pkgname(pkg) for pkg in get_remote_package_list(category)])
    TypeError: 'int' object is not iterable
    

comment:15 Changed 5 years ago by git

  • Commit changed from 31c2fec6204031e9882d278eb2ecd79bbdf74569 to 9810c8cbf207320cf607c9769b713c29de678112

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

9810c8ctrac #17155: reviewer comments

comment:16 Changed 5 years ago by vdelecroix

  • Status changed from needs_work to needs_review

Thanks for your comments. I did all the modifications in my last commit.

comment:17 Changed 5 years ago by vdelecroix

ping!

comment:18 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:19 Changed 5 years ago by vdelecroix

Thanks.

comment:20 Changed 5 years ago by vbraun

  • Branch changed from u/vdelecroix/17155 to 9810c8cbf207320cf607c9769b713c29de678112
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.