Unify behavior of source code search (#332)

* Add tests for walk_source_dir

* Don't test path order: it will differ between Windows and Posix

* Restore 3.9 compatible code. Add sort option. Update tests

* Create source_code_search function

* Use os functions only when necessary. Update comments.

Author: disinvite

reccmp/compare/core.py

@@ -5,7 +5,7 @@
 from typing_extensions import Self
 from reccmp.project.detect import RecCmpTarget
 from reccmp.difflib import get_grouped_opcodes
-from reccmp.dir import walk_source_dir
+from reccmp.dir import source_code_search
 from reccmp.compare.functions import FunctionComparator
 from reccmp.formats import (
     Image,
@@ -195,7 +195,7 @@ def from_target(cls, target: RecCmpTarget) -> Self:
         )
         pdb_file = CvdumpAnalysis(cvdump)
 
-        code_paths = walk_source_dir(target.source_root)
+        code_paths = source_code_search(target.source_root)
         code_files = list(TextFile.from_files(code_paths, allow_error=True))
 
         data_sources = list(TextFile.from_files(target.data_sources, allow_error=True))

reccmp/dir.py

@@ -2,7 +2,7 @@
 
 import os
 import subprocess
-from typing import Iterator
+from typing import Iterable, Iterator
 from pathlib import Path, PurePath, PureWindowsPath
 
 
@@ -134,8 +134,9 @@ def resolve_cvdump(self, path_str: str) -> str:
         return self._memo[path_str]
 
 
-def is_file_c_like(filename: Path | str) -> bool:
-    return Path(filename).suffix.lower() in (
+def is_file_c_like(path: Path) -> bool:
+    """Tests the given path for typical C/C++ file extensions."""
+    return path.suffix.lower() in (
         ".c",
         ".h",
         ".cc",
@@ -144,18 +145,52 @@ def is_file_c_like(filename: Path | str) -> bool:
         ".hxx",
         ".cpp",
         ".hpp",
-        ".C",
     )
 
 
-def walk_source_dir(source: Path, recursive: bool = True) -> Iterator[Path]:
-    """Generator to walk the given directory recursively and return
-    any C++ files found."""
+def platform_independent_path_sort(paths: Iterable[Path]) -> Iterator[Path]:
+    """PosixPaths are case-sensitive. WindowsPaths are not.
+    To ensure that reccmp processing is consistent across platforms,
+    use this method to sort paths in a case-insensitive order.
+    An example of the problem we want to fix: metadata for the same entity address
+    in multiple files may cause us to overwrite data. If we do, it should be done
+    in a consistent way because we have processed the metadata files in the same order.
+    """
+    yield from sorted(paths, key=lambda p: str(p).lower())
+
+
+def walk_source_dir(source: Path, *, recursive: bool = True) -> Iterator[Path]:
+    """Returns any C/C++ source code files found in the given directory tree."""
 
-    for subdir, _, files in source.walk():
+    # Python 3.12 introduced Path.walk(). We use os.walk() instead for broader compatibility.
+    for subdir, _, files in os.walk(source.absolute()):
         for file in files:
-            if is_file_c_like(file):
-                yield subdir / file
+            path = Path(os.path.join(subdir, file))
+            if is_file_c_like(path):
+                yield path
 
         if not recursive:
             break
+
+
+def source_code_search(search_paths: Path | Iterable[Path]) -> Iterator[Path]:
+    """Use the provided search paths to find source code files in the project
+    that we will scan for reccmp metadata. Returns a list of distinct paths
+    sorted by their lower-case string representation."""
+    code_files = set()
+
+    # Allow single Path argument
+    if not isinstance(search_paths, Iterable):
+        search_paths = (search_paths,)
+
+    for path in search_paths:
+        if not path.exists():
+            continue
+
+        if path.is_file() and is_file_c_like(path):
+            code_files.add(path)
+            continue
+
+        code_files.update(walk_source_dir(path))
+
+    yield from platform_independent_path_sort(code_files)

reccmp/tools/decomplint.py

@@ -6,7 +6,7 @@
 from typing import Iterable
 import colorama
 import reccmp
-from reccmp.dir import walk_source_dir
+from reccmp.dir import source_code_search
 from reccmp.parser import DecompLinter
 from reccmp.parser.error import ParserAlert
 from reccmp.project.logging import argparse_add_logging_args, argparse_parse_logging
@@ -108,7 +108,7 @@ def process_files(
 
 def main():
     args = parse_args()
-    files_to_check: list[Path] = []
+    search_paths: list[Path] = []
 
     if not args.paths:
         # No path specified. Try to find the project file.
@@ -121,18 +121,16 @@ def main():
         # then get all filenames from each code directory.
         for target in project.targets.values():
             if target.source_root:
-                files_to_check.extend(walk_source_dir(target.source_root))
+                search_paths.append(target.source_root)
     else:
         for path in args.paths:
-            if path.is_dir():
-                files_to_check.extend(walk_source_dir(path))
-            elif path.is_file():
-                files_to_check.append(path)
+            if path.is_dir() or path.is_file():
+                search_paths.append(path)
             else:
                 logger.error("Invalid path: %s", path)
 
     # Use set() so we check each file only once.
-    reduced_file_list = sorted(set(files_to_check))
+    reduced_file_list = source_code_search(search_paths)
 
     codefiles = list(TextFile.from_files(reduced_file_list, allow_error=True))
 

tests/test_dir.py

@@ -0,0 +1,228 @@
+"""Tests for walking source code directories in search of relevant files"""
+
+from pathlib import Path
+from reccmp.dir import source_code_search, walk_source_dir
+
+
+def create_blank_file(p: Path):
+    """Helper to make this action more idiomatic in the tests."""
+    p.write_text("")
+
+
+def test_walk_empty_dir(tmp_path_factory):
+    """Empty directory returns no files."""
+    path = tmp_path_factory.mktemp("empty")
+
+    files = list(walk_source_dir(path))
+    assert not files
+
+
+def test_walk_dir_with_irrelevant_files(tmp_path_factory):
+    """Skip files that do not match our default filter on file extension."""
+    path = tmp_path_factory.mktemp("no_match")
+    create_blank_file(path / "test.txt")
+    create_blank_file(path / "sample.jpg")
+
+    files = list(walk_source_dir(path))
+    assert not files
+
+
+def test_walk_dir_with_mixed_files(tmp_path_factory):
+    """Return code files and skip non-matching files."""
+    path = tmp_path_factory.mktemp("mixed")
+    create_blank_file(path / "test.txt")
+    create_blank_file(path / "sample.jpg")
+    create_blank_file(path / "game.cpp")
+    create_blank_file(path / "game.hpp")
+
+    files = list(walk_source_dir(path))
+    assert len(files) == 2
+    assert {f.name for f in files} == {"game.cpp", "game.hpp"}
+
+
+def test_walk_non_directory(tmp_path_factory):
+    """Return nothing if the input path is not a directory."""
+    path = tmp_path_factory.mktemp("mixed")
+    create_blank_file(path / "test.txt")
+    create_blank_file(path / "sample.jpg")
+    create_blank_file(path / "game.cpp")
+    create_blank_file(path / "game.hpp")
+
+    files = list(walk_source_dir(path / "game.cpp"))
+    assert not files
+
+
+def test_walk_recursion(tmp_path_factory):
+    """Should scan every subdirectory for matching files."""
+    path = tmp_path_factory.mktemp("recurse")
+    create_blank_file(path / "game.cpp")
+    (path / "x" / "y" / "z").mkdir(parents=True)
+    create_blank_file(path / "x" / "hello.cpp")
+    create_blank_file(path / "x" / "y" / "z" / "test.cpp")
+
+    files = list(walk_source_dir(path))
+    assert {f.name for f in files} == {"game.cpp", "hello.cpp", "test.cpp"}
+
+
+def test_walk_recursion_disabled(tmp_path_factory):
+    """Should not enter subdirectories if recursion is disabled."""
+    path = tmp_path_factory.mktemp("recurse")
+    create_blank_file(path / "game.cpp")
+    (path / "x" / "y" / "z").mkdir(parents=True)
+    create_blank_file(path / "x" / "hello.cpp")
+    create_blank_file(path / "x" / "y" / "z" / "test.cpp")
+
+    files = list(walk_source_dir(path, recursive=False))
+    assert {f.name for f in files} == {"game.cpp"}
+
+
+def test_walk_absolute_paths(tmp_path_factory):
+    """Returned paths should be absolute and include all subdirectories."""
+    path = tmp_path_factory.mktemp("recurse")
+    create_blank_file(path / "game.cpp")
+    (path / "x" / "y" / "z").mkdir(parents=True)
+    create_blank_file(path / "x" / "hello.cpp")
+    create_blank_file(path / "x" / "y" / "z" / "test.cpp")
+
+    files = list(walk_source_dir(path))
+    for f in files:
+        assert f.is_absolute()
+
+        # For files in subdirectories, make sure each one is represented.
+        # Check only the components relative to the base path.
+        # We don't control where pytest creates the tmp directory.
+        if f.name == "hello.cpp":
+            assert "x" in f.relative_to(path).parts
+
+        if f.name == "test.cpp":
+            assert "x" in f.relative_to(path).parts
+            assert "y" in f.relative_to(path).parts
+            assert "z" in f.relative_to(path).parts
+
+
+def test_walk_mixed_case_extension(tmp_path_factory):
+    """Should use case-insensitive match for file extensions."""
+    path = tmp_path_factory.mktemp("mixed_case")
+    create_blank_file(path / "game.CPP")
+    create_blank_file(path / "HELLO.C")
+    create_blank_file(path / "HELLO.h")
+    create_blank_file(path / "test.hpP")
+
+    files = list(walk_source_dir(path))
+    assert {f.name.lower() for f in files} == {
+        "game.cpp",
+        "hello.c",
+        "hello.h",
+        "test.hpp",
+    }
+
+
+def test_search_mixed_files(tmp_path_factory):
+    """Search behaves the same as walk_source_dir: return only matching code files."""
+    path = tmp_path_factory.mktemp("mixed")
+    create_blank_file(path / "test.txt")
+    create_blank_file(path / "sample.jpg")
+    create_blank_file(path / "game.cpp")
+    create_blank_file(path / "game.hpp")
+
+    files = list(source_code_search(path))
+    assert {f.name for f in files} == {"game.cpp", "game.hpp"}
+
+
+def test_search_ignore_missing_dir(tmp_path_factory):
+    """Search should return nothing if the search path does not exist."""
+    path = tmp_path_factory.mktemp("mixed")
+    create_blank_file(path / "test.txt")
+    create_blank_file(path / "sample.jpg")
+    create_blank_file(path / "game.cpp")
+    create_blank_file(path / "game.hpp")
+
+    files = list(source_code_search(path / "x"))
+    assert not files
+
+
+def test_search_input_order_ignored(tmp_path_factory):
+    """The returned paths are always sorted by their lower-case string version,
+    regardless of input order."""
+    path = tmp_path_factory.mktemp("order")
+    create_blank_file(path / "game.cpp")
+    create_blank_file(path / "Test.cpp")
+
+    search_paths = (path / "Test.cpp", path / "game.cpp")
+    files = list(source_code_search(search_paths))
+    assert [f.name for f in files] == ["game.cpp", "Test.cpp"]
+
+
+def test_search_ignore_irrelevant_file_even_if_targeted(tmp_path_factory):
+    """The current behavior is to test each file's extension.
+    This is true even if the input search path points directly at a file."""
+    path = tmp_path_factory.mktemp("mixed")
+    create_blank_file(path / "test.txt")
+    create_blank_file(path / "sample.jpg")
+    create_blank_file(path / "game.cpp")
+    create_blank_file(path / "game.hpp")
+
+    search_paths = path / "test.text"
+    files = list(source_code_search(search_paths))
+    assert not files
+
+
+def test_search_nested_dirs(tmp_path_factory):
+    """Should return distinct files if any search paths overlap."""
+    path = tmp_path_factory.mktemp("recurse")
+    create_blank_file(path / "game.cpp")
+    (path / "x" / "y" / "z").mkdir(parents=True)
+    create_blank_file(path / "x" / "hello.cpp")
+    create_blank_file(path / "x" / "y" / "z" / "test.cpp")
+
+    # Nested paths
+    search_paths = (path, path / "x")
+    files = list(source_code_search(search_paths))
+    assert len(files) == 3
+
+
+def test_search_using_file(tmp_path_factory):
+    """Using a file as our search path, the search function
+    should return the path if the file exists and matches our criteria."""
+    path = tmp_path_factory.mktemp("mixed")
+    create_blank_file(path / "test.txt")
+    create_blank_file(path / "sample.jpg")
+    create_blank_file(path / "game.cpp")
+    create_blank_file(path / "game.hpp")
+
+    files = list(source_code_search(path / "game.cpp"))
+    assert [f.name for f in files] == ["game.cpp"]
+
+
+def test_search_using_file_not_exist(tmp_path_factory):
+    """Search should return nothing if the search path does not exist.
+    Using a file in this example."""
+    path = tmp_path_factory.mktemp("mixed")
+    create_blank_file(path / "test.txt")
+    create_blank_file(path / "sample.jpg")
+    create_blank_file(path / "game.cpp")
+    create_blank_file(path / "game.hpp")
+
+    files = list(source_code_search(path / "hello.cpp"))
+    assert not files
+
+
+def test_search_case_insensitive_order(tmp_path_factory):
+    """Make sure path order is consistent across Posix/Windows runners.
+    The difference is that paths are case-sensitive on Posix but not Windows.
+    The expectation is that reccmp returns the same results regardless of platform.
+    Metadata read from source code files could be overwritten if it is duplicated across files.
+    """
+    path = tmp_path_factory.mktemp("mixed_case")
+    create_blank_file(path / "game.CPP")
+    create_blank_file(path / "HELLO.C")
+    create_blank_file(path / "HELLO.h")
+    create_blank_file(path / "test.hpP")
+
+    files = list(source_code_search(path))
+    assert [f.name.lower() for f in files] == [
+        "game.cpp",
+        "hello.c",
+        "hello.h",
+        "test.hpp",
+    ]