The way we define 'assert' is highly confusing and completely different from all regular programming languages. Additionally, the function is barely used and can be easily replaced by less confusing direct PIPESTATUS check. I think we should ban it.
Huh? What would be used as replacement? Testing PIPESTATUS is not a viable alternative because it is clumsy and cannot be done without a loop. IMHO we shouldn't remove working functionality without good reason.
Actually, the name of the function had been discussed in gentoo-dev. Apparently, assert_true_or_die() and success_or_die() had been proposed as alternative names and they finally settled on the assert() variant: https://archives.gentoo.org/gentoo-dev/message/f11752e18c1f8f6732ceaaac16964ffa https://archives.gentoo.org/gentoo-dev/message/b0ffef8809ea760d53a06d9c25265c50
How about enabling pipefail instead?
(In reply to Michał Górny from comment #3) > How about enabling pipefail instead? I don't think that's really feasible, for similar reasons that using 'set -e' was rejected in favor of just having helpers die by default in EAPI 4 (see bug 138792).
(In reply to Zac Medico from comment #4) > (In reply to Michał Górny from comment #3) > > How about enabling pipefail instead? > > I don't think that's really feasible, for similar reasons that using 'set > -e' was rejected in favor of just having helpers die by default in EAPI 4 > (see bug 138792). Actually, after reading the pipefail documentation in the bash man page, I think the behavior might be reasonable (as long as we don't also enable 'set -e'.
I'm with Zac here. Rewording what the manual says: - without it, $? has the exit code of last process (i.e. PIPESTATUS[-1]), - with it, $? has the exit code of last *failing* process or 0 if none failed. I don't think we really have a use for the former behavior (and even if, it can be done more clearly using PIPESTATUS), while the latter makes $? have behavior equal to the current assert implementation. In fact, it makes it possible to safely just: cmd1 | cmd2 | cmd3 || die ...
Can you please open a new bug for pipefail? Seems that enabling that option doesn't necessarily imply banning assert.
(In reply to Michał Górny from comment #6) > I don't think we really have a use for the former behavior (and even if, it > can be done more clearly using PIPESTATUS) Example based on a line in autotools-utils.eclass (which I realise is deprecated these days; it was just the first concrete example I found in the tree): $ ./configure --help 2>&1 | grep -q '^ *--docdir='; echo $? 0 $ set -o pipefail $ ./configure --help 2>&1 | grep -q '^ *--docdir='; echo $? 141 (Done with paludis's configure since that's what I had conveniently available, but it would happen with any configure script whose --help output is long enough that the grep would exit without reading it all.) It's not a fatal strike against the idea, because as you say it can be written in other ways, but there is a valid reason for the default behaviour, and it may be considered undesirable to make people jump through hoops to get the same thing. Worse, the "obvious" way to write it may work when first written, but then fail if something causes the output to get longer - in some cases, although probably not when testing ./configure output, it may even depend on the local configuration, and so might break for users when it worked for the developer. (Apologies for continuing the OT on this bug, but the one for pipefail hasn't been opened yet.)
As we won't have pipefail, I don't think that we will go forward with this one either. WONTFIX.
This just confused me horribly in https://github.com/gentoo/gentoo/pull/38985#discussion_r1802547730 Per IRC discussion we should probably e.g. rename this to "pipestatus || die". The naming issue is significant regardless of whether enabling global pipefail was the specific technology used as the successor.
This was discussed in #gentoo-qa today. Idea: Introduce a pipestatus function in the next EAPI: pipestatus() { local ret for ret in "${PIPESTATUS[@]}"; do [[ ${ret} -eq 0 ]] || return "${ret}" done return 0 } "assert -n" and "assert" could then be replaced by "pipestatus" and "pipestatus || die", respectively. Presumably its being longer isn't a problem because assert isn't used very frequently.
I agree with the overall premise of comment 1. I would further emphasise that the pipefail shell option is one of the many features of bash that serves to demonstrate that all that glitters is not gold. It should only ever be employed selectively - preferably in a subshell - and even then, with caution. Had it not been hashed out already, I would have elaborated. The idea presented by comment 11 is in comparatively good taste and would further permit useful constructs such as "pipestatus || return". Even then, programmers should elect to use PIPESTATUS wherever it is appropriate. Speaking of which, programmers also need to be mindful of the use of process substitutions. After all, they serve as another means of hooking up pipes but are not covered by PIPESTATUS. There probably exists a fair amount of code that either ought to be using wait "$!" to reap the exit status of the last asynchronous command (bash 4.4+ only), or which ought to be using a conventional pipeline instead. P.S. "return 0" is redundant in the sample function.
(In reply to Ulrich Müller from comment #11) > pipestatus() { > local ret > for ret in "${PIPESTATUS[@]}"; do > [[ ${ret} -eq 0 ]] || return "${ret}" > done > return 0 > } Actually, this implementation won't work because "local ret" clears the status. Presumably, we also want to be consistent with pipefail behaviour, which takes the _last_ nonzero status. The Bash manual says in section 3.2.3 Pipelines: | If 'pipefail' is enabled, the pipeline's return status is the value | of the last (rightmost) command to exit with a non-zero status, | or zero if all commands exit successfully. So: pipestatus() { local -a status=( "${PIPESTATUS[@]}" ) local s ret=0 for s in "${status[@]}"; do [[ ${s} -ne 0 ]] && ret=${s} done return "${ret}" } Alternatively, one could loop backwards over the array (and return for the first nonzero status) but it would be more complicated. I don't think that would be more efficient because the normal case is that all statūs are zero. Plus, pipelines rarely consist of more than 3 commands.
It isn't necessary to use local at all. pipestatus() { for _ in "${PIPESTATUS[@]}"; do [[ $_ -eq 0 ]] || return "$_" done } In action: # : | (exit 2) | : # pipestatus; echo "$?" 2 # : | : | : # pipestatus; echo "$?" 0
That doesn't interfere with the normal operation of the _ variable, by the way. It will still be set as it otherwise would have been once the command incorporating pipestatus has concluded.
Apologies, I misinterpreted the intended shape of the interface. Early morning comments aren't my forte. Given that the right-most non-zero status is desired, the code in comment 13 is as good as it can reasonably be.
(In reply to kfm from comment #14) > pipestatus() { > for _ in "${PIPESTATUS[@]}"; do > [[ $_ -eq 0 ]] || return "$_" > done > } Regardless that this won't work as intended (as you noted in comment #16), I dislike this sort of trickery. It has minimal gain, at the cost of making the code more brittle. For example, it would break if any other command was inserted before the test. And it _does_ abuse a global variable for a purpose it was not meant for.
Discussion in #gentoo-pms on 2024-11-09: <+flow> hmm, i feel this was commented on the bug already, but we also need a way to specify known non-zero exit status(es) per pipe position, the prime example here is grep which returns 1 if nothing matched [...] <@ulm> flow: we could add some option to mask specific positions <@ulm> but maybe we should keep it simple. if you need anything more specific, you can always test PIPESTATUS directly <@ulm> it's similar to pipefail which is also all or nothing [...] <+flow> ulm: "if you need anything more specific, you can always test PIPESTATUS directly" it's not uncommon that 'grep' is used in a pipe, and checking for this is fragile. I assume you still remember when we implemented the the PIPESTATUS check for tex? <+flow> hence I would argue that it's something the function should provide <+flow> like "-s <pipe-pos> <success-status> [<success-status>]" [...] <@ulm> flow: only that I understand it right, if you had foo|bar|baz and bar could return 0 or 1, then you'd do "pipestatus -s 1 1"? <@ulm> having multiple option arguments would be unusual though, maybe "pipestatus -c 1 -s 1" would be less confusing [...] <+flow> ulm: yes, I think you understood it right <@ulm> commands would be numbered from 0 for leftmost? <+flow> no strong opinion, I would probably use what bash uses. IIRC bash is zero-indexed? <@ulm> yes <@ulm> I must think about this. idea is good but syntax is ugly :) <@ulm> pipestatus [<position>:<status>[,<status>]...]... <@ulm> e.g. pipestatus 1:1,2 if command #1 is allowed to exit with 1 or 2 <+flow> wfm Thinking about it, allowing such extra arguments would not harm, because it would not make the trivial use case more complicated. The following function would implement this: pipestatus() { local -a status=( "${PIPESTATUS[@]}" ) local -a ok local arg i for arg; do [[ ${arg} =~ ^[0-9]+:[0-9]+(,[0-9]+)*$ ]] \ || die "${FUNCNAME}: bad argument: ${arg}" ok[${arg%%:*}]=${arg#*:} done for (( i=${#status[@]}-1; i>=0; i-- )); do [[ ${status[i]} -eq 0 ]] || has ${status[i]} ${ok[i]//,/ } \ || return ${status[i]} done return 0 } Some test cases: $ true | true | true; pipestatus; echo $? 0 $ true | false | true; pipestatus; echo $? 1 $ true | false | true; pipestatus 1:1; echo $? 0 $ true | { exit 2; } | true; pipestatus 1:1; echo $? 2 In particular, the "fragile grep" mentioned by flow: $ true | grep foo | true; pipestatus 1:1; echo $? 0 $ true | grep '[' | true; pipestatus 1:1; echo $? grep: Invalid regular expression 2
That code is literally horrible, and making both the function unreadable and its uses potentially confusing is not really worth it for a corner case.
(In reply to Michał Górny from comment #19) > That code is literally horrible, and making both the function unreadable and > its uses potentially confusing is not really worth it for a corner case. Not sure if using grep in a pipeline can be called a corner case. Anyway, if we would just allow "pipestatus [<position>:<status>]..." then the implementation simplifies: pipestatus() { local -a status=( "${PIPESTATUS[@]}" ) local i ret=0 for i in "${!status[@]}"; do [[ ${status[i]} -ne 0 ]] && ! has "${i}:${status[i]}" "$@" \ && ret=${status[i]} done return "${ret}" }
(In reply to Michał Górny from comment #19) > That code is literally horrible, and making both the function unreadable and > its uses potentially confusing is not really worth it for a corner case. I had wearily contemplated writing a longer comment about it before resigning myself to stay out of it but seeing your comment strengthened my resolve. The proposed interface is dubious, both in merit and in design. The proposed code is typical Gentoo slop [*]. Its ensuing use would result in xtrace becoming yet more insufferable than it already is. > true | grep '[' | true; pipestatus 1:1; echo $? Solution #1 (( PIPESTATUS[0] == 0 && PIPESTATUS[1] < 2 && PIPESTATUS[0] == 0 )) Solution #2 For those that simply cannot bear to write a coherent arithmetic expression to begin with and don't care who knows it. local IFS=, ... [[ ${PIPESTATUS[*]} == 0,[01],0 ]] Still much easier to understand and more reliable than the proposed slop, as well as more pleasant to trace and debug. Solution #3 For those that don't like that solution #2 affects IFS in a broader scope than is strictly necessary. # Let's assume that this is a generally available helper function. splat() { printf %s "$*"; } ... [[ $(IFS=, splat "${PIPESTATUS[@]}") == 0,[01],0 ]] Conclusion We don't "need" it. * Not merely an overwrought solution in search of a problem. From a few seconds of reading: tolerates invalid octal numbers that would cause errors as array subscripts; employs unquoted expansions (IFS-unsafe by definition, without even bothering to localise IFS to compensate).
Looks like feedback is mostly negative. Closing.
(In reply to kfm from comment #21) > tolerates invalid octal numbers that would cause errors as array > subscripts; Where do you see that (assuming we're talking about comment #20)? > employs unquoted expansions (IFS-unsafe by definition, without > even bothering to localise IFS to compensate). Again, where?
(In reply to kfm from comment #21) > Solution #1 > > (( PIPESTATUS[0] == 0 && PIPESTATUS[1] < 2 && PIPESTATUS[0] == 0 )) Reopening, because the typo in the third test nicely proves the point that the function is useful. In an ebuild such kind of mistakes might go unnoticed for a long time, while the function would reliably catch it.
(In reply to Ulrich Müller from comment #23) > (In reply to kfm from comment #21) > > tolerates invalid octal numbers that would cause errors as array > > subscripts; > > Where do you see that (assuming we're talking about comment #20)? I was referring to comment #19, which went to the effort of performing parameter validation while permitting invalid octal numbers that would be expanded as an array subscript. I had not seen comment #20 at the time of writing my comment. But that's neither here nor there. I was a little surprised by your decision to temporarily close the bug. Lest we forget, this bug exists as a result of criticism concerning assert. It can be seen that there are multiple individuals, myself included, who were on board with the notion of having a pipefail function as it was originally envisaged. For the record, my negative feedback concerns the turn taken by comment #18 and I stand by it.
Rather, #18, not #19. Anyway.
(In reply to Ulrich Müller from comment #24) > (In reply to kfm from comment #21) > > Solution #1 > > > > (( PIPESTATUS[0] == 0 && PIPESTATUS[1] < 2 && PIPESTATUS[0] == 0 )) > > Reopening, because the typo in the third test nicely proves the point that > the function is useful. As everyone surely knows, single-character typos cannot possibly occur unless they happen to be situated within an arithmetic expression and happen to involve the subscripts of the PIPESTATUS array. > > In an ebuild such kind of mistakes might go unnoticed for a long time, while > the function would reliably catch it. Thy will be done. It's a rather curious flex to have re-opened the bug on that basis alone, almost immediately after closing it because, prima facie, an idea that strayed outside the initial scope of the bug was subjected to criticism by an outsider. It speaks to something other than my error rate, like as not.
Yeah I'd quite like if we could get back to solving the issue where "assert" is horribly confusing and existing uses of it should be banned and replaced by something less confusing such as "pipestatus" with no args. Everyone is agreed the current situation is a problem. There is *zero* consensus for closing this bug and this is now the second time it's been closed for inexplicable reasons. As before: (In reply to Eli Schwartz from comment #10) > The naming issue is significant regardless of whether enabling global > pipefail was the specific technology used as the successor. Well, it's also significant regardless of whether adding complex APIs to check specific error codes 3/15ths of the way through the world's most engineered 15-stage pipeline, is "the specific technology used as the successor". How many times will the same mistake be repeated? Please keep the bug open until "ban or rename assert" is fixed, something that comment 22 didn't demonstrate.
(In reply to Eli Schwartz from comment #28) Closing the bug was an overreaction and I apologise for it. The confirmed status is now reinstated. Then again, comments like #19 and #21 (and also #14 earlier) in reply to some proof-of-concept code don't further the discussion. This it a future-EAPI bug, so it should be clear that we are discussing the concept here. Any posted code snippets are only meant to exemplify the concept. With very few exceptions (namely, default phase functions) the spec even avoids code listings, and leaves the implementation to package manager authors. IMHO the questions to discuss are: a) Are we happy with this being a test function returning a status, or should it die by itself (which could be suppressed by an option like -n)? My impression so far was that it should be the former, because assert dying by itself is was we have now. b) Should the command accept any arguments for expected exit status? This would be for cases like grep in the middle of a pipe.
Even if grep is common, its behavior is very specific, so we're either talking of either: a) adding a simple but non-generic feature here, which seems wrong for a generic function, or b) adding a generic feature that would be complex but in practice would only be used in very specific case. In my opinion, even adding a wrapper to coalesce grep exit status would be a cleaner approach here, however bad an idea that is.
Here are two examples from the tree for manually checking "grep" pipelines. First, in texlive-module.eclass: find ... | grep ... | xargs ... local pipestatus="${PIPESTATUS[*]}" [[ ${pipestatus} == "0 "[01]" 0" ]] eend $? || die "error installing man pages (pipestatus: ${pipestatus})" This may also suffer from an IFS problem. Also, the price to pay for its being compact is that it is not the most beautiful code. :) Second, in linux-firmware-99999999.ebuild: find ... | grep ... | xargs ... if [[ ${PIPESTATUS[0]} -ne 0 ]]; then die "..." elif [[ ${PIPESTATUS[1]} -eq 2 ]]; then die "Grep failed ..." elif [[ ${PIPESTATUS[2]} -ne 0 ]]; then die "..." fi This is in place since 4 years, and nobody noticed that the test is incorrect.
(In reply to Michał Górny from comment #30) > Even if grep is common, its behavior is very specific, so we're either > talking of either: > > a) adding a simple but non-generic feature here, which seems wrong for a > generic function, or > > b) adding a generic feature that would be complex but in practice would only > be used in very specific case. > > In my opinion, even adding a wrapper to coalesce grep exit status would be a > cleaner approach here, however bad an idea that is. One might as well just integrate the test into the pipeline at that juncture. $ true | { grep '[whoever tests their patterns?'; (( $? < 2 )); } | true grep: Unmatched [, [^, [:, [., or [= $ pipestatus; echo "$?" 1 Such is also simple and easy to understand. Granted, it doesn't convey the exit status of grep but I don't believe for a second that anyone will care. Partly because the use case comes across as being contrived - per "b)" - but, more importantly, because it still serves the intended purpose of suitably ascertaining the success of the pipeline as a whole. And "pipestatus || die ..." would be a common case in practice.
(In reply to Michał Górny from comment #30) > Even if grep is common, its behavior is very specific, so we're either > talking of either: > > a) adding a simple but non-generic feature here, which seems wrong for a > generic function, or > > b) adding a generic feature that would be complex but in practice would only > be used in very specific case. I don't understand what the criterion for a "generic feature" is, but probably it doesn't matter. I see only the two examples from my previous comment #31 in the tree that are expecting a nonzero PIPESTATUS element. That may indeed not justify implementing options to the function. (In reply to kfm from comment #32) > One might as well just integrate the test into the pipeline at that juncture. > > $ true | { grep '[whoever tests their patterns?'; (( $? < 2 )); } | true > grep: Unmatched [, [^, [:, [., or [= > $ pipestatus; echo "$?" > 1 Sure, working around the problem isn't difficult. I had posted a similar snippet to #-pms last week: <@ulm> foo | { grep a || [[ $? -eq 1 ]]; } | baz
We can also replace grep with sed, i.e.: grep foo -> sed -ne '/foo/p' that gives expected exit status. And given that sometimes people actually combine grep and sed, we're saving one grep invocation.
(In reply to Ulrich Müller from comment #31) > Here are two examples from the tree for manually checking "grep" pipelines. > First, in texlive-module.eclass: > > find ... | grep ... | xargs ... > local pipestatus="${PIPESTATUS[*]}" > [[ ${pipestatus} == "0 "[01]" 0" ]] > eend $? || die "error installing man pages (pipestatus: ${pipestatus})" > > This may also suffer from an IFS problem. Also, the price to pay for its > being compact is that it is not the most beautiful code. :) Yes, it's IFS-unsafe (also in lieu of $?). It's certainly not the most aesthetically pleasing way of going about it. > > > Second, in linux-firmware-99999999.ebuild: > > find ... | grep ... | xargs ... > if [[ ${PIPESTATUS[0]} -ne 0 ]]; then > die "..." > elif [[ ${PIPESTATUS[1]} -eq 2 ]]; then > die "Grep failed ..." > elif [[ ${PIPESTATUS[2]} -ne 0 ]]; then > die "..." > fi > > This is in place since 4 years, and nobody noticed that the test is > incorrect. I did! My personal linux-firmware ebuild (circa March 2024) is bereft of this defect. Now, as to why I never bothered reporting it, that's a story that doesn't belong here. Suffice to say that I put in many days worth of my time improving linux-firmware on multiple fronts, only to reach an impasse. That particular defect ended up being just one of many paper-cut issues, and I did not have the motivation to do anything much about it thereafter. If anything, it demonstrates the importance of routine review by those that have the pre-requisite degree of skill to rapidly read - and diagnose problems in - shell code. Saying and doing being two disparate things, of course.
eapi9-pipestatus.eclass has been proposed as the first step (to make an implementation available to packages immediately): https://public-inbox.gentoo.org/gentoo-dev/u4j3wrf90@urania.mail-host-address-is-not-set/T/#u
Specification, intended for Council pre-approval: pipestatus Tests the shell's pipe status array, i.e. the exit status of the command(s) in the most recently executed foreground pipeline. Returns shell true (0) if all elements are zero, or the last non-zero element otherwise. If called with -v as the first argument, also outputs the pipe status array as a space-separated list. Only available in EAPIs listed in table 12.x as supporting pipestatus. Table 12.x: EAPIs supporting pipestatus ------------------------------------------------ EAPI Supports pipestatus? ------------------------------------------------ 0, 1, 2, 3, 4, 5, 6, 7, 8 No 9 Yes ------------------------------------------------
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=94ee6e4ec34ece5759c6398cf6d33cf66c5e2687 commit 94ee6e4ec34ece5759c6398cf6d33cf66c5e2687 Author: Ulrich Müller <ulm@gentoo.org> AuthorDate: 2024-11-24 18:25:26 +0000 Commit: Ulrich Müller <ulm@gentoo.org> CommitDate: 2024-12-09 18:48:10 +0000 eapi9-pipestatus.eclass: New eclass This implements the pipestatus command, as proposed for EAPI 9: | Checks the shell's pipe status array, i.e. the exit status of the | command(s) in the most recently executed foreground pipeline. | Returns shell true (0) if all elements are zero, or the last | non-zero element otherwise. If called with -v as the first argument, | also outputs the pipe status array as a space-separated list. Bug: https://bugs.gentoo.org/566342 Signed-off-by: Ulrich Müller <ulm@gentoo.org> eclass/eapi9-pipestatus.eclass | 60 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)
It may also be noteworthy that there is no easy way to prevent "assert" from dying. "assert -n" (e.g. in https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=e1bfef38a962560d37781ca63dce6338f9c06dba) will die in spite of the "-n" option because it isn't called under nonfatal. "nonfatal assert -n" won't work either because nonfatal does some tests (in particular "if [[ $# -lt 1 ]]; then die ...; fi") that will mess up PIPESTATUS. Therefore "nonfatal assert -n" will always return 1, regardless of the exit status of the preceding pipeline.
FTR, this was approved by council on 2014-12-08 [1]. 1: https://projects.gentoo.org/council/meeting-logs/20241208.txt
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/pkgcore/pkgcore.git/commit/?id=b8a2cd690c871c96d9e240a0da29d83df1a36426 commit b8a2cd690c871c96d9e240a0da29d83df1a36426 Author: Arthur Zamarin <arthurzam@gentoo.org> AuthorDate: 2025-03-21 17:34:16 +0000 Commit: Arthur Zamarin <arthurzam@gentoo.org> CommitDate: 2025-03-21 17:36:01 +0000 EAPI=9: ban assert and introduce pipestatus instead Bug: https://bugs.gentoo.org/566342 Signed-off-by: Arthur Zamarin <arthurzam@gentoo.org> data/lib/pkgcore/ebd/eapi/9/global.bash | 21 +++++++++++++++++++++ data/lib/pkgcore/ebd/eapi/9/phase.bash | 1 + 2 files changed, 22 insertions(+)
