Opened 12 years ago
Closed 11 years ago
#11602 closed defect (fixed)
install_scripts should use "$@" instead of $*
Reported by: | Stefan | Owned by: | jason |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7.2 |
Component: | misc | Keywords: | install_scripts, hg, command line |
Cc: | drkirkby, leif | Merged in: | sage-4.7.2.alpha2 |
Authors: | John Palmieri | Reviewers: | Leif Leonhardy |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
When using a script created by Sage's install_scripts() function, parameters with spaces cause problems. Example:
From sage: sage: os.mkdir("temp") sage: install_scripts("temp") sage: quit From shell: $ cd temp $ mkdir hgtest $ cd hgtest $ ../hg init $ ../hg commit -m"Two words" abort: words: No such file or directory $ ../hg commit -m"Oneword" nothing changed
Apparently this was discussed on the mailing list back in 2008: https://groups.google.com/forum/#!msg/sage-devel/oeFrvqWiP_s/o8mKO-4OAKkJ
It seems that the solution is to make install_scripts write "$@" instead of $* .
Apply only trac_11602-install-scripts.v4.patch.
Attachments (6)
Change History (37)
comment:1 Changed 12 years ago by
Cc: | drkirkby leif added |
---|
comment:2 follow-up: 4 Changed 12 years ago by
which isn't portable; one should use the shell built-in command -v ....
Or use the function have_program
from sage.misc.sage_ostools
, which in turn uses command -v
.
comment:3 Changed 12 years ago by
Meanwhile... ;-)
Replying to leif:
which
isn't portable; one should use the shell built-incommand -v ...
.
You can replace
p = Popen(['which', c], stdout=PIPE, stderr=PIPE)
by
p = Popen("command -v %s"%c, shell=True, stdout=PIPE, stderr=PIPE)
comment:4 Changed 12 years ago by
Replying to jhpalmieri:
which isn't portable; one should use the shell built-in command -v ....
Or use the function
have_program
fromsage.misc.sage_ostools
, which in turn usescommand -v
.
Ok, in principle better, but we also want the result (the command or more precisely its path), to check whether it's located inside the Sage installation.
Extending sage.misc.sage_ostools
is IMHO the way to go...
comment:5 follow-up: 6 Changed 12 years ago by
I assume that, when this script gets executed, we're within an environment like the "sage -sh" shell? Then the $SAGE_ROOT/local/bin directory is in the path before any of the system directories, so it will ALWAYS come back with the Sage version of the tools, UNLESS Sage doesn't have them. In the latter case, there's no need to make a shortcut; in the former, a shortcut will ALWAYS be made. I see no way of finding a system install separately...
Hence the suggestion to force an install would not change things; I'll modify the docstring to reflect this.
comment:6 Changed 12 years ago by
Replying to Stefan:
I assume that, when this script gets executed, we're within an environment like the "sage -sh" shell?
Not necessarily, so I wouldn't change that, or rather fix the baviour such that it matches the description, not the other way around.
I see no way of finding a system install separately...
Parameter env
...
comment:7 Changed 12 years ago by
John perhaps knows better from where this (rather plain Python) script is or can be called; it actually (currently) just requires SAGE_ROOT
to be set (otherwise an exception is raised, which is a bit odd by itself).
I haven't found something like sage -install_scripts
though.
Also, not all sage -<program>
instances need to have a corresponding $SAGE_ROOT/local/bin/<program>
; at least the filename might differ.
comment:8 Changed 12 years ago by
Status: | new → needs_review |
---|
I noticed this issue, too, and wrote a patch for it. I think that since this is in a file in the Sage library, we may assume that it is used while Sage is running, so all of the appropriate environment variables are set.
Anyway, here's one attempt at a patch.
comment:9 Changed 12 years ago by
The following is from README.txt in the root directory:
6. OPTIONAL: Start Sage and run the command install_scripts("/usr/local/bin/") # change /usr/local/bin/ Type "install_scripts?" in Sage for more details about what this command does.
comment:10 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
Ahem,
- you really have to add the quotes around
$@
since this has a different meaning, namely quoting each element of$@
. command
is a shell built-in, and hence not an executable to be found alongPATH
, so we needshell=True
and a single string containingcommand -v ...
rather than a list of arguments.- Removing
$SAGE_ROOT/local/bin
fromPATH
should IMHO be more robust w.r.t. redundant slashs andnormpath()
orrealpath()
.
comment:11 Changed 12 years ago by
P.S.:
Also, if ignore_existing==True
and some system-wide version of a program exists, a shortcut will be installed regardless of whether a / the Sage version to which the shortcut is made exists.
comment:12 Changed 12 years ago by
P.P.S.: I'd also move the close()
right before "Created script ..."
.
comment:13 follow-up: 15 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
Replying to leif:
you really have to add the quotes around
$@
since this has a different meaning, namely quoting each element of$@
.
Sorry, I missed the quotes in your original comment
command
is a shell built-in, and hence not an executable to be found alongPATH
, so we needshell=True
and a single string containingcommand -v ...
rather than a list of arguments.
On OS X, it works with shell=False
. I see now that on linux, it doesn't.
- Removing
$SAGE_ROOT/local/bin
fromPATH
should IMHO be more robust w.r.t. redundant slashs andnormpath()
orrealpath()
.
I guess on POSIX systems, the directories are always separated by colons. (If not, are there any functions in the Python library for parsing PATH and then reassembling it? Is there a function or variable saying what the separator is?) So the new patch fixes this. (In this case, anyway, the relevant portion of the PATH variable is set by sage-env, which uses the value of SAGE_ROOT, so "SAGE_ROOT/local/bin" should be there as is. This seems to be the case on several different machines in which my Sage install is buried under various symbolic links.)
Also, if ignore_existing==True and some system-wide version of a program exists, a shortcut will be installed regardless of whether a / the Sage version to which the shortcut is made exists.
Right. See the new version.
I'd also move the close() right before "Created script ...".
Okay.
comment:14 Changed 12 years ago by
Ah, found it. Bad variable names, I think: it's too easy to confuse these.
>>> import os >>> os.pathsep ':' >>> os.path.sep '/'
comment:15 Changed 12 years ago by
Replying to jhpalmieri:
command
is a shell built-in, and hence not an executable to be found alongPATH
, so we needshell=True
and a single string containingcommand -v ...
rather than a list of arguments.On OS X, it works with
shell=False
. I see now that on linux, it doesn't.
There can be a real command command
of course, but POSIX only requires the shell built-in.
- Removing
$SAGE_ROOT/local/bin
fromPATH
should IMHO be more robust w.r.t. redundant slashs andnormpath()
orrealpath()
.[...] (In this case, anyway, the relevant portion of the PATH variable is set by sage-env, which uses the value of SAGE_ROOT, so "SAGE_ROOT/local/bin" should be there as is. This seems to be the case on several different machines in which my Sage install is buried under various symbolic links.)
I wouldn't rely on sage-env
, not just because SAGE_ROOT
might already be set.
I'd rather search PATH
for */local/bin*
entries and compare realpath()
of them to realpath(os.path.join(SAGE_ROOT, 'local', 'bin'))
.
(Haven't yet looked at the new patch though.)
Replying to jhpalmieri:
Ah, found it.
> >>> import os > >>> os.pathsep > ':'
Yep. On M$ Windows, it is ";
".
comment:16 follow-up: 17 Changed 12 years ago by
Ok, install_scripts()
now (IMHO unfortunately) requires more of Sage, which prevents usage from a simple shell script, but the problem with an already set SAGE_ROOT
seems to be solved as far as I can see. (The "useless" os.path.exists()
is a bit odd, but I see os.path.samefile()
requires this.)
The stderr=PIPE
and stdout=PIPE
in have_program()
are quite useless; there apparently is no such thing like DEVNULL
, but we can redirect both to /dev/null
in the string passed to the shell (instead of opening /dev/null
and passing its fileno()
to subprocess.call()
). Redirecting stdout
should suffice anyway.
The description of ignore_existing
appears clumsy (and potentially misleading) to me; I'd just say "If True, install a shortcut to Sage's version of a program even if a system-wide version exists [or: is in the/your PATH].".
Maybe we should also mention that the destination directory has to be added to PATH
unless it is already contained in it. (We could even check this in install_scripts()
.)
There btw. is a typo in the docstring, s/
verbosely tell/
verbosely tells/
(and ignore_existsing
[sic] in a comment). For better readability, I'd also substitute c
by e.g. cmd
in the code.
Also, the message "The command ... is not available ..." should read "... not available in Sage". The ìf-elif-else
is IMHO a bit inappropriate, as the otherwise very verbose script doesn't tell if a system-wide version was found but we install a shortcut because ignore_existing==True
. It could be:
if not cmd_inside_sage: # We *may* also report if a system-wide version is available. ... continue if cmd_outside_sage: print "The command '%s' is installed outside of Sage." % cmd if not ignore_existing: # ambiguous unless one reads the docstring print "Not installing a shortcut to Sage's version." continue # normal operation, simply install the shortcut unless a file with # the same name already exists in the target directory ...
comment:17 Changed 12 years ago by
Replying to leif:
Ok,
install_scripts()
now (IMHO unfortunately) requires more of Sage, which prevents usage from a simple shell script, but the problem with an already setSAGE_ROOT
seems to be solved as far as I can see. (The "useless"os.path.exists()
is a bit odd, but I seeos.path.samefile()
requires this.)
Yes, and on OS X, there is at least one nonexistent item in the PATH, because of these lines in sage-env:
if [ `uname` = "Darwin" ]; then # For a framework Python build on OS X, Python's bin directory is not local/bin PATH="$SAGE_LOCAL/Frameworks/Python.framework/Versions/2.5/bin:$PATH" && export PATH
Note the "2.5". (And of course, someone might have some non-existent directories in their path to start with.)
The
stderr=PIPE
andstdout=PIPE
inhave_program()
are quite useless; there apparently is no such thing likeDEVNULL
, but we can redirect both to/dev/null
in the string passed to the shell (instead of opening/dev/null
and passing itsfileno()
tosubprocess.call()
). Redirectingstdout
should suffice anyway.
I don't see any problem with using PIPE to suppress output. Having redirections as part of the shell command seems somehow less elegant to me.
The description of
ignore_existing
appears clumsy (and potentially misleading) to me; I'd just say "If True, install a shortcut to Sage's version of a program even if a system-wide version exists [or: is in the/your PATH].".
Okay.
Maybe we should also mention that the destination directory has to be added to
PATH
unless it is already contained in it. (We could even check this ininstall_scripts()
.)
I've added some messages about this, both in the docstring and the message printed as the function finishes.
There btw. is a typo in the docstring,
s/
verbosely tell/
verbosely tells/
(andignore_existsing
[sic] in a comment). For better readability, I'd also substitutec
by e.g.cmd
in the code.
Okay.
Also, the message "The command ... is not available ..." should read "... not available in Sage". The
ìf-elif-else
is IMHO a bit inappropriate, as the otherwise very verbose script doesn't tell if a system-wide version was found but we install a shortcut becauseignore_existing==True
. It could be:
if not cmd_inside_sage: # We *may* also report if a system-wide version is available. ... continue if cmd_outside_sage: print "The command '%s' is installed outside of Sage." % cmd if not ignore_existing: # ambiguous unless one reads the docstring print "Not installing a shortcut to Sage's version." continue # normal operation, simply install the shortcut unless a file with # the same name already exists in the target directory ...
I changed it to be something like this.
Changed 12 years ago by
Attachment: | trac_11602-install-scripts.patch added |
---|
comment:18 follow-up: 19 Changed 12 years ago by
Authors: | → John Palmieri |
---|---|
Reviewers: | → Leif Leonhardy |
Status: | needs_review → positive_review |
Sorry, completely forgot this ticket.
IMHO works as advertised with Sage 4.7.1.rc0, so positive review.
Only a few minor things:
- Somehow the empty line is removed from the docstring when I type
install_scripts?
:will need write permissions on ``directory``. Note that one good candidate for ``directory`` is "/usr/local/bin".Running ``install_scripts(directory)`` will be most helpful if
(Also,"/usr/local/bin"
should perhaps be double-backquoted.) - You could add yourself to
AUTHORS
. :-) - I'd quote the
directory
name in the last message. (Also, rerunning the command may make sense if a newer version of Sage ships more components, or the user later installs some optional component. Similarly, a system-wide version might vanish later if it gets uninstalled or thePATH
changes.) - I personally don't like raising
RuntimeError
s for rather "expected" error conditions; I thought there was a way to at least suppress the traceback, but I currently don't recall how. (I did get "RuntimeError
doesn't take keyword arguments" when I tried... ;-) )
If there's no way to suppress the tracebacks, I would catch "expected" exception(s) and usesys.stderr.write()
, then proceed with whatever is appropriate in that situation (e.g.return
).
(There also could be a "Creating script ..." message in advance, since an error ino.open()
oro.write()
could otherwise be associated with "Checking that Sage has the command ...".) - An
else:
is superfluous whenever theif
-branch ends with e.g.continue
, but that's partially a matter of taste. (Omitting it here appears more readable to me, but I don't mind.)
Not that important here, but in general you should pass the whole environment to subprocess.call()
with just the variable(s) modified you have to change, as opposed to passing an "empty" environment with just these new settings added (like you do now in have_program()
). Otherwise settings like e.g. LD_LIBRARY_PATH
get lost, which might lead to errors or undesired behaviour.
comment:19 follow-up: 22 Changed 12 years ago by
Status: | positive_review → needs_work |
---|
Replying to leif:
Only a few minor things:
I've attached trac_11602-install-scripts.v2.patch to deal with these; trac_11602-delta.patch shows the differences between the two versions.
- Somehow the empty line is removed from the docstring when I type
install_scripts?
:
Maybe the .. note::
markup can't handle more than one paragraph, or our sphinxified text version of the docstring can't. Anyway, I broke it into two notes.
(Also,
"/usr/local/bin"
should perhaps be double-backquoted.)
I quoted it both ways, so it's a backquoted string.
- You could add yourself to
AUTHORS
. :-)
Okay.
- I'd quote the
directory
name in the last message.
Okay
(Also, rerunning the command may make sense if a newer version of Sage ships more components, or the user later installs some optional component. Similarly, a system-wide version might vanish later if it gets uninstalled or the
PATH
changes.)
I think it's best to leave this as is. I can't think of a way to phrase it which clarifies things and which isn't too complicated. We could add a phrase like "All things being equal" at the beginning, but I think it's best unchanged.
- I personally don't like raising
RuntimeError
s for rather "expected" error conditions; I thought there was a way to at least suppress the traceback, but I currently don't recall how. (I did get "RuntimeError
doesn't take keyword arguments" when I tried... ;-) )
I think OSError
is better. That's more consistent with the behavior of the os.path
module, for example.
(There also could be a "Creating script ..." message in advance, since an error in
o.open()
oro.write()
could otherwise be associated with "Checking that Sage has the command ...".)
Okay.
- An
else:
is superfluous whenever theif
-branch ends with e.g.continue
, but that's partially a matter of taste. (Omitting it here appears more readable to me, but I don't mind.)
I omitted it.
I made a few other changes: it now checks to see if any scripts were written at all. If not, then the ending message doesn't say "Finished creating scripts", etc. Also, the original docstring (and the function name itself) talked about "scripts", while all of the messages printed during execution talked about "shortcuts". I changed it so now they all talk about "scripts" only.
comment:20 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
comment:21 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:22 follow-up: 24 Changed 12 years ago by
Replying to jhpalmieri:
I've attached trac_11602-install-scripts.v2.patch to deal with these; trac_11602-delta.patch shows the differences between the two versions.
I'm almost happy with the delta patch, should test the new version though.
(Also,
"/usr/local/bin"
should perhaps be double-backquoted.)I quoted it both ways, so it's a backquoted string.
Just double-backquotes (monospaced font) would have sufficed.
I made a few other changes: [...] Also, the original docstring (and the function name itself) talked about "scripts", while all of the messages printed during execution talked about "shortcuts". I changed it so now they all talk about "scripts" only.
Well, the notion of shortcuts (or better shortcut scripts) in the description / docstring isn't all bad, as it emphasizes their purpose and perhaps connotes their "light-weightness".
The docstring actually lacks a description of what a shortcut [script] is or really / technically does; an example saying that the scripts make e.g. gp
an alias for sage -gp
, which starts the stand-alone PARI/GP interpreter, would be helpful.
When talking about software components, I'd use their name rather than the corresponding command, i.e. GAP, Maxima, Singular, PARI/GP, MWrank (?), Mercurial, GNU R etc., without quotes. In contrast, when referring to the commands / options to sage
, I'd typeset them as such (with double-backticks), but also omit single-quotes visible in the HTML documentation.
(I guess William just copied and pasted the contents of the Python list.)
I'd also typeset root
and sage
(lower-case) monospaced in
"You may need to run Sage as root in order to ..."
and perhaps mention sudo
or sudo sage -c "install_scripts(<directory>)"
, or combine it with the next sentence, giving the example
sudo sage -c "install_scripts('/usr/local/bin')"
(Do you want to add a "shortcut script" install_scripts
to $SAGE_ROOT/local/bin/
or $SAGE_ROOT/
? :P )
We should check once in advance that the given directory is writable by the user, right after the presence test, in order to give a nice(r) error message [earlier].
Btw., I still get an ugly traceback if the directory doesn't exist; can we suppress that or do we really have to raise
? (I'd prefer just printing an error message and return
.)
I thought we would advise the user (at the end, if any scripts were created) to add directory
to his/her PATH
in case it isn't already there.
(As you may have noticed, I've meanwhile tested also the v2 a little while writing...)
If you don't want to make further changes, I won't mind and give it positive review as is (unless I missed some severe flaw, which I don't expect).
comment:23 Changed 12 years ago by
P.S.:
As an overkill, install_scripts()
could write
... Checking that Sage has the command 'hg' installed Creating script for Mercurial ('hg')... Created script '/given/bin/directory/hg' ...
etc. :-)
Changed 12 years ago by
Attachment: | trac_11602-install-scripts.v2.patch added |
---|
Changed 12 years ago by
Attachment: | trac_11602-delta2to3.patch added |
---|
for reference only; do not apply
Changed 12 years ago by
Attachment: | trac_11602-install-scripts.v3.patch added |
---|
comment:24 Changed 12 years ago by
Description: | modified (diff) |
---|
Replying to leif:
Just double-backquotes (monospaced font) would have sufficed.
Well, it is a string, so it's nice to explicitly include the single quotes in the html version of the reference manual.
The docstring actually lacks a description of what a shortcut [script] is or really / technically does; an example saying that the scripts make e.g.
gp
an alias forsage -gp
, which starts the stand-alone PARI/GP interpreter, would be helpful.
I expanded this, perhaps too much.
When talking about software components, I'd use their name rather than the corresponding command, i.e. GAP, Maxima, Singular, PARI/GP, MWrank (?), Mercurial, GNU R etc., without quotes. In contrast, when referring to the commands / options to
sage
, I'd typeset them as such (with double-backticks), but also omit single-quotes visible in the HTML documentation.
See the new patch.
I'd also typeset
root
andsage
(lower-case) monospaced in
Okay.
and perhaps mention
sudo
orsudo sage -c "install_scripts(<directory>)"
, or combine it with the next sentence, giving the examplesudo sage -c "install_scripts('/usr/local/bin')"
Okay
(Do you want to add a "shortcut script"
install_scripts
to$SAGE_ROOT/local/bin/
or$SAGE_ROOT/
? :P )
No.
We should check once in advance that the given directory is writable by the user, right after the presence test, in order to give a nice(r) error message [earlier].
Right.
Btw., I still get an ugly traceback if the directory doesn't exist; can we suppress that or do we really have to
raise
? (I'd prefer just printing an error message andreturn
.)
I modified the error messages so they don't raise errors.
I thought we would advise the user (at the end, if any scripts were created) to add
directory
to his/herPATH
in case it isn't already there.
Done, although it will not print anything if directory
is one of the directories added to PATH by sage-env.
As an overkill ...
I didn't do this.
One other change: in the old patch, in every iteration of the loop, we computed PATH and imported have_program
; now this is just done once at the top. (The "delta2to3" patch doesn't contain the relocation of the import command, but it's in the final v3 patch.)
comment:25 Changed 12 years ago by
Status: | needs_review → positive_review |
---|
Ok, have_program()
still runs command -v ...
with an environment just containing PATH
if the path
parameter isn't None
, but that's a minor issue.
It's better than the previous version and much better than the original (and of course fixes the issue of this ticket), so positive review.
comment:26 Changed 12 years ago by
Ah, forgot one thing:
To save one process (yet another shell), we should have written:
#!/bin/sh exec sage -CMD "$@"
Or even with two dashs... ;-)
comment:27 Changed 11 years ago by
I could add these changes:
-
sage/misc/dist.py
diff --git a/sage/misc/dist.py b/sage/misc/dist.py
a b def install_scripts(directory=None, igno 133 133 else: 134 134 o = open(target,'w') 135 135 o.write('#!/bin/sh\n') 136 o.write(' sage-%s "$@"\n'%cmd)136 o.write('exec sage --%s "$@"\n'%cmd) 137 137 o.close() 138 138 print "Created script '%s'"%target 139 139 os.system('chmod a+rx %s'%target) -
sage/misc/sage_ostools.py
diff --git a/sage/misc/sage_ostools.py b/sage/misc/sage_ostools.py
a b def have_program(program, path=None): 34 34 import os 35 35 try: 36 36 if path: 37 # env is a copy of the current environment, so modifying 38 # it won't affect os.environ. 39 env = os.environ.copy() 40 env['PATH'] = path 37 41 return not call('command -v %s' % program, shell=True, 38 stdout=PIPE, env = {'PATH': path})42 stdout=PIPE, env=env) 39 43 else: 40 44 return not call('command -v %s' % program, shell=True, 41 45 stdout=PIPE)
Changed 11 years ago by
Attachment: | trac_11602-install-scripts.v4.patch added |
---|
comment:29 Changed 11 years ago by
Okay, here's a new patch incorporating exactly those changes. I'm leaving the ticket as "positive review", so if you're happy with the new patch, please change the description of the ticket ("Apply ...") to refer to "v4" instead of "v3".
comment:31 Changed 11 years ago by
Merged in: | → sage-4.7.2.alpha2 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Yep, from
sage/misc/dist.py
:So this should be
o.write('sage -%s "$@"\n'%c)
.At first glance, there are some other things that should be changed:
o.close()
).which
isn't portable; one should use the shell built-incommand -v ...
.Moreover:
#!/bin/sh
is ok; for more complex ones, this should be#!/usr/bin/env bash
since on some systems/bin/sh
is very limited or even broken. I wouldn't change it here though, since/bin/sh
is usually a light-weight shell and therefore faster.PATH
. We could at least add a boolean parameter (defaulting toFalse
) to override this behaviour.Btw, you don't have to provide a doctest when fixing this; we'll just review the changes.