Skip to content

Do not use os._exit when build-script fatal errors #79365

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

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

bnbarham
Copy link
Contributor

os._exit does not run any cleanup handlers or flush any buffers, which means that we may end up missing helpful log output. It was being used to avoid printing the log analysis when --help was passed, but we can have the same behavior by just not using finally.

@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham
Copy link
Contributor Author

This keeps the same behavior, which TBH I'm not sure is all that intentional. Presumably the finally: log_analyzer() was intended to have it run no matter what, but a later commit added the except SystemExit as e to avoid printing it for --help (as argparse exits with 1 by default for help output). All the various fatal_error we have use sys.exit in one form or another though, so... we don't output in those cases. Maybe that's fine?

In any case, I just want to fix errors not being output when they should be today and this at least fixes that.

`os._exit` does not run any cleanup handlers or flush any buffers, which
means that we may end up missing helpful log output. It was being used
to avoid printing the log analysis when `--help` was passed, but we can
have the same behavior by just not using `finally`.
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham merged commit b221a92 into swiftlang:main Feb 14, 2025
5 checks passed
@bnbarham bnbarham deleted the build-script-error branch February 14, 2025 17:22
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.

3 participants