[wic][PATCH 0/4] oe/path: three fixes plus unit coverage

[wic][PATCH 0/4] oe/path: three fixes plus unit coverage

[Trevor Woerner]

This series continues the standalone unit-test work, this time over
oe/path.py. It follows the same shape the suite settled on: each
source fix is its own standalone commit, and the green test module
lands last, so the suite passes at every commit.

Three fixes come first, each independent of the others:

  - __realpath's isdir guard assigns a bare `false`, which is not a
    Python name; when os.path.isdir() raises, the handler meant to
    absorb the error raises NameError instead. Use the builtin False.
  - symlink(force=True) cleared the destination through remove(), which
    globs its argument; a destination containing glob metacharacters
    could fail to match itself or match unrelated files. Remove the
    literal destination instead.
  - canonicalize('') and canonicalize(None) returned the current
    working directory, because os.path.realpath('') does; a stray
    separator injected a spurious cwd entry too. Skip empty tokens.

The final commit adds tests/unit/test_oe_path.py, covering oe/path's
own logic (join, is_path_parent, symlink, make_relative_symlink,
canonicalize, which_wild, realpath) and locking in the three fixes.
Backing any fix out turns the matching test red. The suite is green
and ruff-clean, and the series passes oe-core's wic oe-selftest with
no regressions.

Trevor Woerner (4):
  oe/path: fix bare `false` NameError in __realpath's isdir guard
  oe/path: don't glob-expand the destination in symlink(force=True)
  oe/path: canonicalize('') should return '' rather than the cwd
  tests/unit/test_oe_path: cover oe/path's own path logic

 src/wic/oe/path.py         |  21 ++-
 tests/unit/test_oe_path.py | 298 +++++++++++++++++++++++++++++++++++++
 2 files changed, 316 insertions(+), 3 deletions(-)
 create mode 100644 tests/unit/test_oe_path.py

-- 
2.50.0.173.g8b6f19ccfc3a

[wic][PATCH 1/4] oe/path: fix bare `false` NameError in __realpath's isdir guard

[Trevor Woerner]

__realpath() finishes by deciding whether the resolved path is a
directory, tolerating any error from the stat:

    try:
        is_dir = os.path.isdir(file)
    except:
        is_dir = false

but `false` is not a Python name -- the intended value is the builtin
False. os.path.isdir() swallows almost everything and simply returns
False for a path it cannot stat, so the except branch is rarely taken;
that is why the typo has stayed hidden. But when isdir() does raise --
for instance an OSError deep in os.stat() on a broken mount -- the
handler that was meant to absorb the error instead raises
NameError: name 'false' is not defined, masking the original failure
with a misleading one.

Use the builtin False so the fallback behaves as intended: a path that
cannot be stat'd is simply reported as not-a-directory.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
 src/wic/oe/path.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wic/oe/path.py b/src/wic/oe/path.py
index 3e95ea0ec923..862edc532ade 100644
--- a/src/wic/oe/path.py
+++ b/src/wic/oe/path.py
@@ -233,7 +233,7 @@ def __realpath(file, root, loop_cnt, assume_dir):
     try:
         is_dir = os.path.isdir(file)
     except:
-        is_dir = false
+        is_dir = False
 
     return (file, is_dir)
 
-- 
2.50.0.173.g8b6f19ccfc3a

[wic][PATCH 2/4] oe/path: don't glob-expand the destination in symlink(force=True)

[Trevor Woerner]

symlink(source, destination, force=True) cleared an existing
destination by calling remove(destination). remove() is a rm -rf
helper that runs its argument through glob.glob() before unlinking, so
it treats the destination as a glob pattern rather than a literal path.

For ordinary names this is merely wasteful, but a destination that
contains glob metacharacters is actively dangerous. A name with a '['
may fail to match itself and silently leave the old link in place, and
a pattern that happens to match other entries could unlink files the
caller never named. remove() even warns about exactly this in its own
docstring.

Remove the literal destination directly instead: unlink it, fall back
to shutil.rmtree() when it turns out to be a directory (EISDIR), and
treat a missing destination (ENOENT) as success. This mirrors what
remove() does per matched name, minus the glob expansion.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
 src/wic/oe/path.py | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/wic/oe/path.py b/src/wic/oe/path.py
index 862edc532ade..13c52b363694 100644
--- a/src/wic/oe/path.py
+++ b/src/wic/oe/path.py
@@ -168,7 +168,18 @@ def symlink(source, destination, force=False):
     """Create a symbolic link"""
     try:
         if force:
-            remove(destination)
+            # Remove the exact destination path. Do not route this through
+            # remove(), which treats its argument as a glob pattern: a
+            # destination containing glob metacharacters (for example a
+            # '[' in the name) could fail to match, or match and delete
+            # unrelated files.
+            try:
+                os.unlink(destination)
+            except OSError as exc:
+                if exc.errno == errno.EISDIR:
+                    shutil.rmtree(destination)
+                elif exc.errno != errno.ENOENT:
+                    raise
         os.symlink(source, destination)
     except OSError as e:
         if e.errno != errno.EEXIST or os.readlink(destination) != source:
-- 
2.50.0.173.g8b6f19ccfc3a

[wic][PATCH 3/4] oe/path: canonicalize('') should return '' rather than the cwd

[Trevor Woerner]

canonicalize() splits its input on the separator and, for every token
that does not contain an unexpanded bitbake variable, appends
os.path.realpath(path). It never guarded against an empty token, and
os.path.realpath('') returns the current working directory. So
canonicalize('') and canonicalize(None) produced the cwd instead of an
empty result, and a stray separator such as 'a,,b' injected a spurious
cwd entry between the real paths.

Skip empty tokens alongside the '$' tokens the loop already skips, so
empty input canonicalizes to '' and redundant separators no longer
introduce phantom cwd entries.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
 src/wic/oe/path.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/wic/oe/path.py b/src/wic/oe/path.py
index 13c52b363694..95bb9648fab6 100644
--- a/src/wic/oe/path.py
+++ b/src/wic/oe/path.py
@@ -355,7 +355,11 @@ def canonicalize(paths, sep=','):
     # prefixes in sting compares later on, where the slashes then are important.
     canonical_paths = []
     for path in (paths or '').split(sep):
-        if '$' not in path:
+        # Skip empty tokens as well as unexpanded bitbake variables: an
+        # empty path would otherwise reach os.path.realpath(''), which
+        # returns the current working directory, so canonicalize('') or
+        # canonicalize(None) would wrongly produce the cwd instead of ''.
+        if path and '$' not in path:
             trailing_slash = path.endswith('/') and '/' or ''
             canonical_paths.append(os.path.realpath(path) + trailing_slash)
 
-- 
2.50.0.173.g8b6f19ccfc3a

[wic][PATCH 4/4] tests/unit/test_oe_path: cover oe/path's own path logic

[Trevor Woerner]

Add unit coverage for the wic-specific behaviour in oe/path.py:

  - join() keeps an absolute right-hand component relative and
    normalises redundant separators, '.', and '..'.
  - is_path_parent() treats containment by path component, so a shared
    string prefix such as /usrlocal is not below /usr, and every
    supplied path must be below the parent.
  - symlink() creates links, is idempotent for a matching link, raises
    on a conflicting one, and with force=True replaces the literal
    destination -- including names with glob metacharacters and an
    existing directory -- without deleting glob siblings.
  - make_relative_symlink() leaves non-links and already-relative links
    alone and rewrites an absolute link to a working relative one.
  - canonicalize() expands real paths, drops '$' and empty tokens, keeps
    trailing slashes, and returns '' for '' and None.
  - which_wild() honours an explicit search path, expands wildcards, and
    returns the first match per name (last with reverse=True).
  - realpath() resolves links below root, rejects paths outside root, and
    falls back cleanly when os.path.isdir() raises.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
 tests/unit/test_oe_path.py | 298 +++++++++++++++++++++++++++++++++++++
 1 file changed, 298 insertions(+)
 create mode 100644 tests/unit/test_oe_path.py

diff --git a/tests/unit/test_oe_path.py b/tests/unit/test_oe_path.py
new file mode 100644
index 000000000000..a978fcf789d2
--- /dev/null
+++ b/tests/unit/test_oe_path.py
@@ -0,0 +1,298 @@
+"""
+Unit tests for wic/oe/path.py, covering wic's own path logic: join,
+is_path_parent, symlink, make_relative_symlink, canonicalize, which_wild,
+and realpath. Thin standard-library wrappers (relative -> os.path.relpath,
+find -> os.walk) are left to the standard library.
+"""
+import os
+import sys
+from pathlib import Path
+
+import pytest
+
+_SRC = Path(__file__).resolve().parent.parent.parent / "src"
+if str(_SRC) not in sys.path:
+    sys.path.insert(0, str(_SRC))
+
+from wic.oe.path import (
+    join,
+    is_path_parent,
+    symlink,
+    make_relative_symlink,
+    canonicalize,
+    which_wild,
+    realpath,
+)
+
+
+class TestJoin:
+    """join() is os.path.normpath("/".join(paths)); unlike os.path.join it
+    does not treat an absolute right-hand component specially."""
+
+    def test_two_paths(self):
+        assert join("a", "b") == "a/b"
+
+    def test_three_paths(self):
+        assert join("a", "b", "c") == "a/b/c"
+
+    def test_absolute_rhs_is_not_special(self):
+        # os.path.join("a", "/b") == "/b"; join() keeps it relative.
+        assert join("a", "/b") == "a/b"
+
+    def test_redundant_separators_normalised(self):
+        assert join("a//", "b") == "a/b"
+
+    def test_pardir_normalised(self):
+        assert join("a", "..", "b") == "b"
+
+    def test_curdir_normalised(self):
+        assert join("a", ".", "b") == "a/b"
+
+    def test_single_component(self):
+        assert join("a") == "a"
+
+    def test_empty_leading_component_is_absolute(self):
+        # "/".join(["", "b"]) is "/b", which normpath leaves absolute.
+        assert join("", "b") == "/b"
+
+
+class TestIsPathParent:
+    def test_direct_child(self):
+        assert is_path_parent("/usr", "/usr/bin") is True
+
+    def test_deep_child(self):
+        assert is_path_parent("/usr", "/usr/share/doc/readme") is True
+
+    def test_unrelated_path(self):
+        assert is_path_parent("/usr", "/tmp") is False
+
+    def test_child_is_not_parent_of_its_parent(self):
+        assert is_path_parent("/usr/bin", "/usr") is False
+
+    def test_prefix_string_is_not_a_path_parent(self):
+        # "/usrlocal" shares a string prefix with "/usr" but is not below it;
+        # the trailing-separator handling must reject it.
+        assert is_path_parent("/usr", "/usrlocal") is False
+
+    def test_no_paths_returns_false(self):
+        assert is_path_parent("/usr") is False
+
+    def test_all_paths_must_be_below(self):
+        assert is_path_parent("/usr", "/usr/bin", "/usr/lib") is True
+
+    def test_one_path_outside_fails(self):
+        assert is_path_parent("/usr", "/usr/bin", "/tmp") is False
+
+
+class TestSymlink:
+    def test_creates_symlink(self, tmp_path):
+        src = tmp_path / "target.txt"
+        src.write_text("content")
+        dst = tmp_path / "link"
+        symlink(str(src), str(dst))
+        assert os.path.islink(str(dst))
+        assert os.readlink(str(dst)) == str(src)
+
+    def test_matching_existing_symlink_is_idempotent(self, tmp_path):
+        src = tmp_path / "target.txt"
+        src.write_text("content")
+        dst = tmp_path / "link"
+        symlink(str(src), str(dst))
+        symlink(str(src), str(dst))
+        assert os.readlink(str(dst)) == str(src)
+
+    def test_conflicting_existing_symlink_raises(self, tmp_path):
+        src1 = tmp_path / "t1.txt"
+        src2 = tmp_path / "t2.txt"
+        src1.write_text("a")
+        src2.write_text("b")
+        dst = tmp_path / "link"
+        symlink(str(src1), str(dst))
+        with pytest.raises(OSError):
+            symlink(str(src2), str(dst))
+
+    def test_force_overwrites_existing_symlink(self, tmp_path):
+        src1 = tmp_path / "t1"
+        src2 = tmp_path / "t2"
+        src1.write_text("a")
+        src2.write_text("b")
+        dst = tmp_path / "link"
+        symlink(str(src1), str(dst))
+        symlink(str(src2), str(dst), force=True)
+        assert os.readlink(str(dst)) == str(src2)
+
+    def test_force_replaces_destination_with_glob_metacharacters(self, tmp_path):
+        # A destination whose name contains glob metacharacters must still be
+        # replaced. Routing the removal through glob (the old behaviour) fails
+        # to match "link[1]" against itself, leaving the stale entry in place.
+        src = tmp_path / "target"
+        src.write_text("content")
+        dst = tmp_path / "link[1]"
+        dst.write_text("stale regular file")
+        symlink(str(src), str(dst), force=True)
+        assert os.path.islink(str(dst))
+        assert os.readlink(str(dst)) == str(src)
+
+    def test_force_does_not_delete_glob_siblings(self, tmp_path):
+        # The destination "keep?.txt" does not exist, but a sibling
+        # "keepX.txt" matches it as a glob pattern. force=True must remove
+        # only the literal destination, never a pattern sibling.
+        sibling = tmp_path / "keepX.txt"
+        sibling.write_text("do not delete me")
+        src = tmp_path / "target"
+        src.write_text("content")
+        dst = tmp_path / "keep?.txt"
+        symlink(str(src), str(dst), force=True)
+        assert sibling.exists()
+        assert os.readlink(str(dst)) == str(src)
+
+    def test_force_replaces_existing_directory(self, tmp_path):
+        # An existing directory at the destination is torn down (EISDIR ->
+        # rmtree) before the link is created.
+        src = tmp_path / "target"
+        src.write_text("content")
+        dst = tmp_path / "dir"
+        dst.mkdir()
+        (dst / "child").write_text("x")
+        symlink(str(src), str(dst), force=True)
+        assert os.path.islink(str(dst))
+        assert os.readlink(str(dst)) == str(src)
+
+
+class TestMakeRelativeSymlink:
+    def test_non_symlink_is_ignored(self, tmp_path):
+        regular = tmp_path / "file.txt"
+        regular.write_text("content")
+        make_relative_symlink(str(regular))
+        assert regular.is_file()
+
+    def test_already_relative_symlink_is_unchanged(self, tmp_path):
+        target = tmp_path / "target.txt"
+        target.write_text("content")
+        link = tmp_path / "link"
+        os.symlink("target.txt", str(link))
+        make_relative_symlink(str(link))
+        assert os.readlink(str(link)) == "target.txt"
+
+    def test_absolute_symlink_becomes_relative_and_resolves(self, tmp_path):
+        target = tmp_path / "target.txt"
+        target.write_text("content")
+        link = tmp_path / "link"
+        os.symlink(str(target), str(link))
+        assert os.path.isabs(os.readlink(str(link)))
+        make_relative_symlink(str(link))
+        result = os.readlink(str(link))
+        assert not os.path.isabs(result)
+        assert os.path.exists(str(link))
+        assert Path(str(link)).read_text() == "content"
+
+
+class TestCanonicalize:
+    def test_real_path_returned(self, tmp_path):
+        result = canonicalize(str(tmp_path))
+        assert result == os.path.realpath(str(tmp_path))
+
+    def test_unexpanded_variable_is_skipped(self):
+        assert canonicalize("$SOME_VAR") == ""
+
+    def test_variable_token_dropped_leaving_real_path(self, tmp_path):
+        result = canonicalize("$VAR," + str(tmp_path))
+        # The "$VAR" token is dropped entirely: no empty placeholder, no
+        # leading separator, just the canonical real path.
+        assert result == os.path.realpath(str(tmp_path))
+
+    def test_empty_string_returns_empty_string(self):
+        # os.path.realpath("") is the cwd; canonicalize("") must not leak it.
+        assert canonicalize("") == ""
+
+    def test_none_returns_empty_string(self):
+        assert canonicalize(None) == ""
+
+    def test_empty_token_between_paths_is_dropped(self, tmp_path):
+        p1 = tmp_path / "a"
+        p2 = tmp_path / "b"
+        p1.mkdir()
+        p2.mkdir()
+        # The stray separator in "a,,b" must not inject a cwd entry.
+        result = canonicalize("%s,,%s" % (p1, p2))
+        assert result == "%s,%s" % (
+            os.path.realpath(str(p1)), os.path.realpath(str(p2)))
+
+    def test_trailing_slash_preserved(self, tmp_path):
+        result = canonicalize(str(tmp_path) + "/")
+        assert result == os.path.realpath(str(tmp_path)) + "/"
+
+
+class TestWhichWild:
+    def test_finds_existing_executable(self):
+        results = which_wild("python3")
+        assert results
+        assert all(os.path.isabs(p) for p in results)
+        assert all(os.path.basename(p) == "python3" for p in results)
+
+    def test_missing_tool_returns_empty(self):
+        assert which_wild("totally_nonexistent_tool_xyz") == []
+
+    def test_explicit_search_path(self, tmp_path):
+        tool = tmp_path / "mytool"
+        tool.write_text("#!/bin/sh\n")
+        tool.chmod(0o755)
+        results = which_wild("mytool", path=str(tmp_path))
+        assert results == [str(tool)]
+
+    def test_wildcard_pattern(self, tmp_path):
+        (tmp_path / "foo-a").write_text("")
+        (tmp_path / "foo-b").write_text("")
+        (tmp_path / "bar").write_text("")
+        results = which_wild("foo-*", path=str(tmp_path))
+        assert sorted(os.path.basename(p) for p in results) == ["foo-a", "foo-b"]
+
+    def test_first_match_per_name_wins(self, tmp_path):
+        # A name found in an earlier PATH element shadows the same name later.
+        first = tmp_path / "first"
+        second = tmp_path / "second"
+        first.mkdir()
+        second.mkdir()
+        (first / "tool").write_text("")
+        (second / "tool").write_text("")
+        search = "%s:%s" % (first, second)
+        assert which_wild("tool", path=search) == [str(first / "tool")]
+        # reverse=True walks PATH the other way, so the other copy wins.
+        assert which_wild("tool", path=search, reverse=True) == [str(second / "tool")]
+
+
+class TestRealpath:
+    def test_resolves_symlink_below_root(self, tmp_path):
+        target = tmp_path / "real"
+        target.mkdir()
+        link = tmp_path / "lnk"
+        os.symlink("real", str(link))
+        result = realpath(str(link), str(tmp_path))
+        assert result == str(target)
+
+    def test_plain_path_is_returned(self, tmp_path):
+        sub = tmp_path / "d"
+        sub.mkdir()
+        assert realpath(str(sub), str(tmp_path)) == str(sub)
+
+    def test_path_outside_root_raises(self, tmp_path):
+        root = tmp_path / "root"
+        outside = tmp_path / "outside"
+        root.mkdir()
+        outside.mkdir()
+        with pytest.raises(OSError):
+            realpath(str(outside), str(root))
+
+    def test_isdir_failure_falls_back_without_nameerror(self, tmp_path, monkeypatch):
+        # The final stanza of __realpath tolerates any failure from
+        # os.path.isdir by treating the path as not-a-directory. Force isdir
+        # to raise and confirm the fallback returns the resolved path rather
+        # than raising NameError from an undefined fallback value.
+        sub = tmp_path / "d"
+        sub.mkdir()
+
+        def boom(_path):
+            raise OSError("synthetic stat failure")
+
+        monkeypatch.setattr(os.path, "isdir", boom)
+        assert realpath(str(sub), str(tmp_path), use_physdir=False) == str(sub)
-- 
2.50.0.173.g8b6f19ccfc3a