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

Should defusedxml be moved to the runtime dependencies? #12339

Closed
befeleme opened this issue Apr 29, 2024 · 5 comments · Fixed by #12362 · May be fixed by #12341
Closed

Should defusedxml be moved to the runtime dependencies? #12339

befeleme opened this issue Apr 29, 2024 · 5 comments · Fixed by #12362 · May be fixed by #12341
Labels

Comments

@befeleme
Copy link
Contributor

Describe the bug

In pyproject.toml, defusedxml is defined as one of the test dependencies: https://github.com/sphinx-doc/sphinx/blob/master/pyproject.toml#L97
However, in Sphinx 7.3.x its import was added to sphinx/testing/util.py (885818b). Many sphinx plugins use that code in their test suites and break.
Should it be moved to the list of runtime dependencies?

How to Reproduce

(venv)$ pip install sphinx
(venv)$ git clone git@github.com:sphinx-doc/sphinxcontrib-devhelp.git && cd sphinxcontrib-devhelp
(venv)$ tox
 <snip>
  File "~/sphinxcontrib-devhelp/.tox/py313/lib/python3.13/site-packages/sphinx/testing/util.py", line 14, in <module>
    from defusedxml.ElementTree import parse as xml_parse
ImportError: Error importing plugin "sphinx.testing.fixtures": No module named 'defusedxml'

Environment Information

sphinx 7.3.x

Sphinx extensions

No response

Additional context

No response

@jdillard
Copy link
Contributor

I asked a similar question here: https://github.com/orgs/sphinx-doc/discussions/12333

Happy to close the discussion if this is a better place.

@picnixz

This comment was marked as outdated.

jonathansick added a commit to lsst-sqre/documenteer that referenced this issue Apr 30, 2024
jonathansick added a commit to lsst-sqre/documenteer that referenced this issue Apr 30, 2024
@picnixz
Copy link
Member

picnixz commented May 1, 2024

Ok, I see what happened actually. It's because the dependencies of sphinxcontrib- were not updated accordingly, namely we are installing sphinx as sphinx (so we are only using the release dependencies and not the test dependencies for sphinx).

Is there a way to tell tox to install the sphinx test dependencies in tox.ini?

PS: The argument against adding defusedxml as a runtime dependency is that, for consistency, we would also need to add pytest as a runtime dependency since we are using pytest in sphinx.testing.fixture. One could argue that because one should have pytest for using the plugin, we don't need that but technically speaking, both of them should then be present.

Now, I am willing to actually split the test dependencies into dev and test dependencies where dev dependencies would be those needed by extensions and test dependencies would only be for our tests (in particular, test would be a superset of dev, e.g., this would include cython and setuptools packages).

@chrisjsewell
Copy link
Member

chrisjsewell commented May 1, 2024

Now, I am willing to actually split the test dependencies into dev and test

yes I feel this is what should happen; I was about to say, your first responses seemed to be conflating sphinx internal development with the actual opened issue of users interacting with sphinx.testing.fixture 😅

(in general, it is a bit of an open question in python packaging, where/how you define internal development dependencies vs defining extra user dependencies)

@mgorny
Copy link
Contributor

mgorny commented May 8, 2024

I think it would also be useful to avoid importing defusedxml in global scope. It is only used by etree_parse(), and not every consumer of sphinx.testing uses this function.

For example, breathe is currently broken by this. However, it does not use this API and without the import in global scope, it doesn't have to be updated to depend on defusedxml (or sphinx[dev], as suggested above).

mgorny added a commit to mgorny/sphinx that referenced this issue May 8, 2024
Import `defusedxml` inside the `etree_parse()` function rather than
in global scope of `sphinx.testing.util`.  This makes it possible
for reverse dependencies (such as `breathe`) to use this module without
adding an unnecessary transitive dependency on `defusedxml` when it's
not actually used.

See also issue sphinx-doc#12339.
hoodmane added a commit to hoodmane/sphinx-click that referenced this issue May 9, 2024
Otherwise tests fail with:
`ImportError: Error importing plugin "sphinx.testing.fixtures": No module named 'defusedxml'`.

See:
sphinx-doc/sphinx#12339
@chrisjsewell chrisjsewell linked a pull request May 11, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants