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

Fix problematic doc build, due to the new builder image provided by readthedocs doesn't has the graphviz-dev package pre-installed any more #751

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

MapleCCC
Copy link
Contributor

@MapleCCC MapleCCC commented Aug 17, 2022

This pull request should fix #725.

The reason we should choose the graphviz package, instead of other packages whose package names also contain the graphviz keywords, is because it's officially endorsed.

…eadthedocs doesn't has the `graphviz-dev` package pre-installed any more
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 17, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #751 (34474c1) into main (977504f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #751   +/-   ##
=======================================
  Coverage   94.82%   94.82%           
=======================================
  Files         247      247           
  Lines       25799    25799           
=======================================
  Hits        24463    24463           
  Misses       1336     1336           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@MapleCCC
Copy link
Contributor Author

The CI errors are lxml-related. We can safely ignore them for our case here.

@abitrolly
Copy link

Adding it as a Python dependency should also work https://pypi.org/project/graphviz/

@MapleCCC
Copy link
Contributor Author

@abitrolly Thank you for the suggestion. However I am afraid the pypi.org/project/graphviz package is not an option here. The sphinx.ext.graphivz extension, used by libcst to generate graphs in the document, requires a dot command to be present on the $PATH, which the pypi.org/project/graphviz doesn't provide for now.

@zsol
Copy link
Member

zsol commented Aug 17, 2022

Thanks @MapleCCC for digging in and fixing the problem. Much appreciated. I'll merge and trigger a doc rebuild now

@zsol zsol merged commit 2bd6a64 into Instagram:main Aug 17, 2022
@zsol
Copy link
Member

zsol commented Aug 17, 2022

@zsol
Copy link
Member

zsol commented Aug 17, 2022

aaand, it's live: https://libcst.readthedocs.io/en/latest/why_libcst.html

@abitrolly
Copy link

Awesome graphs. :D I didn't pay attention that graphviz is still a wrapper over command line dot. Nice to see it resolved.

@MapleCCC
Copy link
Contributor Author

@zsol Thanks for your swift work! Glad the document works fine now. 🎉

@MapleCCC MapleCCC deleted the fix-doc-build-graphviz branch August 18, 2022 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation graphs are not rendered
5 participants