From 023dcd44c1e628bb654b5472418d6a346b510a71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Verschelde?= Date: Wed, 6 Mar 2024 18:50:35 +0100 Subject: [PATCH] Refactor OS exit code to be `EXIT_SUCCESS` by default - `Main::setup` early exits (failure or `--help`/`--version`) now consistently return `EXIT_FAILURE` or `EXIT_SUCCESS` on all platforms, instead of 255 on some and a Godot Error code on others. - `Main::start` now returns the exit code, simplifying the handling of early failures. - `Main::iteration` needs to explicit set the exit code in OS if it errors out. - Web and iOS now properly return `OS::get_exit_code()` instead of 0. --- core/os/os.h | 3 +- main/main.cpp | 96 +++++++++++++--------------- main/main.h | 2 +- platform/ios/godot_ios.mm | 11 ++-- platform/linuxbsd/godot_linuxbsd.cpp | 13 ++-- platform/macos/godot_main_macos.mm | 19 +++--- platform/web/web_main.cpp | 16 ++--- platform/windows/godot_windows.cpp | 8 ++- 8 files changed, 82 insertions(+), 86 deletions(-) diff --git a/core/os/os.h b/core/os/os.h index e0dda0b1554..4f0df1543fa 100644 --- a/core/os/os.h +++ b/core/os/os.h @@ -56,7 +56,8 @@ class OS { bool _verbose_stdout = false; bool _debug_stdout = false; String _local_clipboard; - int _exit_code = EXIT_FAILURE; // unexpected exit is marked as failure + // Assume success by default, all failure cases need to set EXIT_FAILURE explicitly. + int _exit_code = EXIT_SUCCESS; bool _allow_hidpi = false; bool _allow_layered = false; bool _stdout_enabled = true; diff --git a/main/main.cpp b/main/main.cpp index ec66d479010..4380886099f 100644 --- a/main/main.cpp +++ b/main/main.cpp @@ -766,6 +766,7 @@ Error Main::test_setup() { return OK; } + // The order is the same as in `Main::cleanup()`. void Main::test_cleanup() { ERR_FAIL_COND(!_start_success); @@ -977,8 +978,9 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph packed_data->add_pack_source(zip_packed_data); #endif - // Default exit code, can be modified for certain errors. - Error exit_code = ERR_INVALID_PARAMETER; + // Exit error code used in the `goto error` conditions. + // It's returned as the program exit code. ERR_HELP is special cased and handled as success (0). + Error exit_err = ERR_INVALID_PARAMETER; I = args.front(); while (I) { @@ -1028,12 +1030,12 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph } else if (I->get() == "-h" || I->get() == "--help" || I->get() == "/?") { // display help show_help = true; - exit_code = ERR_HELP; // Hack to force an early exit in `main()` with a success code. + exit_err = ERR_HELP; // Hack to force an early exit in `main()` with a success code. goto error; } else if (I->get() == "--version") { print_line(get_full_version_string()); - exit_code = ERR_HELP; // Hack to force an early exit in `main()` with a success code. + exit_err = ERR_HELP; // Hack to force an early exit in `main()` with a success code. goto error; } else if (I->get() == "-v" || I->get() == "--verbose") { // verbose output @@ -2459,7 +2461,7 @@ error: OS::get_singleton()->finalize_core(); locale = String(); - return exit_code; + return exit_err; } Error _parse_resource_dummy(void *p_data, VariantParser::Stream *p_stream, Ref &r_res, int &line, String &r_err_str) { @@ -3139,7 +3141,10 @@ String Main::get_rendering_driver_name() { // everything the main loop needs to know about frame timings static MainTimerSync main_timer_sync; -bool Main::start() { +// Return value should be EXIT_SUCCESS if we start successfully +// and should move on to `OS::run`, and EXIT_FAILURE otherwise for +// an early exit with that error code. +int Main::start() { ERR_FAIL_COND_V(!_start_success, false); bool has_icon = false; @@ -3276,7 +3281,7 @@ bool Main::start() { { Ref da = DirAccess::open(doc_tool_path); - ERR_FAIL_COND_V_MSG(da.is_null(), false, "Argument supplied to --doctool must be a valid directory path."); + ERR_FAIL_COND_V_MSG(da.is_null(), EXIT_FAILURE, "Argument supplied to --doctool must be a valid directory path."); } #ifndef MODULE_MONO_ENABLED @@ -3311,11 +3316,11 @@ bool Main::start() { // Create the module documentation directory if it doesn't exist Ref da = DirAccess::create_for_path(path); err = da->make_dir_recursive(path); - ERR_FAIL_COND_V_MSG(err != OK, false, "Error: Can't create directory: " + path + ": " + itos(err)); + ERR_FAIL_COND_V_MSG(err != OK, EXIT_FAILURE, "Error: Can't create directory: " + path + ": " + itos(err)); print_line("Loading docs from: " + path); err = docsrc.load_classes(path); - ERR_FAIL_COND_V_MSG(err != OK, false, "Error loading docs from: " + path + ": " + itos(err)); + ERR_FAIL_COND_V_MSG(err != OK, EXIT_FAILURE, "Error loading docs from: " + path + ": " + itos(err)); } } @@ -3323,11 +3328,11 @@ bool Main::start() { // Create the main documentation directory if it doesn't exist Ref da = DirAccess::create_for_path(index_path); err = da->make_dir_recursive(index_path); - ERR_FAIL_COND_V_MSG(err != OK, false, "Error: Can't create index directory: " + index_path + ": " + itos(err)); + ERR_FAIL_COND_V_MSG(err != OK, EXIT_FAILURE, "Error: Can't create index directory: " + index_path + ": " + itos(err)); print_line("Loading classes from: " + index_path); err = docsrc.load_classes(index_path); - ERR_FAIL_COND_V_MSG(err != OK, false, "Error loading classes from: " + index_path + ": " + itos(err)); + ERR_FAIL_COND_V_MSG(err != OK, EXIT_FAILURE, "Error loading classes from: " + index_path + ": " + itos(err)); checked_paths.insert(index_path); print_line("Merging docs..."); @@ -3336,20 +3341,19 @@ bool Main::start() { for (const String &E : checked_paths) { print_line("Erasing old docs at: " + E); err = DocTools::erase_classes(E); - ERR_FAIL_COND_V_MSG(err != OK, false, "Error erasing old docs at: " + E + ": " + itos(err)); + ERR_FAIL_COND_V_MSG(err != OK, EXIT_FAILURE, "Error erasing old docs at: " + E + ": " + itos(err)); } print_line("Generating new docs..."); err = doc.save_classes(index_path, doc_data_classes); - ERR_FAIL_COND_V_MSG(err != OK, false, "Error saving new docs:" + itos(err)); + ERR_FAIL_COND_V_MSG(err != OK, EXIT_FAILURE, "Error saving new docs:" + itos(err)); print_line("Deleting docs cache..."); if (FileAccess::exists(EditorHelp::get_cache_full_path())) { DirAccess::remove_file_or_error(EditorHelp::get_cache_full_path()); } - OS::get_singleton()->set_exit_code(EXIT_SUCCESS); - return false; + return EXIT_SUCCESS; } // GDExtension API and interface. @@ -3364,32 +3368,24 @@ bool Main::start() { } if (dump_gdextension_interface || dump_extension_api) { - OS::get_singleton()->set_exit_code(EXIT_SUCCESS); - return false; + return EXIT_SUCCESS; } if (validate_extension_api) { Engine::get_singleton()->set_editor_hint(true); // "extension_api.json" should always contains editor singletons. bool valid = GDExtensionAPIDump::validate_extension_json_file(validate_extension_api_file) == OK; - OS::get_singleton()->set_exit_code(valid ? EXIT_SUCCESS : EXIT_FAILURE); - return false; + return valid ? EXIT_SUCCESS : EXIT_FAILURE; } } #ifndef DISABLE_DEPRECATED if (converting_project) { int ret = ProjectConverter3To4(converter_max_kb_file, converter_max_line_length).convert(); - if (ret) { - OS::get_singleton()->set_exit_code(EXIT_SUCCESS); - } - return false; + return ret ? EXIT_SUCCESS : EXIT_FAILURE; } if (validating_converting_project) { bool ret = ProjectConverter3To4(converter_max_kb_file, converter_max_line_length).validate_conversion(); - if (ret) { - OS::get_singleton()->set_exit_code(EXIT_SUCCESS); - } - return false; + return ret ? EXIT_SUCCESS : EXIT_FAILURE; } #endif // DISABLE_DEPRECATED @@ -3406,7 +3402,7 @@ bool Main::start() { // this might end up triggered by valid usage, in which case we'll have to // fine-tune further. OS::get_singleton()->alert("Couldn't detect whether to run the editor, the project manager or a specific project. Aborting."); - ERR_FAIL_V_MSG(false, "Couldn't detect whether to run the editor, the project manager or a specific project. Aborting."); + ERR_FAIL_V_MSG(EXIT_FAILURE, "Couldn't detect whether to run the editor, the project manager or a specific project. Aborting."); } #endif @@ -3420,15 +3416,10 @@ bool Main::start() { if (!script.is_empty()) { Ref