Opened 11 years ago

Closed 11 years ago

#10339 closed defect (fixed)

Simplify spkg/pipestatus

Reported by: jdemeyer Owned by: GeorgSWeber
Priority: major Milestone: sage-4.6.1
Component: scripts Keywords: pipestatus Makefile shell environment tee exit code status
Cc: leif, drkirkby, fbissey Merged in: sage-4.6.1.rc0
Authors: Jeroen Demeyer, Leif Leonhardy Reviewers: Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

spkg/pipestatus currently uses two different mechanisms, depending on the bash version (>=3.0 or not). Unfortunately, the two alternatives currently differ in behaviour (the consequence in one case being an install.log file appearing in the wrong place).

Consider a command

pipestatus "A && B" "C"

Alternative 1 (for new versions of bash) does

A && (B | C)

(which is - without regard of the subshell environment - equivalent to the command list without parentheses, i.e. A && B | C, since the pipe symbol has higher precedence),

while alternative 2 (for very old versions of bash) does

(A && B) | C

The attached / merged patch makes pipestatus behave like alternative 1 for all versions of bash, and executes all commands in the same environment.

It also patches the top-level Makefile to no longer depend on (previously) varying behavior of pipestatus by changing a receipt to

        cd some_dir && pipestatus "A" "B"

(where A and B are simple commands).


Merged patches: 10339_pipestatus.v3.patch and 10339_pipestatus.v3_spkg_install.patch

Attachments (6)

10339_pipestatus.patch (2.6 KB) - added by jdemeyer 11 years ago.
non-hg patch for SAGE_ROOT
10339_pipestatus.v2.patch (2.7 KB) - added by jdemeyer 11 years ago.
Alternative patch, based on Leif's proposal
10339_pipestatus.v3.patch (3.7 KB) - added by jdemeyer 11 years ago.
Leif's alternative, apply to SAGE_ROOT
10339_pipestatus.v3_spkg_install.patch (3.5 KB) - added by jdemeyer 11 years ago.
Leif's alternative, patch for spkg/install
10339_authors.patch (642 bytes) - added by jdemeyer 11 years ago.
Apply on top of previous
10339_authors_install.patch (664 bytes) - added by jdemeyer 11 years ago.

Download all attachments as: .zip

Change History (55)

comment:1 in reply to: ↑ description ; follow-ups: Changed 11 years ago by leif

Replying to jdemeyer:

In my opinion, there should be only one alternative. If we have something which works for old versions, why not use that for new versions also?

Because it is completely inefficient; I would even use set -o pipefail in bash >= 3.0 directly, and not call scripts.

IMHO Bash 3.0 is old enough to make it a prerequisite anyway.

In any case, it would be good to think of a more robust implementation of pipestatus. I can think of the following:

  1. Write a simple C program using pipe(), fork(), exec(), wait().
  1. Use the mkfifo command.
  1. Use a temporary file:
EXITSTATUS=/tmp/pipestatus$$     # Obviously, we should use TMPDIR if set
( A; echo $? >$EXITSTATUS ) | B

Back to MS-DOS? ;-)

comment:2 in reply to: ↑ description Changed 11 years ago by leif

Replying to jdemeyer:

Consider a command

pipestatus "A && B" "C"

Someone reading the "documentation":

if [ -z "$1" ]; then
    echo "Run two commands in a pipeline 'CMD1 | CMD2' and exit"
    echo "with the exit status of CMD1, *not* that of CMD2."
    echo "$0 cmd1 cmd2"
    exit
fi

would perhaps do:

pipestatus "(A && B)" "C"

but we can add the parentheses in pipestatus to get the same behavior in both cases:

  • pipestatus

    old new  
    1616
    1717if [ $VER -gt 2 ]; then
    1818    # Use bash 3.0's pipefail option.
    19     (set -o pipefail; eval "$1 | $2")
     19    set -o pipefail
     20    eval "($1) | $2"
    2021    exit $?
    2122else
    2223    # Use redirection.  Adapted from the comp.unix.shell FAQ.  See

Btw, if we change pipestatus, we should also test $# -eq 2 and give an error otherwise. Such a change was previously rejected just because someone didn't want to touch pipestatus or spkg/install again.

Alternative 1 (for new versions of bash) does A && (B | C) while alternative 2 (for old versions of bash) does (A && B) | C.

The patch above fixes that, though it's IMHO a minor issue, because one can easily make the call to pipestatus unambiguous.

comment:3 Changed 11 years ago by leif

Perhaps better

  • pipestatus

    old new  
    11#!/usr/bin/env bash
    22
    3 if [ -z "$1" ]; then
     3if [ $# -ne 2 ] || [ -z "$1" -o -z "$2" ]; then
    44    echo "Run two commands in a pipeline 'CMD1 | CMD2' and exit"
    55    echo "with the exit status of CMD1, *not* that of CMD2."
    66    echo "$0 cmd1 cmd2"
     
    1616
    1717if [ $VER -gt 2 ]; then
    1818    # Use bash 3.0's pipefail option.
    19     (set -o pipefail; eval "$1 | $2")
     19    set -o pipefail
     20    eval "( $1 ) | ( $2 )"
    2021    exit $?
    2122else
    2223    # Use redirection.  Adapted from the comp.unix.shell FAQ.  See

comment:4 Changed 11 years ago by leif

P.P.S.: We should then also update the documentation, i.e. the usage message, to reflect that one or both commands are run in (different) subshells.

comment:5 Changed 11 years ago by leif

P.P.P.S.:

Rather than testing for the Bash version, it would be better to use

if set -o | grep -w pipefail &>/dev/null; then
    # shell (whatever it is) supports set -o pipefail
    ...
else
    # not supported - work around
    ...
fi

since other shells might adopt that feature.

comment:6 in reply to: ↑ 1 ; follow-up: Changed 11 years ago by jdemeyer

Replying to leif:

IMHO Bash 3.0 is old enough to make it a prerequisite anyway.

-1 to this idea, since OS X 10.4 has bash version 2.

The question I posed in the ticket remains: why should there be two cases (the second case should work always, no matter the bash version).

By the way: fixing eval "$1 | $2" is easy: eval "$1" | eval "$2".

comment:7 in reply to: ↑ 1 Changed 11 years ago by jdemeyer

Replying to leif:

Replying to jdemeyer:

In my opinion, there should be only one alternative. If we have something which works for old versions, why not use that for new versions also?

Because it is completely inefficient

Please elaborate...

comment:8 in reply to: ↑ 6 ; follow-ups: Changed 11 years ago by leif

Replying to jdemeyer:

Replying to leif:

IMHO Bash 3.0 is old enough to make it a prerequisite anyway.

-1 to this idea, since OS X 10.4 has bash version 2.

I don't care about it since it is dead old and only causes problems. If someone is able to build Sage on MacOS X 10.4, Bash 3.x or 4.x should also build, be it an optional Sage package for boot-strapping.

The question I posed in the ticket remains: why should there be two cases (the second case should work always, no matter the bash version).

Having both in pipestatus doesn't make much sense. As said, if at all I would make a distinction in the scripts where pipestatus is needed / called, and / or use a variable for the command, which could simply be a shell function. (spkg/standard/deps e.g. should IMHO be generated anyway.)

By the way: fixing eval "$1 | $2" is easy: eval "$1" | eval "$2".

Sure.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 11 years ago by GeorgSWeber

Replying to leif:

Replying to jdemeyer:

Replying to leif:

IMHO Bash 3.0 is old enough to make it a prerequisite anyway.

-1 to this idea, since OS X 10.4 has bash version 2.

I don't care about it since it is dead old and only causes problems. If someone is able to build Sage on MacOS X 10.4, Bash 3.x or 4.x should also build, be it an optional Sage package for boot-strapping.

Well, there are people that do care about OS X 10.4. Since (at least) Sage-3.4 till the current Sage-4.6.alpha2, this is an officially supported platform with the regular binary distributions. If Harald (who is able to look at the statistics) tells me nobody downloads these anymore, I'll stop providing them. OTOH, the inclusion of bash as a Sage dependency has been discussed before. (Mainly due to the problems if the system bash is built to use readline as dynamic librariy, and blows up if the readline lib provided by Sage is older, but is found first in the library search path due to the Sage specific LD_LIBRARY_PATH settings, as e.g. in Arch Linux and younger SuSE distributions.) But the conclusion was always that we don't want to have bash as a core dependency to be shipped with Sage (be it only for a few OSes).

Cheers, Georg

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

Replying to GeorgSWeber:

Replying to leif:

Replying to jdemeyer:

Replying to leif:

IMHO Bash 3.0 is old enough to make it a prerequisite anyway.

-1 to this idea, since OS X 10.4 has bash version 2.

I don't care about it since it is dead old and only causes problems. If someone is able to build Sage on MacOS X 10.4, Bash 3.x or 4.x should also build, be it an optional Sage package for boot-strapping.

Well, there are people that do care about OS X 10.4.

I've also wasted enough time with trying to find out what's going wrong on some MacOS... ;-)

Since (at least) Sage-3.4 till the current Sage-4.6.alpha2, this is an officially supported platform with the regular binary distributions.

Time goes by...; I think five years of "active" support are more than one can expect today, and there's no such machine on the build farm, so it is no longer an "officially supported" platform according to what was written on sage-devel.

OTOH, the inclusion of bash as a Sage dependency has been discussed before. (Mainly due to the problems if the system bash is built to use readline as dynamic librariy, and blows up if the readline lib provided by Sage is older, but is found first in the library search path due to the Sage specific LD_LIBRARY_PATH settings, as e.g. in Arch Linux and younger SuSE distributions.) But the conclusion was always that we don't want to have bash as a core dependency to be shipped with Sage (be it only for a few OSes).

Making a specific (minimal) version mandatory (rather than just requiring Bash, which is the case anyway, i.e. it is already a prerequisite) doesn't mean we should ship it with every release. There are other things that could (and IMHO should) be made available differently, like the odd Fortran binaries that are totally useless to everyone not using MacOS X.

So I would for convenience perhaps offer a Bash 3.x or 4.x for MacOS X [10.4], but separate from Sage distributions. I'm pretty sure such packages are already available elsewhere though.

Btw, I don't think a newer bash is required in a binary Sage distribution for ordinary use. So just having a newer bash on the build system might be sufficient for many people.

comment:11 follow-up: Changed 11 years ago by jdemeyer

How did a discussion of pipestatus turn in a discussion about a bash spkg? The point is, it is easy to implement something like pipestatus on OS X 10.4 without a bash spkg, so what's the problem?

If one day there is truly a big problem on OS X 10.4, then we might consider dropping support. But as long as we can make Sage run on OS X 10.4 without too much effort, I think we should try supporting it.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 11 years ago by leif

Replying to jdemeyer:

How did a discussion of pipestatus turn in a discussion about a bash spkg?

That's actually a continued discussion...

The point is, it is easy to implement something like pipestatus on OS X 10.4 without a bash spkg, so what's the problem?

Go ahead. (If it doesn't make worse what we currently have, which is really suboptimal).

But it's obviously the most easiest thing to simply use a useful feature of a shell we already require anyway.

I'd say define a variable name (e.g. PIPE_STATUS) for what makes up pipestatus and then let $PIPE_STATUS cmd1 cmd2 either call a Bash-3.0 function (using the pipefail shell option) or call a pipestatus script, from which we can remove the first case. Of course the second implementation could also be made into a shell function, though more complex.

As mentioned elsewhere, I would also move the logging (tees) from deps to sage-spkg, which simplifies things.

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

Replying to leif:

As mentioned elsewhere, I would also move the logging (tees) from deps to sage-spkg, which simplifies things.

Well, pipestatus is also used for make doc-html, so I would leave it as a shell script where it is.

comment:14 in reply to: ↑ 8 Changed 11 years ago by drkirkby

Replying to leif:

Replying to jdemeyer:

Replying to leif:

IMHO Bash 3.0 is old enough to make it a prerequisite anyway.

-1 to this idea, since OS X 10.4 has bash version 2.

I don't care about it since it is dead old and only causes problems. If someone is able to build Sage on MacOS X 10.4, Bash 3.x or 4.x should also build, be it an optional Sage package for boot-strapping.

There are several people building on 10.4 (tiger). It is not as dead as you believe. OS X 10.5 (Leopard) was not released until October 2007. Making life hard for people with computers just over 3 years old is not in my opinion very good. Unlike Linux & Solaris, operating system upgrades are not free for OS X.

The first release of Solaris 10 (03/2005) slightly predates OS X 10.4 (April 2005), yet comes with bash 3.00.16(1).

The question I posed in the ticket remains: why should there be two cases (the second case should work always, no matter the bash version).

IMHO, if we relied less on bashims, and sought advice from places like the autoconf mailing list, or comp.unix.shell, we would do a lot better. Those places tend to have people know know how to write portable shell scripts.

Dave

Changed 11 years ago by jdemeyer

non-hg patch for SAGE_ROOT

comment:15 Changed 11 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Status changed from new to needs_review

comment:16 follow-up: Changed 11 years ago by leif

Do we really want to run "cmd1" in a different subshell?


If we change (the meaning of) spkg/pipestatus, we should perhaps also change its (potential) generation in spkg/install.

comment:17 in reply to: ↑ 16 Changed 11 years ago by jdemeyer

Replying to leif:

Do we really want to run "cmd1" in a different subshell?

Why not? It seems to work fine on various systems that I tested.

If we change (the meaning of) spkg/pipestatus, we should perhaps also change its (potential) generation in spkg/install.

I don't see the need for this, what would you propose?

comment:18 Changed 11 years ago by drkirkby

I admit I don't feel comfortable reviewing this. Let me have a go and see what I think.

But I am against making bash 3.0 a perquisite. If this can be done without working around different bash versions, then lets do so.

I found this.

http://cfajohnson.com/shell/cus-faq-2.html#Q11

Dave

comment:19 Changed 11 years ago by leif

As a compromise, I'd suggest using simply Bash's PIPESTATUS array, which is available in by far older versions as well, e.g.

    ...
    # e.g. eval "$1 | $2"
    # or   eval "$1" | eval "$2"

    if [ ${PIPESTATUS[0]} -ne 0 ]; then
        exit ${PIPESTATUS[0]}
    else
        exit ${PIPESTATUS[1]} # equivalent to exit $? (if 'pipefail' is not set)
    fi

    # NOTREACHED

As you can see, the else part is completely redundant, since a script does exit $? by default.

comment:20 follow-up: Changed 11 years ago by jdemeyer

  • Status changed from needs_review to needs_info

If you want compatibility with pipefail, you should exchange 0 and 1 in your patch.

I would do eval "$1" | eval "$2" which I think is more logical than eval "$1 | $2".

comment:21 in reply to: ↑ 20 Changed 11 years ago by leif

Replying to jdemeyer:

If you want compatibility with pipefail, you should exchange 0 and 1 in your patch.

No, that would return 0 if cmd1 fails but not cmd2. So we could do:

#!/usr/bin/env bash

    ...
    if [ ${PIPESTATUS[0]} -ne 0 ]; then
        if [ ${PIPESTATUS[1]} -ne 0 ]; then
            exit ${PIPESTATUS[1]}
        else
            exit ${PIPESTATUS[0]}
        fi
    fi

    # else returns $?, which is ${PIPESTATUS[1]}

Ok, swapping all 0's and 1's in the previous example (but only with the else part) also works.

I would do eval "$1" | eval "$2" which I think is more logical than eval "$1 | $2".

Is it?

comment:22 follow-up: Changed 11 years ago by jdemeyer

  • Status changed from needs_info to needs_review

Leif: what do you think of this alternative patch? I've tested that it behaves the same as set -o pipefail.

comment:23 in reply to: ↑ 22 ; follow-ups: Changed 11 years ago by leif

Replying to jdemeyer:

Leif: what do you think of this alternative patch? I've tested that it behaves the same as set -o pipefail.

Better. :)

Using eval twice still behaves different to what you get directly on the command line; we could instead use:

eval "$1 | $2 ; ecs=(\${PIPESTATUS[*]})"

if [ ${ecs[1]} -ne 0 ]; then
    exit ${ecs[1]}
else
    exit ${ecs[0]}
fi

The comment "Like this, ..." is a bit misleading, as we exactly fix that undesired behavior with pipestatus.

comment:24 in reply to: ↑ 23 ; follow-up: Changed 11 years ago by jdemeyer

Replying to leif:

Using eval twice still behaves different to what you get directly on the command line; we could instead use:

I disagree. Personally, I wouldn't expect pipestatus to behave in a way that the first argument influences the second. For me, the command line arguments to pipestatus are like "blocks" and should not be split. In other words, I expect

pipestatus "A && B" "C"

to behave like

(A && B) | (C)

Changed 11 years ago by jdemeyer

Alternative patch, based on Leif's proposal

comment:25 in reply to: ↑ 23 Changed 11 years ago by jdemeyer

  • Reviewers set to Leif Leonhardy

Replying to leif:

The comment "Like this, ..." is a bit misleading, as we exactly fix that undesired behavior with pipestatus.

True. I have updated the comment.

comment:26 in reply to: ↑ 23 Changed 11 years ago by jdemeyer

Replying to leif:

Using eval twice still behaves different to what you get directly on the command line

Consider the following Python analogy:

def multiply(a,b):
    return a*b

You would not expect multiply(x+y,z) to behave like x+y*z but you expect it to behave like (x+y)*z.

comment:27 in reply to: ↑ 24 ; follow-up: Changed 11 years ago by leif

Replying to jdemeyer:

Replying to leif:

Using eval twice still behaves different to what you get directly on the command line; we could instead use:

I disagree.

Well, certainly not on what one gets directly at the shell prompt.

Personally, I wouldn't expect pipestatus to behave in a way that the first argument influences the second. For me, the command line arguments to pipestatus are like "blocks" and should not be split. In other words, I expect

pipestatus "A && B" "C"

to behave like

(A && B) | (C)

And I would expect pipestatus cmd1 cmd2 to behave as what it is intended to replace (for the only reason that the direct syntax isn't supported by dead old bashs on user-friendly OSs):

$ set -o pipefail; cmd 1 | cmd2

We here have simple textual substitution (or term rewriting like in macros):

    pipestatus = lambda cmd1. cmd2: cmd1 | cmd2

That's not eager evaluation / call by value, like (analogously) pipestatus `eval cmd1` `eval cmd2`, or what one would expect from true functions. Of course I would

#define mul(a,b)  ((a)*(b))  /* and not (a*b), which might give contra-intuitive results */

but that's a different story (and you certainly know the pitfalls when passing things like n++ to macros rather than [inline] functions).

In a shell, with a | operator, I would expect exactly what the shell (and especially make, in our application domain) usually does, namely simple text substitution (and concatenation without bracketing), for perhaps later evaluation. If I want or need parentheses here, I pass them.

And as a side-effect, we get bash into the make receipt. Rather than writing

target: prereq
        bash -c "cmd1" ; pipestatus "cmd2" "cmd3" ; bash -c "cmd4"

we can simply do

target: prereq
        pipestatus "cmd1 ; cmd2" "cmd3 ; cmd4"

with the advantage of having the ability to e.g, set environment variables in the first command that take effect in the latter.

Ok, you could also write

target: prereq
        bash -c "cmd1 ; pipestatus cmd2 cmd3 ; cmd4"

but then you again get into trouble with quoting.

comment:28 follow-up: Changed 11 years ago by jdemeyer

I guess we have to agree to disagree then.

Just to be clear: are you refusing a positive_review because of this or can you live with my patch?

comment:29 in reply to: ↑ 28 Changed 11 years ago by leif

Replying to jdemeyer:

I guess we have to agree to disagree then.

...
if [ $# -eq 3 -a -n "$2" -a -n "$3" ]; then
    case "$1" in
        -j|--jeroen|--contra-intuitively)
             eval "$2" | eval "$3" ; pipestatus=(${PIPESTATUS[*]})
             ;;
        -l|--leif|--as-expected|--as-previously-with-current-bashs)
             eval "$2 | $3 ; pipestatus=(\${PIPESTATUS[*]})"
             ;;
         *)
             usage
    esac
    if [ ${pipestatus[1]} -ne 0 ]; then 
        exit ${pipestatus[1]}
    else
        exit ${pipestatus[0]}
    fi
else
    usage
fi # semper

# NOTREACHED

Just to be clear: are you refusing a positive_review because of this or can you live with my patch?

You refuse to get convinced...

Honestly, I reluctantly gave a lot of positive reviews to IMHO bad (to be fair, some of them worse) things recently, like keeping useless baggage in spkgs or leaving definitely wrong code like unset MAKEFLAGS in because others feared to make changes, in fact corrections, or like introducing new superfluous behavior regarding SAGE64 (which should be called SAGE_FORCE_64_BIT_BUILD, to restate it once more) etc., at the same time having invested into tickets that then wouldn't get merged or again postponed, to next year.

I'll leave the decision to someone else. It's certainly not such an important design decision, but why should I agree on something I still have a different opinion on?

You didn't give an answer to the other aspects I mentioned.

comment:30 in reply to: ↑ 27 Changed 11 years ago by jdemeyer

Replying to leif:

You didn't give an answer to the other aspects I mentioned.

You mean this?:

Replying to leif:

And as a side-effect, we get bash into the make receipt. Rather than writing

target: prereq
        bash -c "cmd1" ; pipestatus "cmd2" "cmd3" ; bash -c "cmd4"

we can simply do

target: prereq
        pipestatus "cmd1 ; cmd2" "cmd3 ; cmd4"

with the advantage of having the ability to e.g, set environment variables in the first command that take effect in the latter.

I don't think that's relevant. If you really need specific bash code in your Makefile, you better just write a separate shell script.

comment:31 follow-up: Changed 11 years ago by jhpalmieri

  • Status changed from needs_review to needs_work

I don't really care which version eventually gets used, but especially since this issue is being debated here, it's crucial that the behavior of pipestatus with regard to a command like pipestatus "A && B" "C" be documented, say in the message at the beginning of that file or in the comments following that, or both.

if [ $# -ne 2 ] || [ -z "$1" -o -z "$2" ]; then 
    echo >&2 "Usage: $0 CMD1 CMD2" 
    echo >&2 "Run two commands in a pipeline 'CMD1 | CMD2' and exit" 
    echo >&2 "with the exit status of CMD1, *not* that of CMD2." 
    echo >&2 "MAYBE ADD SOMETHING HELPFUL HERE"
    exit 2 

Or here:

# Run two commands in a pipeline and exit with the exit status of the 
# first command, not the second.
# This is useful, for example, in a makefile, where we tee the output
# ....
# Note that if you run 
#     pipestatus "A && B" "C"
# then ...

comment:32 in reply to: ↑ 31 Changed 11 years ago by leif

Replying to jhpalmieri:

I don't really care which version eventually gets used, but especially since this issue is being debated here, it's crucial that the behavior of pipestatus with regard to a command like pipestatus "A && B" "C" be documented, say in the message at the beginning of that file or in the comments following that, or both.

Well, my approach follows the principle of least astonishment in that it lets pipestatus behave as if you issued "cmd1 | cmd2" at the shell prompt (with pipefail set), where cmd1 and cmd2 are meta variables (non-terminals) for arbitrary valid shell commands, sequences of commands etc. included. ;-)

That's easy to document.

(I think a comment in the file is sufficient, as pipestatus is not intended to be called directly by a user. A developer if in doubt would perhaps look at the file rather than trying to call it. The usage message should of course mention that pipestatus exits with the last non-zero exit code or zero if the resulting whole statement is a pipeline, i.e. resembles the behavior when Bash's pipefail option is set.)

comment:33 follow-up: Changed 11 years ago by leif

P.S.:

While with my approach one can force the behavoir Jeroen expects (by using parentheses), the opposite is impossible. On the other hand, one could of course also do something like

$ A && pipestatus "B" "C" 

to get the behavior of

$ A && B | C  # same as  pipestatus "A && B" "C"  with my version

with Jeroen's version, but that's IMHO odd, at least in Makefiles where we mainly use it (and A there would potentially be executed by a different shell than the parameters to pipestatus, B and C).

The name of the script (and the current "syntax description") suggests not to use other things than simple commands for each of the two parameters anyway I think.

comment:34 in reply to: ↑ 33 ; follow-up: Changed 11 years ago by leif

Replying to leif:

[...] but that's IMHO odd, at least in Makefiles where we mainly use it (and A there would potentially be executed by a different shell than the parameters to pipestatus, B and C).

Note that we would not log the output of command A in a make rule like

target: prereq
        A && pipestatus "B" "tee -a logfile"

comment:35 in reply to: ↑ 34 ; follow-up: Changed 11 years ago by leif

Replying to leif:

Note that we would not log the output of command A in a make rule like

target: prereq
        A && pipestatus "B" "tee -a logfile"

In fact also not with my version of pipestatus, where one would instead use

target: prereq
        pipestatus "(A && B)" "tee -a logfile"

(If A is e.g. cd spkg there, we would also - as we currently do - use tee -a ../logfile there, because A && B is explicitly executed in a subshell.)

comment:36 in reply to: ↑ 35 ; follow-up: Changed 11 years ago by leif

Replying to leif:

(If A is e.g. cd spkg there, we would also - as we currently do - use tee -a ../logfile there, because A && B is explicitly executed in a subshell.)

I mean we would not have to use ../logfile because cd spkg && B is explicitly executed in a subshell, by using parentheses, i.e.

pipestatus "(cd spkg && B)" "tee -a logfile".

Haven't had much sleep...

comment:37 in reply to: ↑ 36 Changed 11 years ago by jdemeyer

Replying to leif:

I mean we would not have to use ../logfile because cd spkg && B is explicitly executed in a subshell, by using parentheses, i.e.

pipestatus "(cd spkg && B)" "tee -a logfile".

Don't you think that simply being able to write

pipestatus "cd spkg && B" "tee -a logfile"

is the most natural solution? No subshells, no ../

comment:38 Changed 11 years ago by leif

Finally, perhaps we should simply rename the script and just let it do what we mainly need:

tee_output_of_commands <string of commands> <logfile>

(where <logfile> might also be "-a some_file")

and let it just do the equivalent of

set -o pipefail; ( <commands> ) | tee <logfile>

(where in the script <commands> would be eval "$1" and <logfile> would be $2, without quotes).

comment:39 Changed 11 years ago by leif

Jeroen, (at least) in case we use your solution, we have to also change spkg/install where pipestatus is created if it doesn't already exist, for upgrades from older versions prior to 4.5.1.

comment:40 Changed 11 years ago by jdemeyer

  • Component changed from build to scripts

comment:41 Changed 11 years ago by jdemeyer

  • Description modified (diff)

Changed 11 years ago by jdemeyer

Leif's alternative, apply to SAGE_ROOT

Changed 11 years ago by jdemeyer

Leif's alternative, patch for spkg/install

comment:42 Changed 11 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Leif, I implemented the patch your way. Not because I think it is better, but because I think there is a bug to be fixed. I also rewrote the Makefile rule not to assume any particular behaviour for spkg/pipestatus.

Please review, I would like to merge this in 4.6.1.

comment:43 Changed 11 years ago by jdemeyer

  • Merged in set to sage-4.6.1.rc0

comment:44 Changed 11 years ago by leif

  • Status changed from needs_review to positive_review

To bring this (at least the ticket) to an end, positive review from me (as I am a co-author / have suggested most of the current changes ;-) ).

It's also clearly better to use an "unambiguous" call of pipestatus in the (top-level) Makefile. We could still add a note to pipestatus' usage message that it behaves as if

set -o pipefail; CMD1 | CMD2

was issued at the shell prompt, as John suggested IIRC.

Until we drop support for Bashs < 3.0 ...

(and/or generate the Makefiles, creating "customized" versions depending on the version of the shell found.)

In any case, it's better to not mess around with file descriptors that might already be used (i.e. use Bash's PIPESTATUS array instead, since we rely on bash anyway).


Because bash is in fact a prerequisite for Sage, I still see no reason for not making a contemporary version of it a requirement. If people lacking one are unable to build/install it themselves from the GNU tarball, they can still install pre-built binaries, which AFAIK need not even be provided by Sage as there are already such packages available elsewhere.

I wouldn't consider a shell a battery either, and we do care less about older / out-dated releases of other operating systems.


The ticket's description should perhaps be updated to reflect the outcome.

comment:45 Changed 11 years ago by jdemeyer

  • Authors changed from Jeroen Demeyer to Jeroen Demeyer, Leif Leonhardy
  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:46 Changed 11 years ago by leif

  • Description modified (diff)
  • Keywords shell environment tee exit code status added

Changed 11 years ago by jdemeyer

Apply on top of previous

Changed 11 years ago by jdemeyer

comment:47 Changed 11 years ago by jdemeyer

  • Resolution fixed deleted
  • Status changed from closed to new

comment:48 Changed 11 years ago by jdemeyer

  • Merged in sage-4.6.1.rc0 deleted
  • Status changed from new to needs_review

comment:49 Changed 11 years ago by jdemeyer

  • Description modified (diff)
  • Merged in set to sage-4.6.1.rc0
  • Resolution set to fixed
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.