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

Sphinx 3 update #491

Merged
merged 11 commits into from Apr 7, 2020
Merged

Conversation

jakobandersen
Copy link
Collaborator

Updates to Sphinx 3 and improvements of documentation/tests:

  • The usual API fixes, and removal of Sphinx 2 stuff (only the newest version is supported).
  • Remove Python 2 related things, and bump to Python 3 (Python 2 is no longer supported, and Sphinx requires Python 3 anyway).
  • Adapt C constructs to the newly rewritten C domain, and add some specific tests (in the documentation specific.rst).
  • Fix use of breathe_domain_by_file_pattern in the documentation: only filename globbing is possible, not path globbing.
  • Update documentation to avoid duplicate names by putting each example in it's own (anon) namespace.

@jakobandersen
Copy link
Collaborator Author

(Fixes #490)

documentation/Makefile Outdated Show resolved Hide resolved
@vermeeren
Copy link
Collaborator

Everything else seems good to me, thanks as always for the patches Jakob.

@vermeeren vermeeren added analysis Quality assurance, CI test Unit tests, acceptance tests labels Apr 6, 2020
Copy link

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

This looks great, thanks so much! Only one small thing I noticed.

setup.py Show resolved Hide resolved
@jakobandersen
Copy link
Collaborator Author

A small clarification: the Python 2 to 3 changes in this PR are only the necessary ones to make things run. One should probably take a more thorough look at what could be simplified. For example, if I understand correctly, then the six module is no longer needed.

@vermeeren
Copy link
Collaborator

For example, if I understand correctly, then the six module is no longer needed.

Yeah, I believe this is the case too. The few lines where six is used directly should be reworked so that they are just regular python 3. There is likely a lot of legacy logic in Breathe, both for old Python versions and for old Sphinx versions. Fixing all of this is probably something for future polishing issues. (Or perhaps for Breathe 5 (partial) rewrite one day.)

Copy link

@ITAYC0HEN ITAYC0HEN 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. Confirmed working :)

@vermeeren
Copy link
Collaborator

@svenevs could you check if all is ok now and if so approve?

@svenevs
Copy link

svenevs commented Apr 7, 2020

It looks good to me, local testing seems successful too!

Copy link

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

Just tested on Zephyr's documentation and it seems to fix the issue I reported here:
sphinx-doc/sphinx#7424

@vermeeren
Copy link
Collaborator

@svenevs in order to approve for the UI you can go to the Files changed tab and in Review changes approve without submitting any comment. Perhaps some other methods to approve also exist.

Merging this, will do an upload afterwards.

@vermeeren vermeeren merged commit da4b482 into breathe-doc:master Apr 7, 2020
vermeeren added a commit that referenced this pull request Apr 7, 2020
@vermeeren
Copy link
Collaborator

Released Breathe 4.15.0. If you use an older Sphinx version 2.x it might be necessary to remain on 4.14.x, not sure.

@svenevs
Copy link

svenevs commented Apr 7, 2020

Oh, very sorry! I resolved the conversation but did not update the review to indicate no changes needed...

@StrikerRUS
Copy link
Contributor

StrikerRUS commented Apr 7, 2020

Hi guys!

Thanks a lot for the prompt fixes! I can confirm that this PR indeed fixed the error in Sphinx 3 about unknown c domain.
However, breathe 4.15 still cannot be used with Sphinx 3:

Exception occurred:
  File "/home/travis/.local/lib/python3.7/site-packages/sphinx/util/logging.py", line 415, in filter
    raise SphinxWarning(location + ":" + message)
TypeError: can only concatenate str (not "DefinitionError") to str

@vermeeren
Copy link
Collaborator

However, breathe 4.15 still cannot be used with Sphinx 3:

Breathe's CICD is passing, so this is false. It is possible you have discovered another bug in either Breathe or Sphinx though, Breathe's tests do not cover every (edge) case.

If I grep -rIin DefinitionError or grep -rIin SphinxWarning in the Breathe sources I do not find anything, so at a glance it appears this is a bug in Sphinx, even though it may be poor/broken Breathe data indirectly causing the problem.

Still, Sphinx should not exception even if Breathe is doing weird stuff, so for now I suggest opening an issue in the Sphinx repo with instructions on how to reproduce.

@StrikerRUS
Copy link
Contributor

@vermeeren Thanks for your reply! Reported with brief repro in sphinx-doc/sphinx#7423 (comment).

@jakobandersen jakobandersen deleted the sphinx_3_update branch April 9, 2020 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analysis Quality assurance, CI code Source code documentation Generated html, changelog, inline comments test Unit tests, acceptance tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants