Skip to content
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

Merged
merged 5 commits into from
Apr 7, 2023
Merged

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Dec 18, 2022

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.

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @merwok for commit 65d6cdb 🤖

If you want to schedule another build, you need to add the :hammer: test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 11, 2023
@zware
Copy link
Member

zware commented Feb 8, 2023

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 SHELL := $(SHELL) -e?

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.
@mgorny
Copy link
Contributor Author

mgorny commented Feb 9, 2023

This looks good, but is going to need some conflict resolution now after GH-100329.

Done.

I wonder if this could be achieved with less churn by setting SHELL := $(SHELL) -e?

Hmm, I haven't though of that. I suppose it could work too.

@merwok
Copy link
Member

merwok commented Feb 9, 2023

Please use regular merges and pushes for Python repos, force-pushes give reviewers a poor experience on github.

@mgorny
Copy link
Contributor Author

mgorny commented Mar 2, 2023

@zware, do you want to rework this to try updating this to use SHELL right now or is this good as-is?

@zware
Copy link
Member

zware commented Mar 2, 2023

If the SHELL method works and doesn't break anything else, I'd rather go that route. That way we don't have to worry about making sure new commands start with the right magic incantation, and the history is much cleaner.

Instead of setting `set -e` per target, add it to the definition
of `SHELL` to force it globally.  This was suggested by Zachary Ware.
@mgorny
Copy link
Contributor Author

mgorny commented Mar 3, 2023

Switched to the SHELL approach, though the Makefile already sets SHELL, so I've just appended -e there. I've tested it and it seems to work fine: successful build succeeds, and broken build fails.

@zware zware added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 3, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @zware for commit 86c946f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 3, 2023
@zware
Copy link
Member

zware commented Mar 6, 2023

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?

@mgorny
Copy link
Contributor Author

mgorny commented Apr 5, 2023

Ping.

@arhadthedev arhadthedev added the build The build process and cross-build label Apr 5, 2023
@zware zware merged commit a90863c into python:main Apr 7, 2023
@mgorny mgorny deleted the make-fail-100220 branch April 8, 2023 05:14
@mgorny
Copy link
Contributor Author

mgorny commented Apr 8, 2023

Thank you!

warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants