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

feat: Allow additional tools for sphinx_docs #1831

Merged

Conversation

castler
Copy link
Contributor

@castler castler commented Apr 3, 2024

Some Sphinx plugins require that other (not necessarily Python) tools are available.

One example is the plugin https://github.com/basejumpa/sphinxcontrib-umlet, which requires that UMLet is somehow within the sandbox.

  • Adds tools arg to sphinx_docs to allow passing in arbitrary tools
    that are made available at runtime
  • Performs location expansion on extra_opts, which allows passing the
    location of the tools onto sphinx.

@castler castler requested a review from rickeylev as a code owner April 3, 2024 14:44
@aignas
Copy link
Collaborator

aignas commented Apr 4, 2024

Since sphinx is enabled in the next release Ithink this does not need an extrachngalog entry, but the docstring for the tools could include a short example with a particular plugin. Why the opts need expansion is still a little unclear to me. Is it that some plugins may expect the tools to be found in the PATH and that case would not be covered. To be honest, I am not super familiar with sphinx and these questions could be just my ignorance.

@castler
Copy link
Contributor Author

castler commented Apr 4, 2024

. Why the opts need expansion is still a little unclear to me. Is it that some plugins may expect the tools to be found in the PATH and that case would not be covered.

Sure, something like this?

sphinx_docs(
    ...
    name = ...,
    srcs = ...,
    extra_opts = [
        "-Dsome_binary=$(execpath //:some_binary_target)",
    ],
    tools = [
        "//:some_binary_target",
    ],
)

Copy link
Contributor

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Mostly lgtm, just need the extra_opts doc updated (I couldn't suggest an edit since it wasn't a modified line)

sphinxdocs/private/sphinx.bzl Show resolved Hide resolved
sphinxdocs/private/sphinx.bzl Show resolved Hide resolved
@castler castler force-pushed the sphinx_docs_allow_additional_tools branch from 07fbcc9 to a76a05c Compare April 5, 2024 04:13
@castler
Copy link
Contributor Author

castler commented Apr 5, 2024

Mostly lgtm, just need the extra_opts doc updated (I couldn't suggest an edit since it wasn't a modified line)

Thanks, applied requested changes.

When using some sphinx plugins they require that other (not necessarily
Python) tools are available.

One example is the plugin https://github.com/basejumpa/sphinxcontrib-umlet
which requires that UMLet is somehow within the sandbox.

My understanding is that the right way todo so is the following:

1st: Provide a way how additional tools can be passed to the action
-> Done in this commit via the exposed tools attribute

2nd: Allow a way to configure the path to the tool
-> Done with this commit via the ctx.expand_location for additional
attributes

Both changes should be backward compatible and enable a wider usage of
the Sphinx rules.
@rickeylev rickeylev added this pull request to the merge queue Apr 5, 2024
@rickeylev rickeylev removed this pull request from the merge queue due to a manual request Apr 5, 2024
@rickeylev rickeylev added this pull request to the merge queue Apr 5, 2024
Merged via the queue into bazelbuild:main with commit 9e38b65 Apr 5, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants