Skip to content

conf: switch breathe to import into cpp domain#207

Merged
deb-intel merged 1 commit intothesofproject:masterfrom
mmaka1:breathe-to-cpp
Apr 2, 2020
Merged

conf: switch breathe to import into cpp domain#207
deb-intel merged 1 commit intothesofproject:masterfrom
mmaka1:breathe-to-cpp

Conversation

@mmaka1
Copy link
Contributor

@mmaka1 mmaka1 commented Mar 25, 2020

Cpp domain provides much better formatting in Sphinx.
Initialized enum items are correctly displayed, pointer
to functions typedef-ed in place also works fine.

Other projects also tend to use cpp domain for import
from C headers.

Signed-off-by: Marcin Maka marcin.maka@linux.intel.com

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 26, 2020

The page https://breathe.readthedocs.io/en/latest/directives.html says:

If you wanted all .h header files to be treated as being in the cpp domain you might use:

breathe_domain_by_extension = {
        "h" : "cpp",
        }

This commit does something very different... care to elaborate?

@mmaka1
Copy link
Contributor Author

mmaka1 commented Mar 26, 2020

@marc-hb With this commit, breathe stops enforcing C and this seems to be enough for consistent import. Running breathe with different options I found that it imports everything into cpp domain w/o extra options, cpp seems to be the default one, and the output is correct (see also breathe-doc/breathe#282 (comment)). Note that doxygen always outputs <compounddef id="component_8h" kind="file" language="C++">. The doxygen is already configured with OPTIMIZE_OUTPUT_FOR_C = YES and adding EXTENSION_MAPPING = h=C does not make any difference too. Not sure whether this contributes to messy output as well, or the import to c domain, when forced, is broken somewhere else.

@marc-hb
Copy link
Collaborator

marc-hb commented Mar 26, 2020

cpp seems to be the default one

I think this wouldn't hurt in the commit message.

To be honest I think you could just copy your entire last comment in the commit message.

PS: breathe seems unfortunately unmaintained breathe-doc/breathe#444 (comment)

Cpp domain provides much better formatting in Sphinx.
Initialized enum items are correctly displayed, pointer to functions
typedef-ed in place also works fine.

Other projects also tend to use cpp domain for import from C headers.

With this commit, breathe stops enforcing C and this seems to be enough for
consistent import. Running breathe with different options I found that it
imports everything into cpp domain w/o extra options, cpp seems to be the
default one, and the output is correct (see also breathe-doc/breathe#282
(comment)). Note that doxygen always outputs <compounddef id="component_8h"
kind="file" language="C++">. The doxygen is already configured with
OPTIMIZE_OUTPUT_FOR_C = YES and adding EXTENSION_MAPPING = h=C does not
make any difference too.

Signed-off-by: Marcin Maka <marcin.maka@linux.intel.com>
@mmaka1
Copy link
Contributor Author

mmaka1 commented Mar 27, 2020

@marc-hb Sure, the explanation pasted into the commit message.

@deb-intel deb-intel merged commit fe088f0 into thesofproject:master Apr 2, 2020
@marc-hb
Copy link
Collaborator

marc-hb commented Apr 3, 2020

I had to revert this locally because it causes my sphinx+breathe versions to fail like this:

sphinx-build -t development -b html -d _build/doctrees . _build/html  
Running Sphinx v2.1.2
/usr/lib/python3.7/site-packages/sphinx/util/docutils.py:311: RemovedInSphinx30Warning: function based directive support is now deprecated. Use class based directive instead.
  RemovedInSphinx30Warning)
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 109 source files that are out of date
updating environment: 109 added, 0 changed, 0 removed
reading sources... [  3%] api/audio-stream-api                                                                          
Exception occurred:
  File "~/.local/lib/python3.7/site-packages/breathe/renderer/sphinxrenderer.py", line 38, in parse_definition
    ast = parser.parse_declaration("class")
TypeError: parse_declaration() missing 1 required positional argument: 'directiveType'
The full traceback has been saved in /tmp/sphinx-err-mecp16th.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!

This is the small bit of the long traceback in /tmp/sphinx-err-mecp16th.log that gave me the clue to revert this:

  File "~/.local/lib/python3.7/site-packages/breathe/renderer/sphinxrenderer.py", line 353, in run_domain_>
    nodes = domain_directive.run()
  File "/usr/lib/python3.7/site-packages/sphinx/domains/cpp.py", line 6618, in run
    return super().run()
  File "/usr/lib/python3.7/site-packages/sphinx/directives/__init__.py", line 182, in run
    name = self.handle_signature(sig, signode)

Last time I tried to debug breathe I got totally lost in the many layers of OO (Object Obfuscation) and I already spent too many hours finding this particular workaround so I won't investigate further. Who knows, maybe some upgrade will fix it (not a breathe upgrade as it's not maintained anymore)

I'll keep carrying this revert locally, mentioning it here just for the record and in case it can help someone else.

@intelkevinputnam
Copy link
Collaborator

This should be fixed by running: pip3 install -r requirements.txt

Looks like someone updated the dependencies.

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 4, 2020

Looks like someone updated the dependencies.

We relaxed the requirements in commit 0097633 >= instead of =

This should be fixed by running: pip3 install -r requirements.txt

Almost! I downgraded sphinx and the failure goes away. Then I tried a few different versions in https://pypi.org/project/Sphinx/#history and I found the issue starts happening with pip3 install --user sphinx==2.0.0b1, sphinx version 1.8.5 works fine. Maybe breathe is not compatible with sphinx versions 2 and above? We could change requirements.txt to something like >=1.74,<2.0

(Another workaround is not to use doxygen at all, see PR #212 for more background)

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 4, 2020

Doh! After googling the error I discovered I was running an old version of breathe (4.11) for some unknown reason. Upgraded to 4.14.1 and that error is gone with sphinx 2.1.2, sorry for the noise.

breathe-doc/breathe#411 Adapt to upcoming Sphinx 2.0

@mmaka1 mmaka1 deleted the breathe-to-cpp branch April 8, 2020 14:10
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.

5 participants