#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: |
Description (last modified by )
Attachments (3)
Change History (21)
comment:1 Changed 10 years ago by
- Status changed from new to needs_work
comment:2 Changed 10 years ago by
- Description modified (diff)
comment:3 Changed 10 years ago by
- Status changed from needs_work to needs_review
comment:4 follow-up: ↓ 6 Changed 10 years ago by
Reorganization of -help and -advanced looks real good! I prefer the topical organization versus the alphabetical listing. Some comments, questions, suggestions to consider.
- 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 | ----------------------------------------------------------------------
- On help for running the notebook, I would use "for" rather than "to"
(options are the same as to the notebook command in Sage)
- 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)
- 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
- Description modified (diff)
comment:6 in reply to: ↑ 4 Changed 10 years ago by
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
- 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
- Reviewers set to Rob Beezer
comment:9 follow-up: ↓ 11 Changed 10 years ago by
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
echo
s? (Perhaps more than one perusage()
though.)
- What about using the shell's pattern matching for option parsing? (This is more readable, avoids problems with potentially broken
test
s, 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 insys.argv
), so the usage issage 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
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.
comment:11 in reply to: ↑ 9 Changed 10 years ago by
- 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
- Status changed from needs_info to needs_review
comment:13 follow-up: ↓ 14 Changed 10 years ago by
- 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
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: ↓ 17 Changed 10 years ago by
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
- 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: ↓ 18 Changed 10 years ago by
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 fortunately^{TM} 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
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
I'm still reworking the help output