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

Sphinx 7.2 breaks generated documentation headers #11618

Closed
oddbookworm opened this issue Aug 18, 2023 · 17 comments · Fixed by #11647
Closed

Sphinx 7.2 breaks generated documentation headers #11618

oddbookworm opened this issue Aug 18, 2023 · 17 comments · Fixed by #11647

Comments

@oddbookworm
Copy link

oddbookworm commented Aug 18, 2023

Describe the bug

Updating to sphinx 7.2.0 (or higher, including 7.2.2) breaks our documentation headers at pygame-ce. As an example, here's the current doc page for pygame-ce's draw module, and here's what the doc looks like when I generate locally with 7.2.0 (7.1.2 produces what's on github)

/* Auto generated file: with makeref.py .  Docs go in docs/reST/ref/ . */
#define DOC_DRAW_RECT "rect(surface, color, rect) -> Rect\nrect(surface, color, rect, width=0, border_radius=0, border_top_left_radius=-1, border_top_right_radius=-1, border_bottom_left_radius=-1, border_bottom_right_radius=-1) -> Rect\ndraw a rectangle"

The reST source for that doc is here and here is our makeref.py

How to Reproduce

Create a pygame-ce dev environment (our github wiki has instructions)
Install sphinx 7.2-7.2.2 (all of these versions give broken doc headers)
python setup.py docs --fullgeneration
python setup.py install
You should see an error about undefined references to DOC_****
Install sphinx 7.1.2 (or earlier) and try again. Should work

Environment Information

Platform:              linux; (Linux-6.1.44-1-MANJARO-x86_64-with-glibc2.38)
Python version:        3.11.3 (main, Jun  5 2023, 09:32:32) [GCC 13.1.1 20230429])
Python implementation: CPython
Sphinx version:        7.2.2
Docutils version:      0.20.1
Jinja2 version:        3.1.2
Pygments version:      2.15.1

This is from my local environment, but we also see the issue in our CI runs for MacOS, Windows, Debian/Ubuntu, and ManyLinux

Sphinx extensions

No response

Additional context

No response

@oddbookworm oddbookworm changed the title Sphinx 7.2 Sphinx 7.2 breaks generated documentation headers Aug 18, 2023
@AA-Turner
Copy link
Member

Hi Andrew (@oddbookworm),

Is there a smaller reproducer at all than setting up an entire pygame environment please? Ideally a stand-alone Sphinx project, but in effect the fewer moving parts the better!

Thanks,
Adam

@oddbookworm
Copy link
Author

Uh, I don't know enough about how it works to do that myself. Lemme ask around to see if someone else knows enough about our workflow to sort that out

@dr0id
Copy link

dr0id commented Aug 18, 2023

this is on windows:
image
it looks like somewhere this is joined without using os.path.join or similar (left 7.1.2, right 7.2.2)

But this is probably not the problem. I have two verbose logs, I can't see any crucial difference in it. I would guess that somewhere the processing of :mod: or module:: is broken.

@ankith26
Copy link

I did a bit of digging, and noticed that some change in the doctree attribute of the html-page-context event breaks our usage. I'm not sure how much this info helps, but I'll dig further and try to isolate the issue into a minimum reproducer later

@ankith26
Copy link

ankith26 commented Aug 19, 2023

Though I'm not familiar with the sphinx codebase, to me it looks like a regression from #11533
EDIT: ignore this comment, I misread something

@picnixz
Copy link
Member

picnixz commented Aug 20, 2023

@oddbookworm If you are not able to find a minimal reproducer, can you bisect the culprit commit so that we may have an idea of what has been introduced / removed that may have lead to this issue? Thank you.

@oddbookworm
Copy link
Author

@oddbookworm If you are not able to find a minimal reproducer, can you bisect the culprit commit so that we may have an idea of what has been introduced / removed that may have lead to this issue? Thank you.

I was able to do that.
05a14ff -> works fine
97d2c5d -> broken

@AA-Turner
Copy link
Member

This is the last blocker for 7.2.3.

A

@AA-Turner
Copy link
Member

The issue is caused by these two lines--swapping the order fixes things. I'm continuting to investigate for a proper fix.

# The node order is: index node first, then target node.
ret.append(inode)
ret.append(target)

@AA-Turner AA-Turner linked a pull request Aug 26, 2023 that will close this issue
@AA-Turner
Copy link
Member

See #11647, which should fix this issue.

@AA-Turner
Copy link
Member

AA-Turner commented Aug 26, 2023

@oddbookworm / @dr0id / @ankith26: please may you test with Sphinx master?: python -m pip install "Sphinx @ git+https://github.com/sphinx-doc/sphinx"

A

@ankith26
Copy link

Doesn't fix the issue for me.

I didn't come up with a minimal reproducer yet, but in the process of testing things out I did find the source of the issue, though I don't understand it well enough.

So basically we have a custom subclass of docutils.nodes.SparseNodeVisitor that we use in doctree.walkabout (triggered from doctree-read event), and this subclass implements a custom depart_section which does something like if node["ids"][0].startswith("module-"): ... but in the versions that broke our usage, no ids seem to start with module-

@AA-Turner AA-Turner reopened this Aug 26, 2023
@AA-Turner
Copy link
Member

@ankith26 please may you try re-creating your environment from scratch? python buildconfig/makeref.py full_generation produces identical header files when I run the command locally.

@ankith26
Copy link

Oh I'm sorry. I don't know what went wrong when I wrote my previous comment, but I just tried again and yes the issue is indeed fixed.

Thanks!

@AA-Turner
Copy link
Member

Wonderful! Thank you for reporting, this uncovered a very subtle bug that I otherwise would've missed.

A

@dr0id
Copy link

dr0id commented Aug 27, 2023

Thanks for the bugfix, I can confirm if works again.

@AA-Turner
Copy link
Member

AA-Turner commented Aug 28, 2023

Sphinx 7.2.4 has been released with the fix.

A

oddbookworm added a commit to oddbookworm/pygame-ce that referenced this issue Aug 30, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants