install_scripts should use "$@" instead of $*

[sagetrac-Stefan]

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 attachment: trac_11602-install-scripts.v4.patch.

CC: @sagetrac-drkirkby @nexttime

Component: misc

Keywords: install_scripts, hg, command line

Author: John Palmieri

Reviewer: Leif Leonhardy

Merged: sage-4.7.2.alpha2

Issue created by migration from https://trac.sagemath.org/ticket/11602

[nexttime]

comment:1

Yep, from sage/misc/dist.py:

            if os.path.exists(target):
                print "The file '%s' already exists; not adding shortcut"%(target)
            else:
                o = open(target,'w')
                o.write('#!/bin/sh\n')
                o.write('sage -%s $*\n'%c)
                print "Created script '%s'"%target
                os.system('chmod a+rx %s'%target)

So this should be o.write('sage -%s "$@"\n'%c).

At first glance, there are some other things that should be changed:

• The generated file isn't explicitly closed after it was written (i.e., add o.close()).
• which isn't portable; one should use the shell built-in command -v ....

Moreover:

• As long as the generated scripts are trivial, using #!/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.
• I wonder if it's really desirable to not install shortcuts for programs shipped with Sage of which some system-wide version is in the current PATH. We could at least add a boolean parameter (defaulting to False) to override this behaviour.

Btw, you don't have to provide a doctest when fixing this; we'll just review the changes.

[jhpalmieri]

comment:2

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.

[nexttime]

comment:3

Meanwhile... ;-)

Replying to @nexttime:

• which isn't portable; one should use the shell built-in command -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)

[nexttime]

comment:4

Replying to @jhpalmieri:

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.

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

[sagetrac-Stefan]

comment:5

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.

[nexttime]

comment:6

Replying to @sagetrac-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...

[nexttime]

comment:7

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.

[jhpalmieri]

comment:8

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.

[sagetrac-Stefan]

comment:9

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.

[nexttime]

comment:10

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 along PATH, so we need shell=True and a single string containing command -v ... rather than a list of arguments.
• Removing $SAGE_ROOT/local/bin from PATH should IMHO be more robust w.r.t. redundant slashs and normpath() or realpath().

[nexttime]

comment:11

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.

[nexttime]

comment:12

P.P.S.: I'd also move the close() right before "Created script ...".

[jhpalmieri]

comment:13

Replying to @nexttime:

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 along PATH, so we need shell=True and a single string containing command -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 from PATH should IMHO be more robust w.r.t. redundant slashs and normpath() or realpath().

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.

[jhpalmieri]

comment:14

Ah, found it. Bad variable names, I think: it's too easy to confuse these.

>>> import os
>>> os.pathsep
':'
>>> os.path.sep
'/'

[nexttime]

comment:15

Replying to @jhpalmieri:

• command is a shell built-in, and hence not an executable to be found along PATH, so we need shell=True and a single string containing command -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 from PATH should IMHO be more robust w.r.t. redundant slashs and normpath() or realpath().

[...] (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 ";".

[nexttime]

comment:16

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

[jhpalmieri]

comment:17

Replying to @nexttime:

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

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

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 in install_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/ (and ignore_existsing[sic] in a comment). For better readability, I'd also substitute c 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 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
        ...

I changed it to be something like this.

[jhpalmieri]

Attachment: trac_11602-install-scripts.patch.gz

[nexttime]

comment:18

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 the PATH changes.)

• I personally don't like raising RuntimeErrors 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 use sys.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 in o.open() or o.write() could otherwise be associated with "Checking that Sage has the command ...".)

• An else: is superfluous whenever the if-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.

[nexttime]

Reviewer: Leif Leonhardy

[nexttime]

Author: John Palmieri

[jhpalmieri]

for reference only; do not apply

[jhpalmieri]

comment:19

Attachment: trac_11602-delta.patch.gz

Replying to @nexttime:

Only a few minor things:

I've attached attachment: trac_11602-install-scripts.v2.patch to deal with these; attachment: 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 RuntimeErrors 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() or o.write() could otherwise be associated with "Checking that Sage has the command ...".)

Okay.

• An else: is superfluous whenever the if-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.

[nexttime]

comment:22

Replying to @jhpalmieri:

I've attached attachment: trac_11602-install-scripts.v2.patch to deal with these; attachment: 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).

[nexttime]

comment:23

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

[jhpalmieri]

Attachment: trac_11602-install-scripts.v2.patch.gz

[jhpalmieri]

Attachment: trac_11602-delta2to3.patch.gz

for reference only; do not apply

[jhpalmieri]

comment:24

Attachment: trac_11602-install-scripts.v3.patch.gz

Replying to @nexttime:

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 for sage -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 and sage (lower-case) monospaced in

Okay.

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')"

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

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/her PATH 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.)

[nexttime]

comment:25

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.

[nexttime]

comment:26

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

[jhpalmieri]

comment:27

I could add these changes:

diff --git a/sage/misc/dist.py b/sage/misc/dist.py
--- a/sage/misc/dist.py
+++ b/sage/misc/dist.py
@@ -133,7 +133,7 @@ def install_scripts(directory=None, igno
         else:
             o = open(target,'w')
             o.write('#!/bin/sh\n')
-            o.write('sage -%s "$@"\n'%cmd)
+            o.write('exec sage --%s "$@"\n'%cmd)
             o.close()
             print "Created script '%s'"%target
             os.system('chmod a+rx %s'%target)
diff --git a/sage/misc/sage_ostools.py b/sage/misc/sage_ostools.py
--- a/sage/misc/sage_ostools.py
+++ b/sage/misc/sage_ostools.py
@@ -34,8 +34,12 @@ def have_program(program, path=None):
     import os
     try:
         if path:
+            # env is a copy of the current environment, so modifying
+            # it won't affect os.environ.
+            env = os.environ.copy()
+            env['PATH'] = path
             return not call('command -v %s' % program, shell=True,
-                            stdout=PIPE, env = {'PATH': path})
+                            stdout=PIPE, env=env)
         else:
             return not call('command -v %s' % program, shell=True,
                             stdout=PIPE)

[nexttime]

comment:28

Yeah, looks good.

[jhpalmieri]

comment:29

Attachment: trac_11602-install-scripts.v4.patch.gz

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

[nexttime]

comment:30

Positive review also for version 4.

[jdemeyer]

Merged: sage-4.7.2.alpha2