Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#10326 closed enhancement (fixed)

Various clean-up in local/bin/sage-sage

Reported by: jdemeyer Owned by: jason
Priority: minor Milestone: sage-4.6.1
Component: misc Keywords: scripts sage-sage usage help messages
Cc: Merged in: sage-4.6.1.alpha3
Authors: Jeroen Demeyer Reviewers: Rob Beezer, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Attachments (3)

10326_sagelib.patch (2.3 KB) - added by jdemeyer 10 years ago.
SAGELIB patch
10326_cleanup_sage_sage.patch (29.9 KB) - added by jdemeyer 10 years ago.
SCRIPTS patch
10326_help2.patch (5.5 KB) - added by jdemeyer 10 years ago.
SCRIPTS patch, apply on top of previous

Download all attachments as: .zip

Change History (21)

comment:1 Changed 10 years ago by jdemeyer

  • Status changed from new to needs_work

I'm still reworking the help output

comment:2 Changed 10 years ago by jdemeyer

  • Description modified (diff)

Changed 10 years ago by jdemeyer

SAGELIB patch

comment:3 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:4 follow-up: Changed 10 years ago by rbeezer

Reorganization of -help and -advanced looks real good! I prefer the topical organization versus the alphabetical listing. Some comments, questions, suggestions to consider.

  1. The change in the banner listing means the notice about being a "prerelease" version is no longer included in the help output. Was this intended? Unpatched:
    ----------------------------------------------------------------------
    | Sage Version 4.6.1.alpha2, Release Date: 2010-11-21                |
    * Warning: this is a prerelease version, and it may be unstable.     *
    ----------------------------------------------------------------------
    

Patched:

----------------------------------------------------------------------
| Sage Version 4.6.1.alpha2, Release Date: 2010-11-21                |
----------------------------------------------------------------------
  1. On help for running the notebook, I would use "for" rather than "to"
(options are the same as to the notebook command in Sage)
  1. There are tests to see if various packages/add-ons are installed and no help is given of they are not available. I think I would prefer to see all available options and then a message saying when an add-on is not avaailable. This way people could see what is possible and install the add-ons if they want them. So, for example, suppose kash is not installed, then maybe output could be something like:
-kash [...]         -- run Sage's Kash with given arguments
                    (not installed currently, run  sage -i kash)
  1. Applied in local/bin on 4.6.1.alpha2 and got 1 failure. Didn't really investigate any further. And I just now see I failed to notice the dependency on #10300 (and precursors), so likely that is the cause.
rob@wave:/sage/dev/local/bin$ hg qimport -P http://trac.sagemath.org/sage_trac/raw-attachment/ticket/10326/10326_cleanup_sage_sage.patch
adding 10326_cleanup_sage_sage.patch to series file
applying 10326_cleanup_sage_sage.patch
patching file sage-sage
Hunk #9 FAILED at 498
1 out of 18 hunks FAILED -- saving rejects to file sage-sage.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 10326_cleanup_sage_sage.patch
rob@wave:/sage/dev/local/bin$ cat sage-sage.rej
--- sage-sage
+++ sage-sage
@@ -421,15 +499,15 @@
             SHELL_OPTS=" -f -d"
             ;;
         *)
-            echo "Unknown shell: $SHELL!"
-            echo "Exiting Sage."
+            echo >&2 "Unknown shell: $SHELL!"
+            echo >&2 "Exiting Sage."
             exit 1
     esac

     $SHELL_NAME $SHELL_OPTS "$@"

     status=$?
-    echo Exited Sage subshell.
+    echo "Exited Sage subshell."
     exit $status
 fi

comment:5 Changed 10 years ago by jdemeyer

  • Description modified (diff)

Changed 10 years ago by jdemeyer

SCRIPTS patch

comment:6 in reply to: ↑ 4 Changed 10 years ago by jdemeyer

Replying to rbeezer:

Reorganization of -help and -advanced looks real good! I prefer the topical organization versus the alphabetical listing. Some comments, questions, suggestions to consider.

New patch taking into account all your remarks.

comment:7 Changed 10 years ago by rbeezer

  • Status changed from needs_review to positive_review

Suggested changes look real good as implemented - thanks.

Applied all the dependencies and everything applies and runs smoothly. All new tests pass with:

sage -t sage/tests/cmdline.py

Help commands work properly and like I said before, I like the reorganization. The scripting looks good to my eye, but if somebody more experienced wants to take a look, that wouldn't hurt. This was advertised on sage-devel and has been available for several days. I'm going to give this a positive review, and maybe it can sit like that for a day or two before being merged?

comment:8 Changed 10 years ago by jdemeyer

  • Reviewers set to Rob Beezer

comment:9 follow-up: Changed 10 years ago by leif

In any case an improvement, but still room for more... ;-)

  • For educational purposes, what about listing the long options with double-dash [only]? (I don't think we have to mention that Sage also currently supports long options with just a single dash.)
  • What about using "here" documents rather than cascades of echos? (Perhaps more than one per usage() though.)
  • What about using the shell's pattern matching for option parsing? (This is more readable, avoids problems with potentially broken tests, and is in some cases less redundant.)
  • I would list "coverage" under "testing", not "documentation".
  • We could support sage -h <topic> or alike (sage help <topic|option>).

There are some flaws or omissions (can't list them all here, just a few):

  • s/debuging/debugging/
  • -clone doesn't "run" the new branch.
  • -b without a branch (re)builds the current. (And -b main switches back to the "original", hopefully unmodified...)
  • -s is also valid in conjunction with -f (but only as the second argument, like with -i).
  • If you run sage with a file given, the extension is mandatory, and you can also pass further arguments to it (ending up in sys.argv), so the usage is
        sage filename.{sage, py, spyx} [arguments]
    

or

    sage filename.<sage|py|spyx> [arguments]
  • -update and -update-build are obsolete, or at least currently unsupported.
  • -t etc. also takes the additional options -force_lib and -sagenb.
  • -testall also takes additional options, like e.g. -long.
  • ... (I don't recall... ;-) )

comment:10 Changed 10 years ago by leif

P.S.:

Perhaps for a follow-up, we should mention (at least some of the) environment variables important or relevant to a specific topic, e.g. SAGE_TIMEOUT and SAGE_TIMEOUT_LONG under "testing".

(sage -h variables, sage -vars or alike would also be nice, to give a comprehensive list, with a link to the Installation or Developer's Guide. An option for listing the current settings of these could also be helpful.)

We could also mention make targets (and make variables like NUM_THREADS) there, at least the targets for testing and perhaps building the documentation.

Changed 10 years ago by jdemeyer

SCRIPTS patch, apply on top of previous

comment:11 in reply to: ↑ 9 Changed 10 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Replying to leif:

In any case an improvement, but still room for more... ;-)

Leif, I have made a second patch taking into account some of your suggestions, can you review that patch? I think further changes should go to a new ticket.

comment:12 Changed 10 years ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:13 follow-up: Changed 10 years ago by leif

  • Keywords usage help messages added
  • Reviewers changed from Rob Beezer to Rob Beezer, Leif Leonhardy
  • Status changed from needs_review to positive_review

Hoping the other suggestions don't get lost...

Perhaps someone should open a follow-up; I'm currently not going to make changes myself.

Rob, are you ok with the changes? If not, simply revert the status.

Jeroen, perhaps you could add a back-reference in sage-sage to this ticket ("TODO", or "Further suggested changes").

comment:14 in reply to: ↑ 13 Changed 10 years ago by rbeezer

Replying to leif:

Rob, are you ok with the changes? If not, simply revert the status.

Looked over the "help2" patch and it all looks fine to me. I think this one is ready to ship.

Thanks, Jeroen and Leif, for all your work on the Sage infrastructure.

comment:15 follow-up: Changed 10 years ago by rbeezer

OK, I'll bite and be someone. See #10429 for a link back to comment 9 above with Leif's suggestions.

While I have your attention here on-topic - is it possible to do -testall in combination with something like -tp 4 ? I'm always looking to do this when reviewing. I can't see from the code that it is possible. Let me know, and I'll propose it on a ticket if it is not currently possible. Thanks. -Rob

comment:16 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:17 in reply to: ↑ 15 ; follow-up: Changed 10 years ago by leif

Replying to rbeezer:

OK, I'll bite and be someone. See #10429 for a link back to comment 9 above with Leif's suggestions.

Thanks, someone... ;-)


While I have your attention here on-topic - is it possible to do -testall in combination with something like -tp 4 ? I'm always looking to do this when reviewing. I can't see from the code that it is possible.

sage -testall currently doesn't support multiple threads, since there's no separate option to specify the number, just the combined option -tp, which excludes -testall.

But you could (from SAGE_ROOT) run make ptest NUM_THREADS=4 (not ptestall, which includes --optional), which is perhaps less convenient since you have to cd to SAGE_ROOT, or use make's -C option (make -C $SAGE_ROOT ...). Alternatively, do

$ export NUM_THREADS=4   # once per session, or in your ~/.bashrc

$ make -e ptest          # from SAGE_ROOT; '-e' tells 'make' to take NUM_THREADS
                         # from your environment, i.e. override the default setting(s)

Also, the make method doesn't include testing the examples as sage -testall (in theory) does, but fortunatelyTM that isn't implemented at all yet. (SAGE_ROOT/examples/test_all, which is also called when you run sage -testall, just issues a warning and exits.) :)

Let me know, and I'll propose it on a ticket if it is not currently possible. Thanks. -Rob

Feel free to open a ticket for that, suggesting some suitable option to add. (-p <NUM>, --threads=<NUM>, or -j<NUM> like make takes? Note that make -j4 ptest wouldn't work as one perhaps would expect.)

Or we could simply implement sage -tp <NUM> --all, sage -ptestall <NUM>, sage -tp-all <NUM> or whatever. Separating the option to specify the number of threads (and the "all" option) is IMHO a better choice.

We're going to merge the scripts for sequential and parallel testing anyway.

comment:18 in reply to: ↑ 17 Changed 10 years ago by rbeezer

Replying to leif:

Hi Lief,

Been meaning to thank-you for these detailed suggestions. At some point I may open a ticket with some of theses ideas, as suggestions for ways to streamline all these options.

Rob

Note: See TracTickets for help on using tickets.