🐛 Use the physical source directory for generated PlantUML diagrams

[ubmarco]

Closes #1749

Problem

sphinxcontrib-plantuml starts the PlantUML process with
cwd = os.path.join(srcdir, node["incdir"]), so incdir decides where relative
!include paths are resolved.

Sphinx-Needs derived incdir from the logical docname:

puml_node["incdir"] = os.path.dirname(current_needuml["docname"])

For a document whose source file does not physically live under srcdir — e.g.
one contributed by sphinx-mounts,
which registers an absolute external path for its docname — that yields a
directory that does not exist. subprocess.Popen fails with ENOENT, which
sphinxcontrib-plantuml reports as:

WARNING: plantuml command '…/plantuml' cannot be run [plantuml]

The message is misleading: the PlantUML executable is fine, the cwd is not.

Fix

Derive incdir from env.doc2path(docname, base=False) instead — the same
thing sphinxcontrib-plantuml's own uml directive does — through a new shared
set_plantuml_paths() helper in diagrams_common.py.

The bug was present at four sites, all now routed through the helper:

directive	file
needuml / needarch	directives/needuml.py
needflow (plantuml engine)	directives/needflow/_plantuml.py
needsequence	directives/needsequence.py
needgantt	directives/needgantt.py

Why base=False and not an absolute path

doc2path(..., base=False) returns a srcdir-relative path for an ordinary
document and the absolute external path for a mounted one. Since
os.path.join discards its left operand when the right one is absolute, PlantUML
ends up with the correct cwd in both cases.

Picking the relative form matters beyond taste:

• Ordinary documents get a byte-identical incdir to before
  (dirname("a/b/c") == dirname("a/b/c.rst")), so nothing changes for existing
  projects.
• incdir feeds hash_plantuml_node(), i.e. PlantUML's content-addressed cache
  key. Always emitting an absolute path would invalidate every cached diagram and
  make the cache non-portable across machines and checkout locations. This way
  only genuinely-mounted documents get an absolute key.

Test

tests/test_plantuml_incdir.py builds a host project plus a sibling bundle
mounted via a ~20-line stand-in for sphinx-mounts (it reproduces the one trick
that matters — an absolute path in Project._docname_to_path — so the test
carries no new dependency), and asserts:

• an ordinary nested host document keeps incdir == "sub";
• the mounted document gets the bundle's physical directory, not "mounted";
• PlantUML actually runs and resolves a bundle-relative !include, with zero
  build warnings.

Verified failing before the fix (['mounted', 'mounted'] != [<bundle>, <bundle>])
and passing after.

Full suite: 1018 passed. The two tests/test_sn_collapse_button.py failures on
this branch reproduce identically on master and are unrelated.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.42%. Comparing base (4e10030) to head (e8d0791).
⚠️ Report is 308 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1750      +/-   ##
==========================================
+ Coverage   86.87%   89.42%   +2.54%     
==========================================
  Files          56       73      +17     
  Lines        6532    10614    +4082     
==========================================
+ Hits         5675     9492    +3817     
- Misses        857     1122     +265     

Flag	Coverage Δ
pytests	89.42% <100.00%> (+2.54%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[chrisjsewell]

haven't checked through the actual code, but approve in principle

[ubmarco]

Review

Right root cause, right API. Deriving incdir from env.doc2path(docname, base=False) is exactly what sphinxcontrib-plantuml's own uml directive does (plantuml.py:140-145), and collapsing four copy-pasted sites into one helper is the correct shape. I verified the base=False reasoning against Sphinx's Project.doc2path: with a falsy base no srcdir join happens, and Project.discover stores paths through path_stabilize/canon_path, so stored paths are forward-slash even on Windows — the "byte-identical for ordinary documents" claim holds on all platforms, which the green Windows/Sphinx 9.1 jobs confirm. All four call sites pass the correct docname key. Nothing here blocks merge.

1. [Medium] Two of the four refactored call sites are never exercised (correctness / test coverage)

The PR's stated value is that the bug existed at four sites and all four now route through the helper — but the fixture document only contains needuml and needflow, so needsequence.py:194 and needgantt.py:354 never execute in the new test. needarch is covered by proxy (it shares the unconditional line in process_needuml).

sphinx-needs/tests/test_plantuml_incdir.py

Lines 138 to 145 in b8a5502

	.. needuml::
	!include lib.puml
	MountedIncludeWorked -> MountedLocal
	.. needflow::
	:filter: id == "MOUNTED_REQ"

I read all four sites and they do pass the right docname, so there is no present bug — the gap is regression protection for the two untested sites. Adding a needsequence and a needgantt directive to BUNDLE_INDEX (and extending the expected incdirs["mounted/index"] list) closes it cheaply.

2. [Medium] Mounted documents get a location-dependent PlantUML cache key (performance)

incdir is hashed into PlantUML's content-addressed key (hash_plantuml_node, plantuml.py:179-185), and that key becomes the _images/plantuml-<key>.svg filename. For mounted documents incdir is now an absolute path, so any relocation of the source tree changes the key — which is precisely the Bazel runfiles/sandbox setup from #1749. Result: the plantuml_cache never hits and every mounted diagram re-renders (one JVM per diagram) after a relocation, while _images accumulates orphans.

sphinx-needs/sphinx_needs/diagrams_common.py

Lines 133 to 135 in b8a5502

	"""
	puml_node["incdir"] = os.path.dirname(env.doc2path(docname, base=False))
	puml_node["filename"] = os.path.split(docname)[1] # Needed for plantuml >= 0.9

The docstring acknowledges the non-portability but frames it as acceptable because it only affects mounted documents — which are exactly the documents this PR exists to fix. os.path.relpath(dirname(physical), env.srcdir) yields the identical cwd (PlantUML joins it onto srcdir and the OS resolves the ..) with a relocation-stable key; I checked that a runfiles root moving from /build/... to /tmp/sandbox-9876/... gives a byte-identical relative incdir. It needs a try/except ValueError fallback to the absolute path for cross-drive mounts on Windows. Worth considering, but a follow-up is fine.

3. [Low] The mount stand-in stores str where Sphinx and sphinx-mounts store Path (durability)

From Sphinx 8.2 on, Project._docname_to_path/_path_to_docname hold Path objects, and the real sphinx-mounts 0.1.2 registers Path too. The test passes on 7.4/8.2/9.1 regardless, because doc2path re-wraps whatever it finds — but the docstring claims to reproduce "the one trick that matters", and this is the one detail where it doesn't. A str key in _path_to_docname would silently miss a Path-keyed path2doc() lookup if the test is ever extended (e.g. an image referenced from the mounted file). Dropping the str() calls costs nothing.

sphinx-needs/tests/test_plantuml_incdir.py

Lines 54 to 57 in b8a5502

	self.docnames.add(docname)
	self._docname_to_path[docname] = str(src)
	self._path_to_docname[str(src)] = docname
	docs.add(docname)

4. [Low] Blanket zero-warning assertion widens the test's blast radius (durability)

Every other use of get_warnings_list in the suite asserts a specific expected list; this is the only blanket == []. It ties "did the incdir fix work" to "zero warnings anywhere in this build", so an unrelated future warning fails this test with a diff that points at the wrong culprit. Asserting no plantuml-typed warning would isolate the intent.

sphinx-needs/tests/test_plantuml_incdir.py

Lines 193 to 196 in b8a5502

	# End-to-end proof that PlantUML ran and resolved the bundle-relative
	# ``!include``. Before the fix this failed with "plantuml command cannot be
	# run", because the non-existent cwd surfaced as ENOENT from ``subprocess``.
	assert get_warnings_list(app) == []

Minor: the helper computes the physical path but still derives filename from the logical docname, and the docstring — thorough on incdir — never mentions filename. Keeping the value is right (changing it would alter the -filename label), but a half-sentence saying so would help the next reader.

Other dimensions

• Backwards compatibility — no break for ordinary projects; incdir, and therefore every existing cache key, is unchanged. Docnames absent from the project fall back to docname + suffix, whose dirname also matches the old value.
• Architecture — fits existing patterns; env.doc2path is already used at all four sites for puml_node.source (added in 👌 Improve plantuml warnings #982), and ⬆️ Drop Sphinx<7.4, test against Python 3.13 #1447 set the same precedent of replacing hand-rolled docname math with a Sphinx path API.
• History — I looked for a commit that deliberately chose the logical docname over a physical path; there is none, so this is not reintroducing an old fix. The # Needed for plantuml >= 0.9 comment (from 9731de1b) guards the filename line, which is untouched.
• Past reviews — the only prior review is an approval in principle with the code unread, so there are no outstanding comments.

Verified locally

tests/test_plantuml_incdir.py passes at b8a5502; reverting just the incdir line reproduces ['mounted', 'mounted'] != [<bundle>, <bundle>], so the test genuinely pins the fix. Full CI is green across the 7.4/8.2/9.1 × Linux/Windows matrix.

[ubmarco]

Follow-up on the review findings

Addressed in e8d0791.

1. [Medium] Untested call sites — fixed

The mounted bundle document now exercises all four directives that route through set_plantuml_paths(), so needsequence and needgantt are no longer dark. Reverting the incdir line now fails with four wrong entries instead of two.

Two fixture details were needed to get there, both worth recording because they are easy to trip over again:

• process_needsequence throws its PlantUML node away and substitutes "no needs found" unless it can draw at least one connection, and a connection needs a full sender → message → receiver chain over the traversed link type — a two-need chain silently yields no node at all. The single MOUNTED_REQ is therefore now a three-need chain.
• needgantt warns for every need without :duration:, which the zero-warning assertion below would flag, so all three needs carry one.

3. [Low] str vs Path in the mount stand-in — withdrawn, my finding was wrong

I recommended dropping the str() calls so the stand-in stores Path like Sphinx ≥8.2 and the real sphinx-mounts do. That breaks the build on Sphinx 7.4:

TypeError: 'PosixPath' object is not subscriptable
  sphinx/builders/html/__init__.py:612, in get_doc_context
    source_suffix = self.env.doc2path(docname, False)[len(docname):]

Sphinx does not store a plain Path there — it stores _StrPath, a Path subclass that deliberately keeps __getitem__/__len__ precisely because its own HTML builder slices the value to recover the source suffix. str is the one type that works across the whole supported sphinx>=7.4,<10 range, so the original code was right. I left the str() calls alone and added a comment explaining why, so the next reader does not "modernise" it back.

4. [Low] Blanket zero-warning assertion — keeping it, by design

Keeping assert get_warnings_list(app) == [] as-is. Failing on any new warning is the point: it forces someone to look, which is more valuable than the narrower assertion I suggested. Noting it here so the intent is on the record rather than looking like an oversight.

2. [Medium] Cache-key churn for mounted documents — withdrawn, keeping the absolute path

I proposed rewriting only the absolute (mounted) case into a srcdir-relative incdir, so PlantUML's content-addressed key would survive relocation:

incdir = os.path.dirname(env.doc2path(docname, base=False))
if os.path.isabs(incdir):                      # mounted document
    try:
        incdir = os.path.relpath(incdir, env.srcdir)
    except ValueError:                          # different drive on Windows
        pass

It produces a stable key, and os.path.join(srcdir, "../bundle") does name the right directory. It must still not be merged: .. handed to chdir is resolved by the kernel against the real directory tree, not lexically, so when srcdir is reached through a symlink the .. escapes to the symlink's true parent:

srcdir  = .../slink/srcdir   (symlink -> real/docs)
bundle  = .../slink/bundle
relpath = ../bundle
chdir(".../slink/srcdir/../bundle")  ->  FileNotFoundError [Errno 2]

That is the same ENOENT as #1749, reintroduced in exactly the kind of symlink forest (Bazel runfiles) this PR exists to fix. os.path.realpath on either side does not help, because sphinxcontrib-plantuml joins onto its own unresolved builder.srcdir.

So the absolute incdir on this branch is the correct trade-off: correctness under symlinks beats cache portability. The residual impact is also narrower than the review implied — it only costs anything when the PlantUML cache directory is persisted across builds while the source path changes. A proper fix belongs upstream, by letting a PlantUML node carry an explicit cache key instead of hashing its working directory.

---

Full suite on this branch: 1018 passed, 13 skipped, 0 failed.