Description of problem: The tcsh documentation states that the $status (or $?) variables hold the return value of the last command. Therefore in case of pipelines it should be the last command of the pipeline. This doesn't seem to be working OK. Version-Release number of selected component (if applicable): tcsh-6.14-18.el5 How reproducible: Always Steps to Reproduce: 1. cat no-exist | wc -l 2. echo $status (or echo $?) Actual results: 1 Expected results: 0 Additional info: The $? variable should hold the return code of "wc -l" which is 0. Similarly for the backquoted pipeline: echo `cat no-exist | wc -l` ($status would be 1, expected value is 0).
Created attachment 450757 [details] Proposed patch This fixes the problem in the description in my environment. Not tested very thoroughly yet. The patch should cause tcsh to ignore exit value of processes with output redirected to a pipe.
Created attachment 453711 [details] Second attempt The previous patch didn't fix the case of echo `cat nonexistant` Expected return value of this expression is 0 ('echo' succeeded) however tcsh returned 1. The attached patch is supposed to fix that -- few first tests look OK. Comments welcome.
Our partner also verified the test package. <snip> The test package worked fine. </snip>
Have you tried to propose this patch to upstream?
Hello, I haven't send the fix upstream yet. I will try to find a place where to file a bug. To be honest -- if that would require me to subscribe to another mailing list, I'd rather leave it up to you... Regards.
I have submitted patch to upstream for you: http://bugs.gw.com/view.php?id=110
OK. Thank you.
The patch has been accepted by upstream (6.17.03b).
Upstream reverted the patch: "Undo PR/110 breaks unit tests."
The upstream tcsh 6.17.04 test suite with the _reverted_ patch applied: 110: Primary expressions FAILED (expr.at:84) AT_CHECK([tcsh -f -c 'exit { false }'], 0) 113: Escaping special characters FAILED (lexical.at:60) AT_DATA([nosplit.csh], [[echo \&\|\;\<\>\(\)\&\|\|\<\<\>\>\space\ tab\ end echo '&|;<>()&||<<>>space tab end' echo "&|;<>()&||<<>>space tab end" set verbose echo `&|;<>()&||<<>>space tab end` ]]) AT_CHECK([tcsh -f nosplit.csh], 1, [&|;<>()&||<<>>space tab end &|;<>()&||<<>>space tab end &|;<>()&||<<>>space tab end ], [echo `&|;<>()&||<<>>space tab end` Invalid null command. ]) 114: Preventing substitution FAILED (lexical.at:135) AT_CHECK([tcsh -f -c 'echo "`OK`"'], 1, [ ], [OK: Command not found. ]) 123: Command substitution FAILED (subst.at:17) AT_DATA([backq.csh], [[set a=(a`echo 1 2; echo 3 4`b) echo $#a set a=(a"`echo 1 2; echo 3 4`b") echo $#a unset csubstnonl echo `echo 1; \\ echo 2` set csubstnonl echo `echo 1; \\ echo 2` ]]) AT_CHECK([tcsh -f backq.csh], 1, [4 2 1 2 1 ], [ : Command not found. ]) 127: Command execution FAILED (syntax.at:159) AT_CHECK([tcsh -f -c '(echo $this_does_not_exist) |& cat'], 1, [this_does_not_exist: Undefined variable. ])
If I understand what the test macros actually do, then the tests actually expect a wrong behaviour. No wonder they broke...
The only failed test shows us a bug we should figure out: 110: Primary expressions FAILED (expr.at:84) AT_CHECK([tcsh -f -c 'exit { true }'], 1) AT_CHECK([tcsh -f -c 'exit { false }'], 0) $ ./tcsh $ exit { true } exit $ echo $? 1 $ ./tcsh $ exit { false } exit $ echo $? 1 Manual page tcsh(1) line 1736: exit [expr] The shell exits either with the value of the specified expr (an expression, as described under Expressions) or, without expr, with the value 0. Manual page tcsh(1) line 1085 Command exit status Commands can be executed in expressions and their exit status returned by enclosing them in braces (`{}'). Remember that the braces should be separated from the words of the command by spaces. Command executions succeed, returning true, i.e., `1', if the command exits with status 0, otherwise they fail, returning false, i.e., `0'. If more detailed status information is required then the command should be executed out‐side of an expression and the status shell variable examined.
OK. I'll try to take a look at the patch again. Thanks.
Created attachment 476793 [details] New patch Looks that tcsh treats {} and `` constructions similarly though their semantic is a bit different. Here's another patch which should fix the problem in the description and not regress on the 'exit { false }' test. I aimed for a minimal change so the patch is quite ugly: I set the process flag by the actual command string while allocating the process structure, which doesn't seem "right" -- IMHO it belongs to the parser... Feel free to rewrite -- I think where the problem lies is clear from the attached patch.
Created attachment 492015 [details] Patch to fix $status of pipelined/backquoted commands This is slightly modified patch by Tomas Smetana (Comment 16) that fixes possible segmentation fault.
Created attachment 492016 [details] Patch to fix the testsuite that expected wrong behaviour
The new patch has been accepted by upstream (6.17.06b).
Created attachment 492357 [details] Upstream accepted patch with additional $anyerror variable to select behavior
Bugfix was successfully verified on package tcsh617-6.17-4.el5 on all supported architecutres (i386, x86_64, s390x, ppc64 and ia64). $status (or $?) now contain return value of last command.
Technical note added. If any revisions are required, please edit the "Technical Notes" field accordingly. All revisions will be proofread by the Engineering Content Services team. New Contents: This package fixes the return value of the "status" (or "$?") variable in the case of pipelines and backquoted commands. The "anyerror" variable, which selects behavior, has been added to retain the backward compatibility.
An advisory has been issued which should help the problem described in this bug report. This report is therefore being closed with a resolution of ERRATA. For more information on therefore solution and/or where to find the updated files, please follow the link below. You may reopen this bug report if the solution does not work for you. http://rhn.redhat.com/errata/RHBA-2011-1072.html
So let me get this straight... csh/tcsh has been setting $status from pipelines and backquotes the same way for ~20 years now (going back to pure-csh on SunOS and including tcsh on RHEL 3,4,5, and 6)? But there's been a disconnect between the behavior and what's documented in the man page? So the solution is to give preference to the documentation? My entire enterprise is dependent on a piece of software that makes heavy use of csh/tcsh -- and for the past 20 years our customers have built more csh/tcsh infrastructure on top of it. tcsh617 (with this change) has already broken some of our scripts -- and I've identified more places where it possibly could. So now I'm supposed to accept the fact that my enterprise software's legacy of testing is negated if I a) install tcsh617, b) migrate to RHEL5.7 (which runs tcsh617) or c) migrate to RHEL 6.2 (which will probably ship with tcsh-6.17-13.el6: https://bugzilla.redhat.com/show_bug.cgi?id=658190)? And because I'm being affected by this bug #618723 (https://bugzilla.redhat.com/show_bug.cgi?id=618723) in RHEL5 *and* it's only been fixed in tcsh617 (and for some reason not backported into tcsh-6.14), I'm now forced to choose between two "solutions" that each break different parts of my software. The anyerror shell variable is a nice thought, but since it's a shell variable it won't propagate to subshells -- which makes it hard to globally affect "#!/bin/csh -f" scripts. IMO, this entire change should be completely redacted. This should have been considered a documentation bug and the manpage reworded to reflect the way the thing's been working forever. If you're still hellbent on introducing this behavior, then introduce a "lasterror" shell variable that I can set to **opt-in** to the new behavior, instead of opting-in to the old behavior.
(In reply to comment #31) > IMO, this entire change should be completely redacted. This should have been > considered a documentation bug and the manpage reworded to reflect the way the > thing's been working forever. The thing is, that the correct behaviour is defined by POSIX (see http://pubs.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html#tag_02_09_02 ). So, it's not just a 'random mistake' in man page. Our primary target was to fix the wrong behaviour and prevent compatibility issues with other POSIX-compliant shells (bash, ksh, resh etc.). Upstream tcsh developer, who maintains tcsh since 1987, has also accepted the changes (see http://bugs.gw.com/view.php?id=110 ), making this default behaviour starting with TCSH version 6.17.05. > If you're still hellbent on introducing this behavior, then introduce a > "lasterror" shell variable that I can set to **opt-in** to the new behavior, > instead of opting-in to the old behavior. However, I understand your concerns and I'm willing to discuss possible changes to fix your issue (reverting the changes / making the old behaviour default). But please, beware that Bugzilla is not a customer support tool. I recommend you contacting your support representative to escalate this request. [ Global Support Services, http://www.redhat.com/support/ ]
(In reply to comment #32) > The thing is, that the correct behaviour is defined by POSIX (see > http://pubs.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html#tag_02_09_02 > ). So, it's not just a 'random mistake' in man page. If you read that document, you'll realize it has absolutely nothing to do with csh/tcsh -- which are not part of the POSIX shell family. There are literally dozens of things in that document that are not compatible with csh/tcsh. > Our primary target was to fix the wrong behavior The "wrong" behavior has been that way for 20+ years -- which (for better or for worse) makes it the "right" behavior at this point. > and prevent compatibility issues with other POSIX-compliant shells (bash, ksh, resh etc.). Then your entire premise for this change was misguided -- another reason to redact it. Once again, POSIX-compliant shells and csh/tcsh are two completely different things. csh/tcsh will *never* be POSIX compliant -- they're not supposed to be compatible. > Upstream tcsh developer, who maintains tcsh since 1987, has also accepted the > changes (see http://bugs.gw.com/view.php?id=110 ), making this default > behaviour starting with TCSH version 6.17.05. I saw that and I'm surprised he actually accepted it. Remember, he hasn't actually *released* a version of tcsh with this change in it -- and once he realizes the impact, he may not release it. At this point, no one in the world has a tcsh in the wild that behaves this way except for Redhat. > However, I understand your concerns and I'm willing to discuss possible changes > to fix your issue (reverting the changes / making the old behaviour default). Here's the situation: csh/tcsh scripts that have been running for 20+ years on various platforms (including RHEL) will now potentially break if they're run on your new tcsh implementation. Your new tcsh is now incompatible with every other tcsh implementation. Plain and simple! In addition, the change you made was completely gratuitous -- there was no demand for it -- customer or otherwise. RHEL is touted as enterprise software -- your engineers can't go and start changing long-standing, well-established behaviors/APIs to match quirks in documentation. Maintaining stability within a release series should be paramount. Like I've proposed, the minimum you should do is make the new behavior opt-in (instead of opt-out). But even that is risky (e.g. a legacy script might happen to use the same shell variable name that activates this new behavior -- and their script runs different). > But please, beware that Bugzilla is not a customer support tool. I recommend > you contacting your support representative to escalate this request. > [ Global Support Services, http://www.redhat.com/support/ ] We have already opened a formal support request.
For the record, we're also trying to let upstream know that this was a bad idea: http://bugs.gw.com/view.php?id=155
(In reply to comment #35) > For the record, we're also trying to let upstream know that this was a bad > idea: > > http://bugs.gw.com/view.php?id=155 Thanks Angelo. I'm going to write a backward-compatible patch, that would rather opt-in the "new" behaviour.
(In reply to comment #36) > (In reply to comment #35) > > For the record, we're also trying to let upstream know that this was a bad > > idea: > > > > http://bugs.gw.com/view.php?id=155 > > Thanks Angelo. > > I'm going to write a backward-compatible patch, that would rather opt-in the > "new" behaviour. Thanks for responding to this. Making the new behavior opt-in is a much more sound approach. Although keep in mind that csh/tcsh makes no distinction between "special" shell variables that control the shell's behavior (e.g. noclobber, ignoreeof, noglob) and plain-old local variables used in a script. So, if someone already has a script where they're using "set" variables named "anyerror" or "lasterror" (or whatever your variable will be named), they could accidentally enable behavior they don't want. Ideally, I would just want to see tcsh left alone and absent of any "new" behaviors. Short of that, please choose your variable name judiciously to reduce the risk of collisions.
(In reply to comment #37) > Thanks for responding to this. I've created new bug 759132 to cover the Regression (we can't reopen this bugzilla, as it was already shipped within the ERRATA).
(In reply to comment #40) > (In reply to comment #37) > > Thanks for responding to this. > > I've created new bug 759132 to cover the Regression (we can't reopen this > bugzilla, as it was already shipped within the ERRATA). Does this mean that behavior such as ... -------- punkin: set r = `ls blotto` ls: cannot access blotto: No such file or directory punkin: echo $status 0 punkin: bash bash-4.1$ r=`ls blotto` ls: cannot access blotto: No such file or directory bash-4.1$ echo $? 2 bash-4.1$ exit exit punkin: ksh $ r=`ls blotto` ls: cannot access blotto: No such file or directory $ echo $? 2 $ exit punkin: echo $version tcsh 6.17.00 (Astron) 2009-07-10 (x86_64-unknown-linux) options wide,nls,dl,al,kan,rh,color,filec -------- will change such that tcsh will behave as the other shells when the rightmost member of an RHS pipeline returns non-zero exit status? Or are the other shells going to be changed to match tcsh (the login shell)? I've tried to follow the chain of 'issues' on this bug and haven't made much headway so I decided to post here. Seems to me that the attempt to fix the original report introduced a new bug. Thanks, Phil
Hi Phil, please, take a look at bug 759132 (or bug 784510 for RHEL-6). We are going to revert the changed functionality to tcsh default (as it used to work for many years). There will be new tcsh_posix_status variable available to opt-in POSIX-like behaviour (as in bash, ksh etc.).
Phil, Prior to Redhat changing this behavior in TCSH, it would have returned a non-zero exit status in your example (that's what csh/tcsh has done for decades). What I find ironic is part of their justification was to "make tcsh behave more like POSIX-shells" (bash, ksh, sh). But in the examples you posted, they actually made it behave *differently* than POSIX-shells. IMO, another glaring example of why these changes were so brash and ill-advised. -Angelo