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

Ensure that scripts don't swallow errors either #26

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dabrahams
Copy link
Contributor

No description provided.

@dabrahams
Copy link
Contributor Author

As I was attempting to do the same thing for Half I realized that there are a number of places where you use checkresult to attempt to give better diagnostics when there's a failure. This PR will undermine that checking. However, those checkresult $? calls often come after a bunch of steps and $? will only be nonzero if the last of those steps happens to fail. I'm not entirely clear on the best way to proceed here; I never try to write anything more than a few lines using shell scripts because among other things the error-handling model is so screwed up. If it were my project, I'd rewrite these scripts in Swift, have every failing command throw an Error, encode the details of the failure in the error, catch and report all errors in one place, and do some logging to the terminal before each section of work to give me the additional context I need to know what it was doing when the failure occurred.

@dabrahams
Copy link
Contributor Author

dabrahams commented Nov 13, 2023

An initial compromise step might be to accept this PR without the changes to workflowtests.sh and let you work out the approach for that file. Please advise.

Update: the latest commit reverts the changes to workflowtests.sh.

Its structure sort of depended on continuing in the face of errors,
even though the way it works is buggy.  That needs to be resolved by
@SomeRandomiOSDev.
Comment on lines +9 to +11
comparison=$("$(dirname "$0")/versions.sh" "$VERSION" "0.37.0"; echo $?)

if [ $? -ge 0 ]; then
if [ "$comparison" -ge 0 ]; then
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be necessary to do things in this way given how the versions.sh script exits, at least in theory. Are you not seeing this in your testing?

Copy link
Contributor Author

@dabrahams dabrahams Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could state your reasoning?

Here's mine: this script is using -e -o pipefail so it will exit with an error when any command it issues has a nonzero status, and versions.sh “returns” -1, 0, or 1 in its status code. If it exits with code -1, -e will cause this script to interpret it as a failure and exit immediately instead of continuing with the else clause... right? And your intention was that the else clause be reachable if versions.sh returns -1, right?

@SomeRandomiOSDev
Copy link
Owner

@dabrahams Good callouts. I hesitate to re-write these scripts in Swift just given how much of a pain it is to invoke terminal commands from Swift. As you pointed out, however, the error model for bash scripts is pretty terrible, so I guess it's a matter of picking a poison. I'll take a deeper look at these scripts overall but for the time being I'd like to keep them as is

@dabrahams
Copy link
Contributor Author

dabrahams commented Nov 16, 2023

@dabrahams Good callouts. I hesitate to re-write these scripts in Swift just given how much of a pain it is to invoke terminal commands from Swift.

The principled alternative is to use -e -o pipefail everywhere, and give up on the pretty error messages you get with checkresult. Other ways to know what failed: set +x (ugly but effective) or just echo something about what each step does to the console before each step, much the same way your use of checkresult does so after a failure.

@SomeRandomiOSDev how does that approach sound to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants