Skip to content

fix: stop the remaining iter_*_dirs yielding duplicate or joined paths - #520

Merged
gaborbernat merged 3 commits into
tox-dev:mainfrom
darrenhuai:fix/iter-dirs-cache-state-log-runtime
Aug 10, 2026
Merged

fix: stop the remaining iter_*_dirs yielding duplicate or joined paths#520
gaborbernat merged 3 commits into
tox-dev:mainfrom
darrenhuai:fix/iter-dirs-cache-state-log-runtime

Conversation

@darrenhuai

Copy link
Copy Markdown
Contributor

iter_config_dirs and iter_data_dirs stopped yielding the same directory twice in #469, for the case where use_site_for_root redirects the user dirs to their site equivalents. The cache, state, log and runtime families were left on the base implementation in PlatformDirsABC, so they still do it. As root with use_site_for_root=True:

iter_cache_dirs      ['/var/cache/foo', '/var/cache/foo']
iter_state_dirs      ['/var/lib/foo',   '/var/lib/foo']
iter_log_dirs        ['/var/log/foo',   '/var/log/foo']
iter_runtime_dirs    ['/run/foo',       '/run/foo']

By that point user_cache_dir and friends already return the site path, and the base implementation yields it a second time.

While writing tests for that I ran into a second one on macOS. site_cache_dir joins the Homebrew entry and /Library/Caches with os.pathsep when multipath is set, so iter_cache_dirs handed back a single string holding two paths, and iter_cache_paths turned it into Path('/opt/homebrew/var/cache/foo:/Library/Caches/foo'). iter_data_dirs splits properly because _site_data_dirs already exists as a list; cache had no equivalent. Added _site_cache_dirs in the same shape and had site_cache_dir format from it, so its own output is unchanged.

Neither case shows up in a default configuration — use_site_for_root is off by default, and the macOS one needs both Homebrew and multipath — so this is latent rather than something callers are likely to have hit.

On the tests: each of the four Unix iterators gets a no-duplicates check as root and a counterpart as non-root, so a fix that simply dropped the user dir would fail the second one. macOS covers iter_cache_dirs under Homebrew with multipath set and unset, plus iter_cache_paths to pin down the Path behaviour.

PR tox-dev#469 stopped iter_config_dirs and iter_data_dirs from yielding the same
directory twice on Unix when use_site_for_root redirects the user dirs to their
site equivalents, but the cache, state, log and runtime families were left on
the base implementation. Running as root with use_site_for_root=True they still
yield the site directory twice, because user_cache_dir and friends already
return the site path by then.

iter_cache_dirs on macOS had a second problem. site_cache_dir joins the Homebrew
and /Library/Caches entries with os.pathsep when multipath is set, so the
iterator handed back one string holding two paths, and iter_cache_paths turned
that into a single unusable Path. Pulled the list out into _site_cache_dirs, the
same shape as _site_data_dirs, and yield from that instead.

Neither case shows up in a default configuration - use_site_for_root is off by
default, and the macOS one needs both Homebrew and multipath - so this is
latent rather than something most callers will have hit.
@gaborbernat
gaborbernat merged commit a01396c into tox-dev:main Aug 10, 2026
36 checks passed
gaborbernat added a commit that referenced this pull request Aug 24, 2026
#520 fixed this for the Unix `use_site_for_root` case, but the problem
is more general: whenever a `site_*_dir` resolves to the same string as
its `user_*_dir`, the iterator hands the caller that directory twice.
Code that merges config or data files, the use case `docs/howto.rst`
recommends these iterators for, then reads and applies the same file
twice.

The case most likely to bite is Unix `iter_runtime_dirs()` with
`XDG_RUNTIME_DIR` set, the normal state on any systemd system:

```python
>>> os.environ["XDG_RUNTIME_DIR"] = "/run/user/1000"
>>> list(Unix(appname="foo").iter_runtime_dirs())
['/run/user/1000/foo', '/run/user/1000/foo']
```

Both `user_runtime_dir` and `site_runtime_dir` read that variable, so
the two entries are identical. It slipped through last time because the
two tests #520 added both unset `XDG_RUNTIME_DIR` before asserting,
leaving the common configuration untested.

Looking for the same shape elsewhere turned up more. Windows and macOS
both define `site_runtime_dir` as `user_runtime_dir`, so their
`iter_runtime_dirs()` duplicates too. Android defines every `site_*_dir`
as its `user_*_dir`, so all six of its iterators returned a pair of
identical paths.

Rather than override the iterators per platform a third time,
`PlatformDirsABC` keeps the `yield user, yield site` logic in a
protected `_iter_*_dirs` and deduplicates at the single public choke
point. Unix and macOS override the protected hooks instead of the public
methods, so their behaviour does not change and one place owns the
invariant. The dedupe preserves order and still lets every distinct
entry through; only exact repeats drop out.

Two things the dedupe rests on, each with a test:

- The `_use_site` guards in `_iter_config_dirs` and `_iter_data_dirs`
survive the dedupe rather than becoming redundant. Under `multipath` the
user dir is an `os.pathsep`-joined string that equals no single site
entry, so nothing would drop it.
- `_unique` stays lazy. Under `ensure_exists` reading a `site_*_dir`
creates it, so draining the source up front would create directories for
a caller that stops after the first entry.

This is duplicate work rather than a wrong answer, so it stays latent
for callers that only do existence checks. It matters for callers that
accumulate.

Testing: full suite passes, plus a regression test per affected
platform. I mutation-tested the new tests by replacing the dedupe with a
passthrough and confirmed all nine fail, then restored it. `ty check
--error-on-warning`, `ruff format --check` and the `-W` docs build are
clean. Verified on Windows itself and the other platforms through the
existing mocking fixtures.

One thing I noticed but did not touch: macOS `site_state_dir` is
documented as "same as `site_data_dir`" but calls `_base_site_dirs()`,
so it ignores `$XDG_DATA_DIRS` while `site_data_dir` honours it. That
looks deliberate given there is no `XDG_STATE_DIRS` and Unix behaves the
same way, so I left the code alone, but the docstring misleads either
way. Happy to fix it here or separately.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bernát Gábor <gaborjbernat@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants