From 493f6d4e83ffa2ca62133f96dc3a9b8851a5bc4a Mon Sep 17 00:00:00 2001 From: Qrox Date: Sun, 27 Dec 2020 19:18:11 +0800 Subject: [PATCH 1/3] Display multi-line error messages with github action --- build-scripts/build.sh | 2 +- build-scripts/problem-matchers/json.json | 16 --- build-scripts/requirements.sh | 1 - data/json/achievements.json | 6 +- src/cached_options.cpp | 1 + src/cached_options.h | 12 +++ src/json.cpp | 129 +++++++++++++++++++++-- tests/json_test.cpp | 10 ++ tests/test_main.cpp | 13 +++ 9 files changed, 163 insertions(+), 27 deletions(-) delete mode 100644 build-scripts/problem-matchers/json.json diff --git a/build-scripts/build.sh b/build-scripts/build.sh index cdef2f63c599d..3c56ff8f98895 100755 --- a/build-scripts/build.sh +++ b/build-scripts/build.sh @@ -9,7 +9,7 @@ num_jobs=3 function run_tests { # --min-duration shows timing lines for tests with a duration of at least that many seconds. - $WINE "$@" --min-duration 0.2 --use-colour yes --rng-seed time $EXTRA_TEST_OPTS + $WINE "$@" --min-duration 0.2 --use-colour yes --rng-seed time --error-format=github-action $EXTRA_TEST_OPTS } # We might need binaries installed via pip, so ensure that our personal bin dir is on the PATH diff --git a/build-scripts/problem-matchers/json.json b/build-scripts/problem-matchers/json.json deleted file mode 100644 index eafe4261f6bd4..0000000000000 --- a/build-scripts/problem-matchers/json.json +++ /dev/null @@ -1,16 +0,0 @@ -{ - "problemMatcher": [ - { - "owner": "cata-json", - "pattern": [ - { - "regexp": "^Json error: ([^:]*):([0-9]+):([0-9]+): (.*)$", - "file": 1, - "line": 2, - "column": 3, - "message": 4 - } - ] - } - ] -} diff --git a/build-scripts/requirements.sh b/build-scripts/requirements.sh index 3cdf4ec136952..23ec88a309a55 100644 --- a/build-scripts/requirements.sh +++ b/build-scripts/requirements.sh @@ -20,7 +20,6 @@ function just_json # (See https://github.com/actions/toolkit/blob/master/docs/problem-matchers.md) echo "::add-matcher::build-scripts/problem-matchers/catch2.json" echo "::add-matcher::build-scripts/problem-matchers/debugmsg.json" -echo "::add-matcher::build-scripts/problem-matchers/json.json" if which travis_retry &>/dev/null then diff --git a/data/json/achievements.json b/data/json/achievements.json index 9a01c6e47d9bd..0da67a3bd8c2f 100644 --- a/data/json/achievements.json +++ b/data/json/achievements.json @@ -3,7 +3,11 @@ "id": "achievement_kill_zombie", "type": "achievement", "name": "One down, billions to go\u2026", - "requirements": [ { "event_statistic": "num_avatar_zombie_kills", "is": ">=", "target": 1 } ] + "requirements": [ { "event_statistic": "num_avatar_zombie_kills", "is": ">=", "target": 1 } ], + "foo": 0, + "bar": 1, + "baz": 2, + "qux": 3 }, { "id": "achievement_kill_cyborg", diff --git a/src/cached_options.cpp b/src/cached_options.cpp index dcfd581dc9741..3f87c82385472 100644 --- a/src/cached_options.cpp +++ b/src/cached_options.cpp @@ -12,3 +12,4 @@ bool use_tiles; test_mode_spilling_action_t test_mode_spilling_action = test_mode_spilling_action_t::spill_all; bool direct3d_mode; bool pixel_minimap_option; +error_log_format_t error_log_format = error_log_format_t::human_readable; diff --git a/src/cached_options.h b/src/cached_options.h index 7333b2c82c969..e6ac4695eae5e 100644 --- a/src/cached_options.h +++ b/src/cached_options.h @@ -27,4 +27,16 @@ extern test_mode_spilling_action_t test_mode_spilling_action; extern bool direct3d_mode; +enum class error_log_format_t { + human_readable, + // Output error messages in github action command format (currently json only) + // See https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-commands-for-github-actions#setting-an-error-message + github_action, +}; +#ifndef CATA_IN_TOOL +extern error_log_format_t error_log_format; +#else +constexpr error_log_format_t error_log_format = error_log_format_t::human_readable; +#endif + #endif // CATA_SRC_CACHED_OPTIONS_H diff --git a/src/json.cpp b/src/json.cpp index 0edfead88e5d2..3a79ec65b788e 100644 --- a/src/json.cpp +++ b/src/json.cpp @@ -20,6 +20,7 @@ #include "cached_options.h" #include "cata_utility.h" #include "debug.h" +#include "output.h" #include "string_formatter.h" // JSON parsing and serialization tools for Cataclysm-DDA. @@ -1650,16 +1651,111 @@ bool JsonIn::read( JsonDeserializer &j, bool throw_on_error ) } } +/** + * Get the normal form of a relative path. Does not work on absolute paths. + * Slash and backslash are both treated as path separators and replaced with + * slash. Trailing slashes are always removed. + */ +static std::string normalize_relative_path( const std::string &path ) +{ + if( path.empty() ) { + // normal form of an empty path is an empty path + return path; + } + std::vector names; + for( size_t name_start = 0; name_start < path.size(); ) { + const size_t name_end = std::min( path.find_first_of( "\\/", name_start ), + path.size() ); + if( name_start < name_end ) { + const std::string name = path.substr( name_start, name_end - name_start ); + if( name == "." ) { + // do nothing + } else if( name == ".." ) { + if( names.empty() || names.back() == ".." ) { + names.emplace_back( name ); + } else { + names.pop_back(); + } + } else { + names.emplace_back( name ); + } + } + name_start = std::min( path.find_first_not_of( "\\/", name_end ), + path.size() ); + } + if( names.empty() ) { + return "."; + } else { + std::string normpath; + for( auto it = names.begin(); it != names.end(); ++it ) { + if( it != names.begin() ) { + normpath += "/"; + } + normpath += *it; + } + return normpath; + } +} + +/** + * Escape special chars in github action command properties. + * See https://github.com/actions/toolkit/blob/main/packages/core/src/command.ts + */ +static std::string escape_property( std::string str ) +{ + switch( error_log_format ) { + case error_log_format_t::human_readable: + break; + case error_log_format_t::github_action: + replace_substring( str, "%", "%25", true ); + replace_substring( str, "\r", "%0D", true ); + replace_substring( str, "\n", "%0A", true ); + replace_substring( str, ":", "%3A", true ); + replace_substring( str, ",", "%2C", true ); + break; + } + return str; +} + +/** + * Escape special chars in github action command messages. + * See https://github.com/actions/toolkit/blob/main/packages/core/src/command.ts + */ +static std::string escape_data( std::string str ) +{ + switch( error_log_format ) { + case error_log_format_t::human_readable: + break; + case error_log_format_t::github_action: + replace_substring( str, "%", "%25", true ); + replace_substring( str, "\r", "%0D", true ); + replace_substring( str, "\n", "%0A", true ); + break; + } + return str; +} + /* error display */ // WARNING: for occasional use only. std::string JsonIn::line_number( int offset_modifier ) { - const std::string &name = path ? *path : ""; + const std::string name = escape_property( path ? normalize_relative_path( *path ) + : "" ); if( stream && stream->eof() ) { - return name + ":EOF"; + switch( error_log_format ) { + case error_log_format_t::human_readable: + return name + ":EOF"; + case error_log_format_t::github_action: + return "file=" + name + ",line=EOF"; + } } else if( !stream || stream->fail() ) { - return name + ":???"; + switch( error_log_format ) { + case error_log_format_t::human_readable: + return name + ":???"; + case error_log_format_t::github_action: + return "file=" + name + ",line=???"; + } } // else stream is fine int pos = tell(); int line = 1; @@ -1687,17 +1783,32 @@ std::string JsonIn::line_number( int offset_modifier ) } seek( pos ); std::stringstream ret; - ret << name << ":" << line << ":" << offset; + switch( error_log_format ) { + case error_log_format_t::human_readable: + ret << name << ":" << line << ":" << offset; + break; + case error_log_format_t::github_action: + ret.imbue( std::locale::classic() ); + ret << "file=" << name << ",line=" << line << ",col=" << offset; + break; + } return ret.str(); } void JsonIn::error( const std::string &message, int offset ) { - std::ostringstream err; - err << "Json error: " << line_number( offset ) << ": " << message; + std::ostringstream err_header; + switch( error_log_format ) { + case error_log_format_t::human_readable: + err_header << "Json error: " << line_number( offset ) << ": "; + break; + case error_log_format_t::github_action: + err_header << "::error " << line_number( offset ) << "::"; + break; + } // if we can't get more info from the stream don't try if( !stream->good() ) { - throw JsonError( err.str() ); + throw JsonError( err_header.str() + escape_data( message ) ); } // Seek to eof after throwing to avoid continue reading from the incorrect // location. The calling code of json error methods is supposed to restore @@ -1705,6 +1816,8 @@ void JsonIn::error( const std::string &message, int offset ) on_out_of_scope seek_to_eof( [this]() { stream->seekg( 0, std::istream::end ); } ); + std::ostringstream err; + err << message; // also print surrounding few lines of context, if not too large err << "\n\n"; stream->seekg( offset, std::istream::cur ); @@ -1774,7 +1887,7 @@ void JsonIn::error( const std::string &message, int offset ) if( !msg.empty() && msg.back() != '\n' ) { msg.push_back( '\n' ); } - throw JsonError( msg ); + throw JsonError( err_header.str() + escape_data( msg ) ); } void JsonIn::string_error( const std::string &message, const int offset ) diff --git a/tests/json_test.cpp b/tests/json_test.cpp index a7eb0aab63fd7..41a7d57ee6ba7 100644 --- a/tests/json_test.cpp +++ b/tests/json_test.cpp @@ -11,6 +11,8 @@ #include #include "mutation.h" +#include "cached_options.h" +#include "cata_utility.h" #include "colony.h" #include "string_formatter.h" #include "type_id.h" @@ -255,6 +257,8 @@ TEST_CASE( "translation_text_style_check", "[json][translation]" ) { // this test case is mainly for checking the format of debug messages. // the text style check itself is tested in the lit test of clang-tidy. + restore_on_out_of_scope restore_error_log_format( error_log_format ); + error_log_format = error_log_format_t::human_readable; // string, ascii test_translation_text_style_check( @@ -400,6 +404,9 @@ TEST_CASE( "translation_text_style_check", "[json][translation]" ) TEST_CASE( "translation_text_style_check_error_recovery", "[json][translation]" ) { + restore_on_out_of_scope restore_error_log_format( error_log_format ); + error_log_format = error_log_format_t::human_readable; + SECTION( "string" ) { const std::string json = R"([)" "\n" @@ -499,6 +506,9 @@ static void test_string_error_throws_matches( Matcher &&matcher, const std::stri TEST_CASE( "jsonin_get_string", "[json]" ) { + restore_on_out_of_scope restore_error_log_format( error_log_format ); + error_log_format = error_log_format_t::human_readable; + // read plain text test_get_string( "foo", R"("foo")" ); // ignore starting spaces diff --git a/tests/test_main.cpp b/tests/test_main.cpp index eca2360bd0fd0..47992a2274ac5 100644 --- a/tests/test_main.cpp +++ b/tests/test_main.cpp @@ -285,6 +285,16 @@ int main( int argc, const char *argv[] ) std::string user_dir = extract_user_dir( arg_vec ); + std::string error_fmt = extract_argument( arg_vec, "--error-format=" ); + if( error_fmt == "github-action" ) { + error_log_format = error_log_format_t::github_action; + } else if( error_fmt == "human-readable" || error_fmt.empty() ) { + error_log_format = error_log_format_t::human_readable; + } else { + printf( "Unknown format %s", error_fmt.c_str() ); + return EXIT_FAILURE; + } + // Note: this must not be invoked before all DDA-specific flags are stripped from arg_vec! int result = session.applyCommandLine( arg_vec.size(), &arg_vec[0] ); if( result != 0 || session.configData().showHelp ) { @@ -294,6 +304,9 @@ int main( int argc, const char *argv[] ) printf( " -D, --drop-world Don't save the world on test failure.\n" ); printf( " --option_overrides=n:v[,…] Name-value pairs of game options for tests.\n" ); printf( " (overrides config/options.json values)\n" ); + printf( " --error-format= Format of error messages. Possible values are:\n" ); + printf( " human-readable (default)\n" ); + printf( " github-action\n" ); return result; } From 247a561429b717f1a42f85d2545ff28d009c7430 Mon Sep 17 00:00:00 2001 From: Qrox Date: Thu, 31 Dec 2020 16:30:40 +0800 Subject: [PATCH 2/3] Revert testing json changes --- data/json/achievements.json | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/data/json/achievements.json b/data/json/achievements.json index 0da67a3bd8c2f..9a01c6e47d9bd 100644 --- a/data/json/achievements.json +++ b/data/json/achievements.json @@ -3,11 +3,7 @@ "id": "achievement_kill_zombie", "type": "achievement", "name": "One down, billions to go\u2026", - "requirements": [ { "event_statistic": "num_avatar_zombie_kills", "is": ">=", "target": 1 } ], - "foo": 0, - "bar": 1, - "baz": 2, - "qux": 3 + "requirements": [ { "event_statistic": "num_avatar_zombie_kills", "is": ">=", "target": 1 } ] }, { "id": "achievement_kill_cyborg", From 497cc6f935829efc99e1b47c64293a758b37cab6 Mon Sep 17 00:00:00 2001 From: Qrox Date: Thu, 31 Dec 2020 21:15:30 +0800 Subject: [PATCH 3/3] Fix clang-tidy warning --- src/json.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/json.cpp b/src/json.cpp index 3a79ec65b788e..d2ae168e5e3df 100644 --- a/src/json.cpp +++ b/src/json.cpp @@ -1708,7 +1708,7 @@ static std::string escape_property( std::string str ) break; case error_log_format_t::github_action: replace_substring( str, "%", "%25", true ); - replace_substring( str, "\r", "%0D", true ); + replace_substring( str, "\r", "%0D", true ); // NOLINT(cata-text-style) replace_substring( str, "\n", "%0A", true ); replace_substring( str, ":", "%3A", true ); replace_substring( str, ",", "%2C", true ); @@ -1728,7 +1728,7 @@ static std::string escape_data( std::string str ) break; case error_log_format_t::github_action: replace_substring( str, "%", "%25", true ); - replace_substring( str, "\r", "%0D", true ); + replace_substring( str, "\r", "%0D", true ); // NOLINT(cata-text-style) replace_substring( str, "\n", "%0A", true ); break; }