Merge branch 'xml_converter' into command_line_parser

.github/workflows/diff_protobin.yml

@@ -1,10 +1,16 @@
 name: Diff Protobin
 
 on:
+  # This event occurs when there is activity on a pull request
   pull_request_target:
+    # This filter looks for files that have been changed with the suffix bin or guildpoint
     paths:
       - '**/*.bin'
       - '**/*.guildpoint'
+    # To reduce noise, this filter restricts to the default activity plus when a draft is changed to open
+    # Default activity types as listed in the documentation are "opened, synchronize, and reopened"
+    # https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target
+    types: [opened, ready_for_review, reopened, synchronize]
 jobs:
   custom-diff:
     runs-on: ubuntu-latest
@@ -40,7 +46,7 @@ jobs:
             touch $file.textproto._new
           fi
 
-          diff -u $file.textproto._old $file.textproto._new > $file._diff || true
+          diff -u $file.textproto._old --label old $file.textproto._new --label new > $file._diff || true
           echo $file >> file_list.txt
         done
 
@@ -62,12 +68,29 @@ jobs:
           const issue_number = context.issue.number;
           const githubToken = process.env.GITHUB_TOKEN;
 
+          // Get the existing comments.
+          const {data: comments} = await github.rest.pulls.listReviewComments({
+            owner: context.repo.owner,
+            repo: context.repo.repo,
+            pull_number: issue_number,
+          });
+
           const files = fs.readFileSync('file_list.txt', 'utf8').split("\n");
 
           for (let file of files) {
             if (file == "") {
               continue;
             }
+
+            // Filter comments related to the current file
+            const file_comments = comments.filter(comment => comment.path === file);
+
+            // Sort comments by creation date in descending order
+            file_comments.sort((a, b) => new Date(b.created_at) - new Date(a.created_at));
+
+            // Get the most recent comment
+            const most_recent_comment = file_comments.length > 0 ? file_comments[0] : null;
+
             const diff_contents = fs.readFileSync(file + "._diff", 'utf8')
             let comment_body = [
               "<details>",
@@ -79,6 +102,12 @@ jobs:
               "</details>",
             ].join("\n");
             
+            // Check if a similar comment already exists
+            if (most_recent_comment && most_recent_comment.body === comment_body) {
+              console.log(`Skipping file ${file}, identical comment already exists.`);
+              continue;
+            }
+
             console.log(file)
 
             await github.rest.pulls.createReviewComment({

.github/workflows/main.yml

@@ -78,8 +78,13 @@ jobs:
           cp build/compile_commands.json ./compile_commands.json
           ./presubmit.sh
 
+      - name: Run Integration Tests
+        run: |
+          cd xml_converter/integration_tests
+          ./run_tests.py --no-build
+
       - name: Upload created file
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: xml_converter
           path: xml_converter/build/xml_converter
@@ -102,14 +107,14 @@ jobs:
           make
 
       - name: Upload Standalone Executable Burrito Link
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: burrito_link_exe
           path: burrito_link/build/burrito_link.exe
           if-no-files-found: error
 
       - name: Upload Shared Library Burrito Link
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: burrito_link_dll
           path: burrito_link/build/d3d11.dll
@@ -127,7 +132,7 @@ jobs:
             cargo build --release
 
       - name: Upload created file
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: BurritoFG
           path: burrito-fg/target/release/libburrito_fg.so
@@ -145,7 +150,7 @@ jobs:
             cargo build --release
 
       - name: Upload created file
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: TacoParser
           path: taco_parser/target/release/libgw2_taco_parser.so
@@ -205,7 +210,7 @@ jobs:
           ./Godot_v${GODOT_VERSION}-stable_linux_headless.64 --export "Linux/X11"
           chmod +x build/burrito.x86_64
 
-      - uses: actions/upload-artifact@v3
+      - uses: actions/upload-artifact@v4
         with:
           name: Burrito_UI
           path: build/burrito.x86_64
@@ -221,32 +226,32 @@ jobs:
       - Build-TacoParser-Linux
     steps:
       - name: Download Burrito UI
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: Burrito_UI
 
       - name: Download Burrito FG
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: BurritoFG
 
       - name: Download TacoParser
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: TacoParser
 
       - name: Download XML Converter
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: xml_converter
 
       - name: Download Burrito Link Exe
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: burrito_link_exe
 
       - name: Download Burrito Link DLL
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: burrito_link_dll
 
@@ -257,7 +262,7 @@ jobs:
           mv burrito_link.exe burrito_link/
           mv d3d11.dll burrito_link/
 
-      - uses: actions/upload-artifact@v3
+      - uses: actions/upload-artifact@v4
         with:
           # Artifact name
           name: "Burrito_Linux" # optional, default is artifact

xml_converter/CMakeLists.txt

@@ -27,6 +27,12 @@ add_library(${CORE_LIB} STATIC ${SOURCES} ${PROTO_SRC})
 # Include protobuf libraies.
 target_link_libraries(${CORE_LIB} PUBLIC ${Protobuf_LIBRARIES})
 
+# Include LibZip libraries.
+find_package(PkgConfig REQUIRED)
+pkg_check_modules(LIBZIP REQUIRED libzip)
+target_include_directories(${CORE_LIB} PRIVATE ${LIBZIP_INCLUDE_DIRS})
+target_link_libraries(${CORE_LIB} PRIVATE ${LIBZIP_LIBRARIES})
+
 if(MSVC)
   target_compile_options(${CORE_LIB} PUBLIC /W4 /WX)
 else()

xml_converter/integration_tests/run_tests.py

@@ -40,9 +40,13 @@ def run_xml_converter(
         cmd += ["--split-by-map-id"]
 
     # Run the C++ program and capture its output
-    result = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)
+    result = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
-    return (result.stdout, result.stderr, result.returncode)
+    return (
+        result.stdout.decode(encoding="utf-8", errors="backslashreplace"),
+        result.stderr.decode(encoding="utf-8", errors="backslashreplace"),
+        result.returncode,
+    )
 
 
 def compare_text_files(file_path1: str, file_path2: str) -> List[str]:
@@ -144,6 +148,7 @@ def main() -> bool:
     parser = argparse.ArgumentParser(description="A test harness for evaluating the output of the xmlconverter program.")
     parser.add_argument("-v", "--verbose", help="Prints the results from xmlconverter in JSON format.", action="store_true")
     parser.add_argument("--filter", help="Filter which tests to run by a regex pattern.", type=str)
+    parser.add_argument("--no-build", help="Do not automatically build xml_converter before running the integration tests.", action="store_true")
     args = parser.parse_args()
 
     output_parent_dirpath = "./outputs"
@@ -154,7 +159,8 @@ def main() -> bool:
 
     all_tests_passed = True
 
-    rebuild_xml_converter_binary()
+    if not args.no_build:
+        rebuild_xml_converter_binary()
 
     test_run_count = 0
 

xml_converter/integration_tests/src/proto_utils.py

@@ -67,4 +67,4 @@ def get_guildpoint_textproto(protobin_path: str) -> str:
 
         # TODO: sanity check return code, stdout, and stderr
 
-        return result.stdout.decode("utf-8")
+        return result.stdout.decode("utf-8", errors="backslashreplace")

xml_converter/integration_tests/test_cases/proto_and_xml_input_no_duplicates/testcase.yaml

@@ -10,7 +10,7 @@ expected_stdout: |
     The following top level categories were found in more than one pack:
         "mycategory" in files:
             pack/xml_file.xml
-            pack2/markers.bin
+            pack2/markers.guildpoint
             pack3/xml_file.xml
 expected_stderr: |
 expected_returncode: 0

xml_converter/src/file_helper.cpp

@@ -1,6 +1,7 @@
 #include "file_helper.hpp"
 
 #include <dirent.h>
+#include <zip.h>
 
 #include <algorithm>
 #include <filesystem>
@@ -106,3 +107,78 @@ vector<MarkerPackFile> get_files_by_suffix(
 vector<MarkerPackFile> get_files_by_suffix(const string& base, const string& suffix) {
     return get_files_by_suffix(base, suffix, "");
 }
+
+////////////////////////////////////////////////////////////////////////////////
+// _open_directory_file_for_read
+//
+// Helper function for `open_file_for_read()` to open a file that is inside of
+// a directory.
+////////////////////////////////////////////////////////////////////////////////
+unique_ptr<basic_istream<char>> _open_directory_file_for_read(
+    const string& base,
+    const string& filename) {
+    unique_ptr<ifstream> input_filestream = make_unique<ifstream>();
+    input_filestream->open(join_file_paths(base, filename), ios::in | ios::binary);
+
+    unique_ptr<basic_istream<char>> basic_istream_stream(move(input_filestream));
+
+    return basic_istream_stream;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// _open_zip_file_for_read
+//
+// Helper function for `open_file_for_read()` to open a file that is inside of
+// a zipfile.
+////////////////////////////////////////////////////////////////////////////////
+unique_ptr<basic_istream<char>> _open_zip_file_for_read(
+    const string& zipfile,
+    const string& filename) {
+    int err = 0;
+    struct zip* zip_archive = zip_open(zipfile.c_str(), 0, &err);
+
+    // Search for the filenname
+    const char* name = filename.c_str();
+    struct zip_stat st;
+    zip_stat_init(&st);
+    zip_stat(zip_archive, name, 0, &st);
+
+    char* contents = new char[st.size];
+    zip_file_t* f = zip_fopen(zip_archive, name, 0);
+    zip_fread(f, contents, st.size);
+    zip_fclose(f);
+    zip_close(zip_archive);
+
+    // Copy the file string into the stringstring and move it into a basic_istream
+    unique_ptr<istringstream> string_stream = make_unique<istringstream>(contents);
+    unique_ptr<basic_istream<char>> basic_istream_stream(move(string_stream));
+    delete[] contents;
+
+    return basic_istream_stream;
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// open_file_for_read
+//
+// Opens a file to read from that is either inside of a directory or inside of
+// a zip file. If `base` is a file then it will be assumed to be a zip file and
+// `filename` will be opened inside of it. If `base` is a directory then
+// `filename` inside of that directory will be opened instead.
+////////////////////////////////////////////////////////////////////////////////
+unique_ptr<basic_istream<char>> open_file_for_read(const MarkerPackFile& file) {
+    if (!filesystem::exists(file.base)) {
+        cout << "Error: " << file.base << " does not exist" << endl;
+        return nullptr;
+    }
+    // If it is a directory, call open_directory_file_to_read
+    else if (filesystem::is_directory(file.base)) {
+        return _open_directory_file_for_read(file.base, file.relative_filepath);
+    }
+    // If it is a file call open_zip_file_to_read
+    else if (filesystem::is_regular_file(file.base)) {
+        return _open_zip_file_for_read(file.base, file.relative_filepath);
+    }
+
+    cout << "Error: " << file.base << " is not a directory or a file" << endl;
+    return nullptr;
+}

xml_converter/src/file_helper.hpp

@@ -1,4 +1,5 @@
 #pragma once
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -15,3 +16,5 @@ class MarkerPackFile {
 void copy_file(std::string path, std::string new_path);
 
 std::vector<MarkerPackFile> get_files_by_suffix(const std::string& base, const std::string& suffix);
+
+std::unique_ptr<std::basic_istream<char>> open_file_for_read(const MarkerPackFile& file);

xml_converter/src/packaging_protobin.cpp

@@ -3,6 +3,7 @@
 #include <filesystem>
 #include <fstream>
 #include <map>
+#include <memory>
 #include <set>
 #include <string>
 #include <vector>
@@ -61,20 +62,24 @@ string parse_guildpoint_categories(
 }
 
 ////////////////////////////////////////////////////////////////////////////////
+// read_protobuf_file
 //
+// Reads a protobuf file into memory.
 ////////////////////////////////////////////////////////////////////////////////
-set<string> read_protobuf_file(string proto_filepath, const string marker_pack_root_directory, map<string, Category>* marker_categories, vector<Parseable*>* parsed_pois) {
-    fstream infile;
-    guildpoint::Guildpoint proto_message;
-    ProtoReaderState state;
-    state.marker_pack_root_directory = marker_pack_root_directory;
-    set<string> category_names;
+set<string> read_protobuf_file(
+    const MarkerPackFile& proto_filepath,
+    map<string, Category>* marker_categories,
+    vector<Parseable*>* parsed_pois) {
+    unique_ptr<basic_istream<char>> infile = open_file_for_read(proto_filepath);
 
-    infile.open(proto_filepath, ios::in | ios::binary);
-    proto_message.ParseFromIstream(&infile);
+    guildpoint::Guildpoint proto_message;
+    proto_message.ParseFromIstream(&*infile);
 
+    ProtoReaderState state;
+    state.marker_pack_root_directory = proto_filepath.base;
     state.textures = proto_message.textures();
 
+    set<string> category_names;
     for (int i = 0; i < proto_message.category_size(); i++) {
         category_names.insert(parse_guildpoint_categories("", proto_message.category(i), marker_categories, parsed_pois, &state));
     }

xml_converter/src/packaging_protobin.hpp

@@ -7,13 +7,13 @@
 #include <vector>
 
 #include "category_gen.hpp"
+#include "file_helper.hpp"
 #include "guildpoint.pb.h"
 #include "parseable.hpp"
 #include "string_hierarchy.hpp"
 
 std::set<std::string> read_protobuf_file(
-    std::string proto_filepath,
-    const std::string marker_pack_root_directory,
+    const MarkerPackFile& proto_filepath,
     std::map<std::string, Category>* marker_categories,
     std::vector<Parseable*>* parsed_pois);