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

Doxygen dot to Sphinx graphviz #757

Merged
merged 9 commits into from
Feb 13, 2022
Merged

Doxygen dot to Sphinx graphviz #757

merged 9 commits into from
Feb 13, 2022

Conversation

2bndy5
Copy link
Contributor

@2bndy5 2bndy5 commented Nov 7, 2021

This resolves #45

The approach is to translate any dot and dotfile doxygen commands into sphinx's graphviz directive. This now encompasses the inheritance/include graphs that are generated by Doxygen (which are now translated to dot syntax and put in a graphviz directive).

This feature is my first comprehensive contribution to this library, so I'm very open to criticism about lib convention and testing.

Limitations

This feature depends on having the dot executable installed to work its magic.
As such, I've added a allow-dot-graphs option to the following directives:

  • doxygenclass
  • doxygenstruct
  • doxygenfile
  • doxygenindex
  • autodoxygenfile
  • autodoxygenindex

Without this new option specified, breathe will behave like normal. If the dot executable is installed, then the user needs to specify this option to have graphs generated from XML.

  • I did not add this limitation to using the doxygen \dot & \dotfile cmds as those already (required by doxygen) depend on having the dot tool accessible.
  • Some graphs (like inheritance graphs) don't need the dot tool in doxygen, but breathe will need the dot tool to translate the generated graph for sphinx.

Also, as stated in the added document dot_graphs.rst

  1. The only graphviz directive option supported is the caption option. Graph captions are optionally
    specified using the dot or dotfile command syntax. All other graphviz directive options
    fallback to their default behavior.

    • any size hints from Doxygen's dot or dotfile commands are ignored.
  2. Using Doxygen's @ref command within any dot syntax is functionally ignored and treated as literal text.

Sample (updated)

Attached is a sample I've been using for testing purposes:
graph_breathe.zip

Note: I don't use the doxygenautoindex directive in the provided sample

unit test CI change

@michaeljones The sphinx branch 4.2.x no longer exists (happened a few days ago). I've changed this in the CI to use the new 4.3.x branch. I hope this is ok.

@2bndy5
Copy link
Contributor Author

2bndy5 commented Nov 7, 2021

The doxygendiagram branch (albeit very outdated) attempts a more complicated solution for supporting dot graphs. It may actually be focused only on builtin graphs generated by doxygen whereas this PR additionally focuses on user-defined graphs via \dot * \dotfile cmds.

@2bndy5 2bndy5 marked this pull request as draft November 10, 2021 22:03
@2bndy5 2bndy5 marked this pull request as ready for review November 11, 2021 14:15
@2bndy5 2bndy5 mentioned this pull request Nov 11, 2021
@2bndy5
Copy link
Contributor Author

2bndy5 commented Nov 15, 2021

Found a bug in the NodeFinder when enabling dot graphs for the doc's "Test Suite"'s "diagrams" example (generated xml from examples/doxygen/diagrams.cfg). It wasn't recognizing the graphviz nodes, so I fixed it. Now the docs show this feature in use (as it was originally intended).

@vermeeren
Copy link
Collaborator

@2bndy5 This seems very nice, thanks a lot. Just to make sure (even though I believe it says so in description), this does also include the inheritance diagrams etc found in Doxygen's own HTML output right? That really is a long-standing wish so fantastic to see this implemented if the case.

Do you consider this ready for review/merging?

@2bndy5
Copy link
Contributor Author

2bndy5 commented Dec 12, 2021

Just to make sure (even though I believe it says so in description), this does also include the inheritance diagrams etc found in Doxygen's own HTML output right?

Indeed, it does. This addition makes use of the dot executable though (as noted in the new docs' page dot_graphs.rst)

Do you consider this ready for review/merging?

Yes, please.

@2bndy5
Copy link
Contributor Author

2bndy5 commented Jan 28, 2022

Its been so long since I opened this PR, that I figured I'd double check the proposed changes....

I think I messed up my fork's master branch by merging all changes from all my separate PRs (targeting upstream) into a branch on my fork that was supposed to be called "remix" (I think I mistakenly merged to my fork's master).

  • The idea was to pip install my fork's remix branch until these changes were merged & released (because of limited attention/time allotted to this lib from maintainers).

With that said, I found and removed changes that were specific to #760 and #759 . If I have to keep rebasing this branch, then I'll have to do it manually because git seems to be confusing my fork's master with upstream master.

With all due respect, I want to mention that this lib's infrequent/sporadic dev activity is disheartening and tends to discourage contributions from those willing to do so. I mean no offense at all. It is amazing that the lib has gotten so old and popular considering the maintainers' limited time.

@vermeeren
Copy link
Collaborator

With all due respect, I want to mention that this lib's infrequent/sporadic dev activity is disheartening and tends to discourage contributions from those willing to do so. I mean no offense at all. It is amazing that the lib has gotten so old and popular considering the maintainers' limited time.

@2bndy5 Specifically for this, I do agree it's unfortunate. Especially because the bigger changes like this that really give a lot of value usually end up sitting in queue longer as a result. Normally things progress more quickly but the past year (or maybe two at this point) have just been tough for me personally resulting in too little time for Breathe.

I'll try my best to get some more time available for the pending PRs within a week and bump version again afterwards.

@vermeeren vermeeren self-assigned this Feb 13, 2022
@vermeeren vermeeren added code Source code enhancement Improvements, additions (also cosmetics) labels Feb 13, 2022
Copy link
Collaborator

@vermeeren vermeeren left a comment

Choose a reason for hiding this comment

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

@2bndy5 Really many thanks for the very thorough patches, documentation looking very good too! Will do some rebasing to clean up the commits and to get things on top of master, then final local test and merge.

I know it took way to long to get these things properly reviewed and checked, will try to continue and clear the entire queue today.

documentation/source/directives.rst Outdated Show resolved Hide resolved
examples/specific/dot_graphs.h Outdated Show resolved Hide resolved
examples/specific/dotfile.dot Outdated Show resolved Hide resolved
@vermeeren
Copy link
Collaborator

Rebasing this properly and handling and the merge commits and conflicts, removing the does+undoes, merging fixups was a ton of work, but it's finally done now and clean enough for merging.

@2bndy5 For a large part the convoluted history is a result of the delayed reviews, however for next time (at any project) I definitely recommend using git rebase -i upstream/master or similar to update a patchset with a force-push instead of separate fixup and merge branch commits. This is a lot easier to review per commit and also helps keep history clean and concise. (https://git-rebase.io/ may be useful in case you are not familiar.)

Thanks again for this great addition!

@michaeljones michaeljones merged commit 3435205 into breathe-doc:master Feb 13, 2022
michaeljones pushed a commit that referenced this pull request Feb 13, 2022
@2bndy5 2bndy5 deleted the doxygen-dot-to-sphinx-graphviz branch February 14, 2022 02:42
@gmarull
Copy link
Contributor

gmarull commented Feb 14, 2022

FYI #803

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code enhancement Improvements, additions (also cosmetics)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linking to dot graphs
4 participants