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

C coverage support in sphinx.ext.coverage is non-functional #11590

Closed
stephenfin opened this issue Aug 15, 2023 · 5 comments
Closed

C coverage support in sphinx.ext.coverage is non-functional #11590

stephenfin opened this issue Aug 15, 2023 · 5 comments

Comments

@stephenfin
Copy link
Contributor

stephenfin commented Aug 15, 2023

Describe the bug

The sphinx.ext.coverage extension supports checking coverage for both the Python and C domain. However, the latter does not appear to work. The extension checks the self.env.domaindata['c']['objects'] but unlike in the Python domain, this attribute is never set. As a result, every API will show as undocumented. My git splunking suggests this has been the case for many years and there's a chance this has never worked, which probably explains why the only usage of the various C-specific configuration options I can find on GitHub are in CPython (an attempted user), Sphinx (the definition) or a fork of one of these two projects.

How to Reproduce

To reproduce this you need a C application. I have packaged a minimal example, which you can find here:

https://github.com/stephenfin/sphinx_ext_coverage_test

Instructions are provided there on how to use this repo to reproduce.

Environment Information

Platform:              linux; (Linux-6.4.8-200.fc38.x86_64-x86_64-with-glibc2.37)
Python version:        3.11.4 (main, Jun  7 2023, 00:00:00) [GCC 13.1.1 20230511 (Red Hat 13.1.1-2)])
Python implementation: CPython
Sphinx version:        7.2.0+/2f9d39ce8
Docutils version:      0.20.1
Jinja2 version:        3.1.2
Pygments version:      2.16.1

Sphinx extensions

extensions = [
    'sphinx.ext.coverage',
]

Additional context

No response

@stephenfin
Copy link
Contributor Author

stephenfin commented Aug 15, 2023

I mentioned this in the PR to fix this, #11591, but I wonder if we want to deprecate the C coverage feature entirely instead? Nobody appears to actually be using it, as evidenced by the fact that no one complained about it being broken, and compared to the Python coverage feature the C one is super janky. We don't have any of the introspection capabilities for C that we have with Python and we also don't have the ability to parse C beyond the limited AST subset we've implemented to parse things like function definitions (the fact that we don't insist on the code being preprocessed to resolve macros etc. hamstrings us also). Because we're missing all these things, it means the user needs to "roll their own" for each and every project and provide a rather convoluted list of configuration options including the list of directories to find code (most likely header files) in, regexes that Sphinx can use to "parse" said code and differentiate between functions, macros, etc. (and because there's no requirement to preprocess, the user needs to take these into account also), additional regexes to ignore certain objects etc. It's a real mess IMO.

If we wanted to properly do this, I'd like to incorporate e.g. https://github.com/eliben/pycparser (which, note, does insist of preprocessing the code first) so that we could truly "parse" the C code, but that doesn't feel like something that should live in Sphinx core. Also, we have Doxygen and a variety of documentation coverage tools that we could simply push users towards, such as https://github.com/psycofdj/coverxygen.

What are peoples' thoughts on just dumping this whole aspect of the coverage tool and focusing on Python coverage only (at least within core Sphinx)?

@picnixz
Copy link
Member

picnixz commented Aug 15, 2023

It's funny to see that 1) the feature seemed buggy for a long time 2) no one complained. Instead of trying to reinvent the wheel by incorporating the C coverage in Sphinx, I think it's indeed better to just tell users to use another tool.

(Also, less features means less possible issues and less work)

@AA-Turner
Copy link
Member

Techically we'd need to remove this in 8.0 or 9.0. Perhaps we could adapt the coverage builder to be language-agnostic, and offer the C parts as a first party extension?

A

@stephenfin
Copy link
Contributor Author

Techically we'd need to remove this in 8.0 or 9.0.

Yes, to be clear I am suggesting deprecating for removal (hard deprecation). Removal wouldn't happen until a major version bump as per our usual deprecation policies.

Perhaps we could adapt the coverage builder to be language-agnostic, and offer the C parts as a first party extension?

We could but until someone comes asking for this and offers to do the work I'd be inclined to deprecate and remove. Again, it appears no one is using this functionality and I only discovered this while trying to add docs for the extension.

stephenfin added a commit to stephenfin/sphinx that referenced this issue Aug 15, 2023
Signed-off-by: Stephen Finucane <stephen@that.guru>
@stephenfin
Copy link
Contributor Author

Fixed in #11591

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants