Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make sphinx Breaks when Certain Untracked Files are Present #2404

Closed
ericsnowcurrently opened this issue Mar 9, 2022 · 13 comments · Fixed by #2415
Closed

make sphinx Breaks when Certain Untracked Files are Present #2404

ericsnowcurrently opened this issue Mar 9, 2022 · 13 comments · Fixed by #2415
Assignees
Labels
bug infra Core infrastructure for building and rendering PEPs

Comments

@ericsnowcurrently
Copy link
Member

  • make sphinx fails if I have some file like "pep-0684-old.rst" (the filename regex in pep_sphinx_extensions/pep_zero_generator/pep_index_generator.py isn't precise enough)
  • make sphinx fails if I have a venv dir named "venv-spam" sitting at the top of the repo (tries to parse a vendor.txt file inside the venv)

In both cases I'd expect it to work fine, ignoring the relevant files.

@AA-Turner
Copy link
Member

The first case is easier to solve, the second is not possible as far as I'm aware -- Sphinx has no way to specify a whitelist of files -- the exclude_files list is the only mechanism and as such must exhaustivley list things. Sorry.

A

@ericsnowcurrently
Copy link
Member Author

I'm not familiar with how Sphinx works. It is surprising, though, that it can't handle the case. IIUC, it is blindly walking the entire directory tree and doesn't support a wildcard whitelist. What does it need to recurse in the PEP repo?

@AA-Turner
Copy link
Member

IIUC, it is blindly walking the entire directory tree and doesn't support a wildcard whitelist. What does it need to recurse in the PEP repo?

You're right. And I'd ask the same question on default recursion. One thing that might be possible is adding venv* to exclude_files in conf.py & hoping Sphinx does string-matching on paths, though untested.

A

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Mar 10, 2022

Sorry for the delay responding; I've been dealing with urgent family issues most of today.

Good catch. It seems like the issue is ultimately due to having to try to render files with the legacy .txt extension along with .rst, which results in irrelevant .txt files being picked up. (Sidenote: it seems like they were mass renamed before, but that was reverted, IIRC).

However, since full Unix-style glob syntax is supported in exclude_patterns, we can exclude any .txt files in subdirectories (like this example), and any in the root directory that don't match the exact pattern used for .txt PEPs (i.e. the equivalent of the regex ^pep?[0-9]{4}\.txt$), which fully resolves the issue for the general case.

I implemented and tested this on some sample files in the root and subdirectories (including your example, and a few with varying degrees of similar names), and it excluded all of them, while not excluding any valid PEPs. Therefore, I opened PR #2405 which fixes this.

@hugovk
Copy link
Member

hugovk commented Mar 11, 2022

I think the big difference here to "normal" Sphinx docs builds is we're reading the source documentation files from the root, and trying to ignore certain support and any other random user files.

Whereas normal Sphinx builds virtually always read source files from a subdirectory such as docs:

Is it feasible to move our source PEP files into a subdirectory?

Can/should Sphinx be made to ignore non-SVN non-CVS non-source-controlled files? (Edit: fixed typo!)

@AA-Turner
Copy link
Member

Is it feasible to move our source PEP files into a subdirectory?

Last time mass moves were done, it was reverted due to the GitHub interface: #462 (comment)

This is still the case: https://github.com/python/peps/commits/main/.github/workflows/render.yml (the blame UI does track moves, though -- a fun inconsistency).

A

@encukou
Copy link
Member

encukou commented Mar 11, 2022

Is it feasible to move our source PEP files into a subdirectory?

Please don't. The text in this repo isn't about the project in the repo, it is the project.

@CAM-Gerlach
Copy link
Member

@hugovk see #2405 for a general-case fix.

I think the big difference here to "normal" Sphinx docs builds is we're reading the source documentation files from the root, and trying to ignore certain support and any other random user files.

The other even bigger difference, that is more directly the problem in this case, is that for legacy reasons, the older PEPs have the txt extension, and with all the content files in the repo root, without some clever exclude_patterns (added in #2405), there's no easy way for Sphinx to tell what it should consider content and what it should ignore without hardcoding everything.

Please don't. The text in this repo isn't about the project in the repo, it is the project.

To be clear, given the GitHub UI limitations and general churn I don't believe its worth changing now (when, at least for this problem, a relatively straightforward fix is available).

However, for what it's worth, I don't really understand this reasoning. In almost any sort of medium to large project, source content goes in a subdir—for Python projects, src/projectname (or just projectname, with the attended downsides for autodiscovering, testing, etc of the implicit layout); for docs repos, in a docs or similar subdir; for static sites, source content typically goes in a content or similar subdir, etc.

On this repo in particular, it harms UX, on GitHub and locally, since page after page of PEPs have to be scrolled through to read the Readme on GitHub, or get from the files that have names before p to those after, and it makes it very difficult for anyone not intimately familiar with everything on the repo to know what files are infra, reademe-type documents, etc., and which are files related to PEP content—there are a couple that despite tracking down the history, I'm still not 100% sure. Not to mention, it leads to tooling problems like this.

@ncoghlan
Copy link
Contributor

I think the issue I'm seeing trying to build locally is this one (rather than #2402), since the linting works fine, but the rendering is complaining about old files inside the .git folder:

... snip ...
checking consistency... /home/ncoghlan/devel/peps/.git/logs/refs/heads/augmented-assignment-expressions.rst: WARNING: document isn't included in any toctree
/home/ncoghlan/devel/peps/.git/logs/refs/remotes/pr/augmented-assignment-expressions.rst: WARNING: document isn't included in any toctree
/home/ncoghlan/devel/peps/.git/refs/heads/augmented-assignment-expressions.rst: WARNING: document isn't included in any toctree
/home/ncoghlan/devel/peps/.git/refs/remotes/pr/augmented-assignment-expressions.rst: WARNING: document isn't included in any toctree
/home/ncoghlan/devel/peps/docs/build.rst: WARNING: document isn't included in any toctree
/home/ncoghlan/devel/peps/docs/rendering_system.rst: WARNING: document isn't included in any toctree
/home/ncoghlan/devel/peps/pep-0593.rst:265: WARNING: Citation [struct-doc] is not referenced.
/home/ncoghlan/devel/peps/pep-0553.rst:283: WARNING: Citation [envar] is not referenced.
/home/ncoghlan/devel/peps/pep-0554.rst:1645: WARNING: Citation [CSP] is not referenced.
/home/ncoghlan/devel/peps/pep-0554.rst:1677: WARNING: Citation [mp-conn] is not referenced.
/home/ncoghlan/devel/peps/pep-0554.rst:1686: WARNING: Citation [main-thread] is not referenced.
/home/ncoghlan/devel/peps/pep-0554.rst:1709: WARNING: Citation [nathaniel-asyncio] is not referenced.
/home/ncoghlan/devel/peps/pep-0554.rst:1712: WARNING: Citation [extension-docs] is not referenced.
/home/ncoghlan/devel/peps/pep-0520.txt:416: WARNING: Citation [nick_concern] is not referenced.
/home/ncoghlan/devel/peps/pep-0520.txt:419: WARNING: Citation [orig] is not referenced.
/home/ncoghlan/devel/peps/pep-0520.txt:422: WARNING: Citation [followup1] is not referenced.
/home/ncoghlan/devel/peps/pep-0520.txt:425: WARNING: Citation [followup2] is not referenced.
/home/ncoghlan/devel/peps/pep-0484.txt:2490: WARNING: Citation [peps] is not referenced.
done
preparing documents... done
Traceback (most recent call last):
  File "/home/ncoghlan/devel/peps/build.py", line 86, in <module>
    app.build()
  File "/home/ncoghlan/devel/peps/.venv/lib64/python3.10/site-packages/sphinx/application.py", line 337, in build
    self.builder.build_update()
  File "/home/ncoghlan/devel/peps/.venv/lib64/python3.10/site-packages/sphinx/builders/__init__.py", line 294, in build_update
    self.build(to_build,
  File "/home/ncoghlan/devel/peps/.venv/lib64/python3.10/site-packages/sphinx/builders/__init__.py", line 358, in build
    self.write(docnames, list(updated_docnames), method)
  File "/home/ncoghlan/devel/peps/.venv/lib64/python3.10/site-packages/sphinx/builders/__init__.py", line 529, in write
    self._write_parallel(sorted(docnames),
  File "/home/ncoghlan/devel/peps/.venv/lib64/python3.10/site-packages/sphinx/builders/__init__.py", line 556, in _write_parallel
    self.write_doc(firstname, doctree)
  File "/home/ncoghlan/devel/peps/.venv/lib64/python3.10/site-packages/sphinx/builders/html/__init__.py", line 631, in write_doc
    ctx = self.get_doc_context(docname, body, metatags)
  File "/home/ncoghlan/devel/peps/pep_sphinx_extensions/pep_processor/html/pep_html_builder.py", line 39, in get_doc_context
    if len(toc_tree[0]) > 1:
  File "/home/ncoghlan/devel/peps/.venv/lib64/python3.10/site-packages/docutils/nodes.py", line 654, in __getitem__
    return self.children[key]
IndexError: list index out of range
make: *** [Makefile:9: render] Error 1

I'll try the #2415 branch to see if that resolves the issue.

@ncoghlan
Copy link
Contributor

Confirmed - if I check out #2415 and merge in the current main branch (to pick up the various #2402 fixes), then local Sphinx rendering starts working for me (as it isn't picking up the old PEP drafts in the .git history).

@hugovk
Copy link
Member

hugovk commented Mar 12, 2022

Thanks for testing!

Whichever of #2405 and #2415 we go for (I'm leaning towards #2405), let's explicitly exclude .git. Please see PR #2423.


I'll also repeat this question, my original typo made no sense :)

Can/should Sphinx be made to ignore non-SVN non-CVS non-source-controlled files? (Edit: fixed typo!)

@AA-Turner
Copy link
Member

Can/should Sphinx be made to ignore non-source-controlled files?

Maybe. A lot more work though. Also two approaches (files tracked by git, or files not in .gitignore). I think for the initial implementation here we shouldn't do so.

A

@CAM-Gerlach
Copy link
Member

Whichever of #2405 and #2415 we go for (I'm leaning towards #2405), let's explicitly exclude .git. Please see PR #2423.

See comments there; in short, this is only needed if we really want a very short-term stopgap until one of those PRs is merged.

Maybe. A lot more work though. Also two approaches (files tracked by git, or files not in .gitignore). I think for the initial implementation here we shouldn't do so.

This does seem like a lot more work, even more so than #2415 , which solves the issue much more precisely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug infra Core infrastructure for building and rendering PEPs
Projects
None yet
6 participants