sphinx-doc / sphinx

The Sphinx documentation generator
https://www.sphinx-doc.org/
Other
6.59k stars 2.12k forks source link

Page requisite behaviour inconsistent between Windows and others #10910

Open davidwuehrer opened 2 years ago

davidwuehrer commented 2 years ago

Describe the bug

The method relfn2path of the class BuildEnvironment in the file sphinx/environment/__init__.py supposedly treats absolute paths as relative to the source dir. This is not the case in Windows. Absolute paths to page requisites provided by conf.py in the source dir remain absolute paths in Windows, but become relative paths in every other OS.

The reason is that os.path.join in Windows applied to two (or more) absolute paths returns the right-most absolute path, throwing away everything to the left of it. This is different in other OSes, in which an absolute path is made relative to the preceding path.

The effect in relfn2path is that absolute paths to page requisites declared by conf.py in the source dir remain those same absolute in Windows, but not anywhere else, causing inconsistent behaviour.

I would suggest the following patch:

@@ -390,8 +390,8 @@ class BuildEnvironment:
         containing document.
         """
         filename = os_path(filename)
-        if filename.startswith('/') or filename.startswith(os.sep):
-            rel_fn = filename[1:]
+        if os.path.isabs(filename):
+            rel_fn = filename[filename.index(os.path.sep)+1:]
         else:
             docdir = path.dirname(self.doc2path(docname or self.docname,

This will make the behaviour consistent.

It will break Windows-only projects that rely on absolute paths being absolute rather than relative to the source dir.

How to Reproduce

from sphinx.environment import BuildEnvironment
env = BuildEnvironment()
env.srcdir = 'srcdir'
env.relfn2path('C:\\project\\srcdir\\img.png', 'document')

Expected behavior

Paths starting with a slash or os.path.sep are understood as relative to the source dir by relfn2path; in most operating systems this is the same as an absolute path, meaning that actually absolute paths are not possible, and probably not desired.

In Windows, actually absolute paths are possible, which causes sphinx to behave differently between operating systems if conf.py declares actually absolute paths, for example using os.path.abspath.

This makes the behaviour of sphinx inconsistent across platforms. It should be consistent. Either absolute paths remaining absolute is an intended feature, then it should be available in operating systems other than Windows as well; or it is not, then absolute Windows paths should be treated the same way as absolute paths everywhere else.

AA-Turner commented 1 year ago

@davidwuehrer thank you for this comprehensive report.

As an alternate patch, what do you think about:

@@ -412,6 +412,7 @@ class BuildEnvironment:
         containing document.
         """
         filename = os_path(filename)
+        _, filename = os.path.splitdrive(filename)
         if filename.startswith(('/', os.sep)):
             rel_fn = filename[1:]
         else:
12rambau commented 5 months ago

I tried both solutions in this PR (https://github.com/sphinx-doc/sphinx/pull/12405) and tested it in my own repository: https://github.com/sphinx-contrib/icon/pull/34

None of them changed anything from my side, all was working in other OSs and wasn't working in Windows.

picnixz commented 5 months ago

None of them changed anything from my side, all was working in other OSs and wasn't working in Windows.

I'm pretty sure it has something to do with the drive that can be changed. But then, I'm wondering how to properly fix it.

electric-coder commented 5 months ago

I'm pretty sure it has something to do with the drive that can be changed. But then, I'm wondering how to properly fix it.

Given that support for Python 3.8 was dropped in Sphinx 7.2.0 it should be possible to remove use of the os.path module altogether in favor using only pathlib (it would simplify the codebase overall to use one less module). However, I'm assuming that was tried and removed in Sphinx 7.2.3 for causing bugs with the note: "Restore support string methods on path objects. This is deprecated and will be removed in Sphinx 8."

picnixz commented 5 months ago

I don't think it has anything to do with the pathlib and the os.path implementation. Those are compatible and are interchangable (os.path works with Path inputs).