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

Use tags in docs building to split up requirements #13168

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matthewhughes934
Copy link
Contributor

The goal is to allow a build of just the man pages with a minimal set of dependencies, like:

sphinx-build \
    --tag man \
    -c docs/html \
    -d docs/build/doctrees/man \
    -b man \
    docs/man \
    docs/build/man

This is for the sake of people distributing pip who want to build the man pages (but not the HTML ones) along side the application.

This is an alternative to the approach proposed in[1] and similarly addresses[2]

Link: #12900 [1]
Link: #12881 [2]

@matthewhughes934
Copy link
Contributor Author

matthewhughes934 commented Jan 15, 2025

🤔 I need to tell readthedocs about --tag html for it to correctly build

EDIT: quick hack e06b1cf, I don't see any out of the box support for them (I found one issue: readthedocs/readthedocs.org#4603)

]

# 'tags' is a special object handled by sphinx
# https://www.sphinx-doc.org/en/master/usage/configuration.html#project-tags
if "html" in tags or "READTHEDOCS" in os.environ: # type: ignore[name-defined] # noqa: F821
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linters get a bit upset with using tags like this, and complain about each nice, but maybe more usage makes things more clear, e.g.:

_tags: Tags = tags  # type: ignore[name-defined] # noqa: F821
if "READTHEDOCS" in os.environ:
    _tags.add("html")
if "html" in _tags:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've also seen (and commented on) some tag use by @AA-Turner that was accessing them through the Sphinx application object in setup(). Adam — do you remember of such an example? I can't seem to find a project that uses said approach…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it! I was thinking about those projects linked in sphinx-doc/sphinx#12490. Looks like the tags are accessible via app.tags.tags in the extension context...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The object is injected into conf.py. Please only use tags or app.tags, we have deprecated app.tags.tags!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know it's injected. I was just thinking about the postponed evaluation case. So I'd go for calling app.setup_extension() based on the state of app.tags or the currently selected builder…

@ichard26
Copy link
Member

@eli-schwartz would gating the HTML build behind a Sphinx tag --tag html work for you? Presumably, you can modify the sphinx-build invocation to pass custom flags, but I don't want to assume!

@eli-schwartz
Copy link
Contributor

This is a neat trick that I didn't know sphinx could do!

Presumably, you can modify the sphinx-build invocation to pass custom flags, but I don't want to assume!

We can, yes. It's already necessary in order to pass -c to override the conf.py directory.

That being said, it looks like this actually defaults to omitting the html logic by default (it doesn't matter whether you use --tag man) and only doing something different when building html. Was that the intention? You could alternatively do if "man" not in tags to require people to manually opt in to this by passing --tag man. I'm not sure how much it matters since noxfile.py will always do the correct invocation and anyone manually running sphinx-build is presumably in "you must check how noxfile.py did it and ensure your usage is aligned" mode.

]

# 'tags' is a special object handled by sphinx
# https://www.sphinx-doc.org/en/master/usage/configuration.html#project-tags
if "html" in tags or "READTHEDOCS" in os.environ: # type: ignore[name-defined] # noqa: F821
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think the idiomatic check is tags.has('html').

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that it's not just html but htmldir too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think the idiomatic check is tags.has('html').

I was just following the docs on that one https://www.sphinx-doc.org/en/master/usage/configuration.html#project-tags (look to be a recent update <1 year ago sphinx-doc/sphinx#12490)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense.

# https://www.sphinx-doc.org/en/master/usage/configuration.html#project-tags
if "html" in tags or "READTHEDOCS" in os.environ: # type: ignore[name-defined] # noqa: F821
# html specific deps
extensions.extend(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurred to me that maybe if the extension loading was to be moved to the setup() entry point, the builder name would already be known by then and so this might not even have to use tags but would rely on the requested builders.

Another approach I used in the past, in a little different context, was scanning for the builder args in sys.argv: sphinx-contrib/spelling#55 (comment) / cherrypy/cheroot@63097fb#diff-85933aa74a2d66c3e4dcdf7a9ad8397f5a7971080d34ef1108296a7c6b69e7e3R12-R15.

@webknjaz
Copy link
Member

🤔 I need to tell readthedocs about --tag html for it to correctly build

That's one of the reasons I've migrated my RTD configs to just call the project's main workflow tool: https://github.com/ansible/awx-plugins/blob/0d569b5/.readthedocs.yaml#L27-L29. In my case it's tox, but in pip's case it'd be nox. This brings RTD and local+CI builds even closer in what they produce.

noxfile.py Show resolved Hide resolved
"sphinx.ext.extlinks",
"sphinx.ext.intersphinx",
# our extensions
# extensions common to 'man' and 'html' builds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also recommend keeping the order of extensions: first-party -> third-party -> local. Just like with regular imports.

Comment on lines 17 to 20
# first-party extensions
"sphinx.ext.autodoc",
"sphinx.ext.todo",
"sphinx.ext.extlinks",
"sphinx.ext.intersphinx",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all built-in so there's no value moving to conditional (and it makes things harder to reason about)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there might be value in skipping some code paths, though.

"sphinx_copybutton",
"sphinx_inline_tabs",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two seem to be the only genuine 'HTML-only' extensions. Perhaps the condition might be better expressed as 'man' not in tags?

You could also look at the Python docs, where we use a pattern to check if an extension is importable, and if so add it to extensions. Eg

import importlib.util
if importlib.util.find_spec(...):
    extensions.append(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the condition might be better expressed as 'man' not in tags?

👍 this was also suggested above (#13168 (comment)) and I agree it makes this clearer (and simplifies the code!) so trying that: 033c31c

]

# 'tags' is a special object handled by sphinx
# https://www.sphinx-doc.org/en/master/usage/configuration.html#project-tags
if "html" in tags or "READTHEDOCS" in os.environ: # type: ignore[name-defined] # noqa: F821

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The object is injected into conf.py. Please only use tags or app.tags, we have deprecated app.tags.tags!

@matthewhughes934 matthewhughes934 force-pushed the experiment-tags-for-split-builds branch from 82da058 to 033c31c Compare January 16, 2025 19:23
Comment on lines 22 to 37
if "man" not in tags: # type: ignore[name-defined] # noqa: F821
# extensions not needed for building man pages
extensions.extend(
(
# first-party extensions
"sphinx.ext.autodoc",
"sphinx.ext.todo",
"sphinx.ext.extlinks",
"sphinx.ext.intersphinx",
# third-party extensions
"myst_parser",
"sphinx_copybutton",
"sphinx_inline_tabs",
"sphinxcontrib.towncrier",
),
)
Copy link
Member

@webknjaz webknjaz Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be cleaner?

Suggested change
if "man" not in tags: # type: ignore[name-defined] # noqa: F821
# extensions not needed for building man pages
extensions.extend(
(
# first-party extensions
"sphinx.ext.autodoc",
"sphinx.ext.todo",
"sphinx.ext.extlinks",
"sphinx.ext.intersphinx",
# third-party extensions
"myst_parser",
"sphinx_copybutton",
"sphinx_inline_tabs",
"sphinxcontrib.towncrier",
),
)
IS_MAN_BUILD = "man" in tags # type: ignore[name-defined] # noqa: F821
extensions += [] if IS_MAN_BUILD else [
# first-party extensions
"sphinx.ext.autodoc",
"sphinx.ext.todo",
"sphinx.ext.extlinks",
"sphinx.ext.intersphinx",
# third-party extensions
"myst_parser",
"sphinx_copybutton",
"sphinx_inline_tabs",
"sphinxcontrib.towncrier",
]

@ichard26 ichard26 requested a review from pradyunsg January 18, 2025 20:06
Copy link
Member

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'll wait for other people to comment, however.

@ichard26
Copy link
Member

Oh yeah, we should probably add a docs news entry to let our redistributors know about this change.

The goal is to allow a build of just the `man` pages with a minimal set
of dependencies, like:

    sphinx-build \
        --tag man \
        -c docs/html \
        -d docs/build/doctrees/man \
        -b man \
        docs/man \
        docs/build/man

This is for the sake of people distributing `pip` who want to build the
man pages (but not the HTML ones) along side the application.

This is an alternative to the approach proposed in[1] and similarly
addresses[2]

Link: pypa#12900 [1]
Link: pypa#12881 [2]
@matthewhughes934
Copy link
Contributor Author

Oh yeah, we should probably add a docs news entry to let our redistributors know about this change.

Done, and squashed my fixup commits in with 782c3a2

There have been some good suggestions on this PR about large restructurings of the conf.py that could be useful, but given I'm no sphinx expert I decided to lean on the side of keeping this PR small/simple. Maybe it's wroth a separate issue to discuss some broader improves that could simplify the whole setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants