Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#12627 closed defect (fixed)

The spkg/bin/sage script engraves paths to executables

Reported by: Snark Owned by: leif
Priority: major Milestone: sage-6.1
Component: scripts Keywords: rd2
Cc: kini Merged in:
Authors: Julien Puydt, Volker Braun Reviewers: R. Andrew Ohana
Report Upstream: N/A Work issues:
Branch: u/vbraun/use_path_for_binaries (Commits) Commit: 44c4cf1d27b8562200c5b1978d29ad7b70b2f687
Dependencies: Stopgaps:

Description

There is no need to make full paths explicit - it is even an hindrance when one moves things around.

Since sage-env already sets PATH correctly, let's just use that.

The use of 'command' to test for the presence of an executable in the path is portable, as documented in this stackoverflow question.

Change History (42)

comment:1 Changed 5 years ago by Snark

  • Status changed from new to needs_review

comment:2 Changed 5 years ago by jhpalmieri

This needs to be rebased to Sage 5.0: sage-sage no longer exists; the relevant script is $SAGE_ROOT/spkg/bin/sage and is in the root repository.

comment:3 Changed 5 years ago by Snark

Done ; it wasn't just a case of applying the same patch, as now 'R' can be called through the script.

comment:4 Changed 5 years ago by jhpalmieri

This patch doesn't seem like a bad idea, but I don't know if it's actually useful for anything. Using the environment variable SAGE_LOCAL is supposed to make these paths portable already, so I'm not sure I understand the comment about the current situation being "an hindrance when one moves things around". Another way to say it: sage-env sets the PATH by using SAGE_LOCAL, so with or without this patch, if SAGE_LOCAL is right, these commands will work, while if it is wrong, they won't.

comment:5 Changed 5 years ago by Snark

With the current situation, sage-env makes sure the rest of sage has a good environment to work, but sage-sage decides it knows better and shortcuts it.

This is one of the situations I denounced on the sage-devel mailing-list, where sage is supposed to be modular but in fact isn't, because the modules don't just depend on each other's work : there are hidden, subtle places where they do their own thing.

I have a few patches up my sleeve to move things around ; they're not supposed to go upstream anytime soon. But they make some hidden things appear, so I fix them, and as it's interesting for upstream, I report immediately, with a proper fix, independent of my experiments.

To be fully honest, the goal of my not-for-upstream experiments is precisely to find modularity problems and fix them...

comment:6 Changed 5 years ago by ohanar

  • Authors set to Julien Puydt
  • Keywords rd2 added
  • Reviewers set to R. Andrew Ohana
  • Status changed from needs_review to positive_review

The patch looks good to me and works properly for me.

comment:7 follow-up: Changed 5 years ago by leif

Julien, I'm not sure whether you're aware of that, but your patch changes in at least some situations the fully intentional behavior.

For example, a couple of Sage components / programs are optional, like e.g. kash or Macaulay2, and it is not intended to run a system-wide / non-Sage version of kash (that might be found along PATH) if one types sage --kash. (Although I admit that the current response in case Sage's version isn't installed -- "File not found" -- is quite odd, i.e., not very user-friendly.)

In other cases a path is prepended to make sure we really run the correct instance of a Sage script, namely that of the "current" Sage installation, given by SAGE_ROOT (or SAGE_LOCAL), or to make sure our assumptions about the current directory (or other variable settings) are correct -- especially since different people develop things, sometimes perhaps too independently. Not very pythonic, but similar to assert(), which at least signals errors [before things get worse and untraceable].

Redundancy is necessary to achieve some robustness, catch (and hopefully appropriately handle) errors when they occur, or as early as possible, e.g. if a script or program doesn't exist where it is supposed to be located, since we cannot rule out an installation gets messed up -- probably by Sage itself, due to bugs, if not by a user...

comment:8 Changed 5 years ago by leif

Oh, forgot, the PATH issue reminds me of this... (Web 3.0)

comment:9 Changed 5 years ago by Snark

@leif: I did check sage-env prepended to PATH before proposing the patch, so I can confirm that sage will run what it wants when it wants something, and perhaps still run something even if it isn't sure what it wants. And when one moves things around, that is useful. So, positive review?

comment:10 in reply to: ↑ 7 Changed 5 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Replying to leif:

Julien, I'm not sure whether you're aware of that, but your patch changes in at least some situations the fully intentional behavior.

For example, a couple of Sage components / programs are optional, like e.g. kash or Macaulay2, and it is not intended to run a system-wide / non-Sage version of kash (that might be found along PATH) if one types sage --kash. (Although I admit that the current response in case Sage's version isn't installed -- "File not found" -- is quite odd, i.e., not very user-friendly.)

I think this still needs an answer.

comment:11 Changed 5 years ago by jdemeyer

  • Summary changed from The sage-sage script engraves paths to executables to The spkg/bin/sage script engraves paths to executables

comment:12 follow-ups: Changed 5 years ago by Snark

Well, in fact I did answer, but apparently I wasn't clear enough ; let's rephrase it with arrays.

The current situation is :

in-sage out-of-sage what runs?
yes yes in-sage
yes no   in-sage
no yes error
no no error

with my patch :

in-sage out-of-sage what runs?
yes yes in-sage
yes no in-sage
no yes out-of-sage
no no error

and additionnally, using PATH instead of putting the PATH inside removes implicit dependencies, which makes sage really more modular (things can move around and that won't break).

comment:13 in reply to: ↑ 12 Changed 5 years ago by jdemeyer

I think the current behaviour is better. I certainly would not expect

sage --kash

to run some random kash which happens to be installed on my system. I expect sage --kash to run the kash from sage and give an error message when kash is not installed in Sage.

Replying to Snark:

and additionnally, using PATH instead of putting the PATH inside removes implicit dependencies, which makes sage really more modular (things can move around and that won't break).

I really don't see the need for this. Maybe you should be more explicit about the patches on your sleeve.

comment:14 Changed 5 years ago by Snark

If there is a kash on your system, there shouldn't be any problem using it. Just because it doesn't come from sage doesn't mean it's wrong.

My patch makes sage less fragile (less error cases) and more portable (use of 'command' to test the presence of an executable) and less surprising (don't go against what sage-env says it PATH). It doesn't have the side effect of running something out-of-sage when it's available in-sage. What exactly is the problem?

[I can't make more patches public : I already made public those that looked good (see #12623, #12624, #12639 in addition of this one), and the rest were too hackish and are probably obsolete now. Besides, this one patch here has enough merits of its own.]

comment:15 in reply to: ↑ 12 Changed 5 years ago by leif

Replying to Snark:

using PATH instead of putting the PATH inside removes implicit dependencies, which makes sage really more modular (things can move around and that won't break).

I don't really understand what you want to "move around".

As mentioned, the explicit paths, although (or because they are -- in a perfect world) redundant, are for safety reasons, since they lead to errors if a script is missing, i.e., not where it is supposed to be located. That's IMHO just fine. And I don't see what you want to make "more modular" there.

W.r.t. sage --PROGRAM:

If you want to run a system-wide installed version of R, type (outside of a Sage subshell) R.

If you want to run Sage's version of R, type sage --R, or type R from within a Sage subshell.

Same for optional components. The following part of your patch is IMHO plain wrong:

  • spkg/bin/sage

    diff -r 959bec9e01d9 -r a46cac822f1e spkg/bin/sage
    a b  
    9292    echo "  -ipython [...]      -- run Sage's IPython using the default environment (not"
    9393    echo "                         Sage), passing additional options to IPython"
    9494    echo "  -kash [...]         -- run Sage's Kash with given arguments"
    95     test -x "$SAGE_LOCAL/bin/kash" || \
     95    command -v kash &>/dev/null || \
    9696    echo "                         (not installed currently, run sage -i kash)"
    9797    echo "  -lisp [...]         -- run Lisp interpreter included with Sage"
    9898    echo "  -M2 [...]           -- run Sage's Macaulay2 with given arguments"
    99     test -x "$SAGE_LOCAL/bin/M2" || \
     99    command -v M2 &>/dev/null || \
    100100    echo "                         (not installed currently, run sage -i macaulay2)"
    101101    echo "  -maxima [...]       -- run Sage's Maxima with given arguments"
    102102    echo "  -mwrank [...]       -- run Sage's mwrank with given arguments"

along with

  • spkg/bin/sage

    diff -r 959bec9e01d9 -r a46cac822f1e spkg/bin/sage
    a b  
    417417if [ "$1" = '-kash'  -o "$1" = '--kash' ]; then
    418418    cd "$CUR"
    419419    shift
    420     "$SAGE_LOCAL/bin/kash" "$@"
     420    kash "$@"
    421421    exit $?
    422422fi
    423423

and

  • spkg/bin/sage

    diff -r 959bec9e01d9 -r a46cac822f1e spkg/bin/sage
    a b  
    445445if [ "$1" = '-M2'  -o "$1" = '--M2' ]; then
    446446    cd "$CUR"
    447447    shift
    448     "$SAGE_LOCAL/bin/M2" "$@"
     448    M2 "$@"
    449449    exit $?
    450450fi
    451451

(Instead, we should use test -x "$SAGE_LOCAL"/bin/<program> there, too, and tell the user how to install it in case it's not. You could of course also use case "`command -v <program>`" in ..., although the latter might give unexpected results with symlinks; I'm not 100% sure about that.)

Similar for standard components (and other scripts) of Sage, since if -- for whatever reason -- $SAGE_LOCAL/bin/<program> is missing, which is an error, no other program along the path which happens to have the same name should be run; instead, an appropriate error message should be printed.

If you want to "move things around", why not create symbolic links in $SAGE_LOCAL/bin/?

Or you have to be more explicit with what you want to move, or what should be more customizable.


Slightly related:

To install small scripts to directly run Sage's versions of GAP,
the PARI/GP interpreter, Maxima, or Singular etc. (by typing e.g.
just 'gap' or 'gp') into a standard 'bin' directory, start Sage
by typing 'sage' (or './sage') and enter something like

    install_scripts('/usr/local/bin')

at the Sage command prompt ('sage:').

(Type install_scripts? at the Sage prompt for more information, i.e., options to it.)

comment:16 follow-up: Changed 5 years ago by Snark

Sigh.

First, you still mention "test -x" although I pointed out it's not the recommended way to check for an executable.

Second, as I already pointed out, my patch makes sage run sage versions by default if available. And if not available in sage, if they are available on the system, use that. The only error case is the one where indeed, it is really not available. That makes sense.

There seem to be a high dose of paranoia in sage developers about everything that doesn't come out of sage.

You still have to show me why I should be more cautious about running /usr/bin/kash : after all, sage runs on the system bash and uses the system libc! And for that last one, there are definitive reports where that gives problems and failed doctests -- I know it well, it's on ubuntu/ARM and I did the port!

My point is simple : from a sage-whatever-beta_n to sage-whatever-beta_{n+1}, 90% of the spkg are the same. So if I could make foo-3.14.spkg install in $HOME/sage/foo-3.14 and make the rest of sage use that (this is modularity -- the contrary of a one-block where nothing can move an hair's width without everything collapsing), then that means I can gain 90% of build time. And I have the room to have more versions of sage installed simultaneously.

And maintaining a big chunk of symlinks is a bad solution to a wrong problem.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 5 years ago by jdemeyer

Replying to Snark:

Sigh.

First, you still mention "test -x" although I pointed out it's not the recommended way to check for an executable.

You "pointed out" without saying why. I think test -x is used perfectly here.

Second, as I already pointed out, my patch makes sage run sage versions by default if available. And if not available in sage, if they are available on the system, use that. The only error case is the one where indeed, it is really not available.

As I (and leif, for once I agree with him) have said, sage --COMMAND is supposed to run the Sage version of COMMAND.

There seem to be a high dose of paranoia in sage developers about everything that doesn't come out of sage.

That's not really the point. Within Sage, we have more control over the versions of the optional packages. Also, nothing prevents you from running in a shell (either a Sage shell or not):

$ kash

It's as easy as that.

My point is simple : from a sage-whatever-beta_n to sage-whatever-beta_{n+1}, 90% of the spkg are the same. So if I could make foo-3.14.spkg install in $HOME/sage/foo-3.14 and make the rest of sage use that (this is modularity -- the contrary of a one-block where nothing can move an hair's width without everything collapsing), then that means I can gain 90% of build time. And I have the room to have more versions of sage installed simultaneously.

You could play with environment variables to solve this: apply patch #12647 and write in your .sage/sagerc:

export PATH="$PATH:$HOME/sage/foo-3.14/local/bin"
export LD_LIBRARY_PATH="$LD_LIBRARY_PATH:$HOME/sage/foo-3.14/local/lib"

or something like this.

comment:18 follow-up: Changed 5 years ago by Snark

  1. the .sagerc trick depends on setting PATH, but my point is precisely that spkg/bin/sage doesn't use PATH, but instead points directly to where it insists things must be (one of my dirty definitely-not-for-upstream patch was precisely modifying sage-env to set PATH, LD_LIBRARY_PATH and the rest...) ;
  2. "test -x" is nice when you know exactly where things must be ; but if they can be in another place, then that doesn't fly. "command -v" is portable and will find things in PATH.

comment:19 in reply to: ↑ 18 Changed 5 years ago by jdemeyer

Why do you insist on running

sage --kash

as opposed to

kash

comment:20 follow-up: Changed 5 years ago by Snark

I don't insist on running "sage --kash".

I ran "make ptestlong" and it tried to run things using "sage --something".

Kash wasn't the problem ; I don't remember if it was ecl or scons... or something else... Of course the test failed : even though I made sure in sage-env it had what it takes in the PATH, the script didn't even see it!

I claim sage-as-it-is is broken because it can't accept a small change in a place without breaking in an unrelated place for no serious reason.

My patch doesn't break sage-as-it-is, and makes it more easy to modify in the future: tomorrow, you'll decide to put the executables in $SAGE/whatever/bin instead of $SAGE_LOCAL/bin. Of course, you'll modify sage-env to cope with the change. And it will still not work. Why? Because that script doesn't care about sage-env, it does it's own thing... and I claim that is wrong.

My patch makes it possible to change where sage finds executables in one sensible place instead of several.

I'm not telling you: "There is a theoretical issue, drop everything you do and work on it for me!". I'm telling you : "I made private changes, hit a practical issue, and even though you might not have met it yet, there's a chance you will, and here is a simple nice fix."

Does it sound that unreasonable?

comment:21 in reply to: ↑ 17 Changed 5 years ago by leif

Replying to jdemeyer:

Replying to Snark:

First, you still mention "test -x" although I pointed out it's not the recommended way to check for an executable.

That's simply bu**sh**. First of all, I did mention command -v as well, and more importantly, test -x and command have different use cases. Nobody claimed that command wasn't portable; we use it in many places of Sage -- where it is appropriate.


You "pointed out" without saying why. I think test -x is used perfectly here.

It is.


Second, as I already pointed out, my patch makes sage run sage versions by default if available. And if not available in sage, if they are available on the system, use that. The only error case is the one where indeed, it is really not available.

As I (and leif, for once I agree with him)

:-)

have said, sage --COMMAND is supposed to run the Sage version of COMMAND.

And if sage --PROGRAM should run external versions at all, then the help messages would have to be changed as well, which your patch doesn't, although it changes the (btw. intuitive) behavior:

     echo "  -kash [...]         -- run Sage's Kash with given arguments"
...
     echo "  -M2 [...]           -- run Sage's Macaulay2 with given arguments"

(And if there happen to be system-wide versions along the path, there's no mention that Sage's optional packages could be installed [in case they aren't already]. People frequently have completely outdated, incompatible or broken versions installed -- sometimes even not to their knowledge, which is one reason Sage ships such a lot, or at least offers such components as optional packages.)


There seem to be a high dose of paranoia in sage developers about everything that doesn't come out of sage.

Sometimes for good reason; see above.

And as mentioned, it's better to not blindly trust even the Sage installation itself, since its integrity is not necessarily an invariant, especially if users (and developers, which most people of the Sage community intentionally don't differentiate) "play" with it.

comment:22 in reply to: ↑ 20 Changed 5 years ago by leif

Replying to Snark:

I ran "make ptestlong" and it tried to run things using "sage --something".

Kash wasn't the problem ; I don't remember if it was ecl or scons... or something else... Of course the test failed : even though I made sure in sage-env it had what it takes in the PATH, the script didn't even see it!

I claim sage-as-it-is is broken because it can't accept a small change in a place without breaking in an unrelated place for no serious reason.

Not in an "unrelated place", nor for no reason: ptestlong checks whether the installation went (or is still) ok, and testing whether programs installed by Sage are where they belong (or have been installed), and whether they work, is just part of that.

If you change their locations, you'd obviously also have to change these tests.

comment:23 Changed 5 years ago by Snark

Oh, dear, that was the thing I missed to understand!

Ok, so when "make ptestlong" runs "sage --something", it really checks :

  1. things are at the right place ;
  2. things work ;
  3. things work correctly.

I was only considering points 2 and 3 since the beginning! My patch does indeed break point 1.

Now, I still think there are things to fix :

  1. checking that something was installed at the right place should be done by something.spkg, not in somewhere unrelated -- that will cover 1 where it should be covered ;
  2. and then my patch should be applied, because now the implicit constraint will be satisfied in an explicit way by a proper test somewhere.

comment:24 Changed 5 years ago by leif

How about introducing SAGE_BINARIES_PATH? (to be optionally set by the user; containing a colon-separated list of directories to search for Sage programs and scripts)

(sage-env would probably prepend e.g. "$SAGE_LOCAL/bin:" if SAGE_BINARIES_PATH is non-empty, otherwise set it to the former.)

Then we could use (logically)

PATH="$SAGE_BINARIES_PATH" command -v FOO >/dev/null || echo "Sage's FOO is not installed..."

or

PATH="$SAGE_BINARIES_PATH" command FOO
# exit status 127 would (usually) tell that FOO wasn't found (along $SAGE_BINARIES_PATH)

and similar.

A potential problem being that FOO would inherit the modified PATH in the second example; we could therefore also use

prog=`PATH="$SAGE_BINARIES_PATH" command -v FOO`
if [[ -z $prog ]]; then
    # FOO isn't installed anywhere along $SAGE_BINARIES_PATH
else
    # found; for example run FOO:
    $prog
fi


Better names appreciated. SAGE_PATH is unfortunately already used for a slightly different purpose.

comment:25 follow-up: Changed 5 years ago by Snark

I'm not sure the idea is that neat, but SAGE_EXEC_PATH could be a better name for it ;-)

"PATH" does it already... doesn't it?

comment:26 in reply to: ↑ 25 Changed 5 years ago by leif

Replying to Snark:

"PATH" does it already... doesn't it?

PATH contains arbitrary things... (and we still don't want to remove parts from it ;-) )

comment:27 Changed 5 years ago by kini

  • Cc kini added

comment:28 in reply to: ↑ 12 Changed 4 years ago by felixs

The build system proposed in http://wiki.sagemath.org/BuildSystemSEP (#14796), provides options to disable packages (and falling back to already installed things outside of sage). also sage (the library) needs to work without the distribution.

Finally, under some circumstances, sage --PROGRAM needs to just use plain $PATH. And it always should.

So lets solve this problem at its root:

  • sage (the distribution) places a dummy into $SAGE_LOCAL/bin/p for all enabled packages p, if such a file doesnt exist (or more dummies if needed)
  • installation of a package will replace these dummies
  • sage (the distribution) clears these dummies upon disabling a package (which also triggers uninstall)
  • sage (the environment) just sets PATH=$(SAGE_LOCAL)/bin:$PATH (as it currently does)
  • sage (the library) works outside sage (the distribution) without headache

To deal with even more paranoia, sage (the distribution) could also include a list of header and library locations for each package. Placed in a similar way, respective files could make sure gcc won't fall back to /usr/include or /usr/lib without your consent.

comment:29 Changed 4 years ago by felixs

This is now implemented in #14796 (Component: 'build'). Here, Sage (the distribution) creates/removes dummies depending on how it is configured. $PATH is made to contain SAGE_LOCAL/bin, PATH where the dummies are, the system PATH in that order.

With this mechanism, sage --PROGRAM may safely use $PATH without accidentally running into system executables from within sage (the distribution).

comment:30 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:31 Changed 4 years ago by was

  • Status changed from needs_info to needs_review

comment:32 Changed 4 years ago by was

Please rebase this:

(sage-sh) Nav1ZWFD@compute4a:bin$ hg import trac-12627.patch
applying trac-12627.patch
patching file spkg/bin/sage
Hunk #2 FAILED at 265
Hunk #3 FAILED at 367
Hunk #4 FAILED at 402
Hunk #5 FAILED at 430
Hunk #6 FAILED at 465
Hunk #7 FAILED at 548
6 out of 7 hunks FAILED -- saving rejects to file spkg/bin/sage.rej
abort: patch failed to apply

There's exec's at the beginning of many of the lines in sage, which cause the patch to be rejected. Otherwise, I think this should go into Sage soon-ish, since the change makes sense, and will help with the OS X port ticket (e.g., because there we use the system-wide sqlite3, but we still want sage --sqlite3 to be portable and work).

comment:33 Changed 4 years ago by was

  • Status changed from needs_review to needs_work

comment:34 follow-up: Changed 4 years ago by jhpalmieri

There are disagreements about whether the changes here make sense, and I'm not even sure what the status of this ticket is. Is it subsumed by #14796? I'll post a suggestion about sqlite3 to #15433.

comment:35 in reply to: ↑ 34 Changed 4 years ago by felixs

Replying to jhpalmieri:

Is it subsumed by #14796?

yes, in #14796, '$PATH' is used to locate executables. it is a good idea to to so, whether or not a dummy wall or a configuarable toplevel build system exists.

... and while you/someone are/is at it: the code may be reworded to

arg=${1/--/-}
case $arg in
    -axiom|-combinat|-gap|-gp|-singular|-sqlite3|-twistd|-ecl|\
    -kash|-maxima|-mwrank|-M2|-scons|-python|-ipython|-R)
        shift
        exec "${arg#-}" "$@"
    ;;
    -lisp)
        shift
        exec "ecl" "$@"
    ;;
esac

, so it will even fit on a single screen.

comment:36 Changed 4 years ago by vbraun

  • Branch set to u/vbraun/use_path_for_binaries
  • Commit set to 44c4cf1d27b8562200c5b1978d29ad7b70b2f687

New commits:

44c4cf1Use foo from path when running sage --foo

comment:37 Changed 4 years ago by vbraun

  • Authors changed from Julien Puydt to Julien Puydt, Volker Braun
  • Status changed from needs_work to needs_review

IMHO the whole sage -foo script is a mistake. If you want to use Sage's version of binaries, you put them into your path, should be as easy as that. Of course this assumes proper rpaths for libraries instead of the LD_LIBRARY_PATH hack.

In any case, until then we should just implement the proposed workaround.

comment:38 Changed 4 years ago by vbraun

  • Status changed from needs_review to positive_review

Passes doctests.

comment:39 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.13 to sage-6.0

comment:40 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:41 Changed 4 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:42 Changed 4 years ago by jhpalmieri

  • if you run sage --kash, should it run a system version of kash if Sage's optional kash spkg hasn't been installed, or not?

DWIM (http://en.wikipedia.org/wiki/DWIM) is pretty unambiguous here. Having typed that command, do I want kash to run or do I want to get some obscure error even though kash is actually on my computer?

  • in the old version there was some redundancy: the path to gap, for example, was explicit, to make sure that Sage's version of GAP was used. Is this redundancy important?

Thats really just a corollary to the first point---we should make the best effort to get gap up and running, otherwise our user interface is crap.

  • the message for sage --help says that sage --M2 (for example) runs "Sage's Macaulay2", but this is no longer accurate with the current patch.

This is now #15538

Last edited 4 years ago by vbraun (previous) (diff)
Note: See TracTickets for help on using tickets.