-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-100220: Fix error handling in make rules #100328
Conversation
This looks good, but is going to need some conflict resolution now after GH-100329. I wonder if this could be achieved with less churn by setting |
Use `set -e` before compound shell commands in order to ensure that make targets fail correctly when at least one of the subcommands fail. This is necessary since make considers a target failed only if one of the shell invocations returns with unsuccessful exit status. If a shell script does not exit explicitly, the shell uses the exit status of the *last* executed command. This means that when multiple commands are executed (e.g. through a `for` loop), the exit statuses of prior command invocations are ignored. This can be either resolved by adding an explicit `|| exit 1` to every command that is expected to succeed, or by running the whole script with `set -e`. The latter was used here as it the rules seem to be written with the assumption that individual commands were supposed to cause the make rules to fail.
65d6cdb
to
6c05684
Compare
Done.
Hmm, I haven't though of that. I suppose it could work too. |
Please use regular merges and pushes for Python repos, force-pushes give reviewers a poor experience on github. |
@zware, do you want to rework this to try updating this to use |
If the |
Instead of setting `set -e` per target, add it to the definition of `SHELL` to force it globally. This was suggested by Zachary Ware.
Switched to the |
The two buildbot failures don't appear to have anything to do with this; they shouldn't block a merge. @kushaldas, you had assigned yourself to this one; WDYT? |
Ping. |
Misc/NEWS.d/next/Build/2022-12-18-07-24-44.gh-issue-100220.BgSV7C.rst
Outdated
Show resolved
Hide resolved
Thank you! |
Set `SHELL = /bin/sh -e` to ensure that complex recipes fail on the first error rather than incorrectly reporting success. Co-authored-by: Zachary Ware <[email protected]>
Use
set -e
before compound shell commands in order to ensure that make targets fail correctly when at least one of the subcommands fail.This is necessary since make considers a target failed only if one of the shell invocations returns with unsuccessful exit status. If a shell script does not exit explicitly, the shell uses the exit status of the last executed command. This means that when multiple commands are executed (e.g. through a
for
loop), the exit statuses of prior command invocations are ignored.This can be either resolved by adding an explicit
|| exit 1
to every command that is expected to succeed, or by running the whole script withset -e
. The latter was used here as it the rules seem to be written with the assumption that individual commands were supposed to cause the make rules to fail.make sharedinstall
does not return failure if install commands fail #100220