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

[docs] Fix broken docs links in PyCDE and diagrams on Getting Started page #5013

Merged
merged 1 commit into from
May 1, 2023

Conversation

VMois
Copy link
Contributor

@VMois VMois commented Apr 12, 2023

The current link to PyCDE in https://circt.llvm.org/docs/PythonBindings/ leads to https://circt.llvm.org/PyCDE, which is broken. The correct one is "docs/PyCDE".

While checking for this, I found that a few more links/images are broken, and building "docs/" using doxygen 1.9.6 does not work. I spent some time fighting it (replacing abs_top_src with hardcoded paths) and still could get similar pages as in the current docs website.

Am I doing something wrong with doxygen? I want to learn more about CIRCT and was checking docs.

@teqdruid
Copy link
Contributor

I've never tried to build the doxygen docs locally... Are links/images in the doxygen-generated html broken or are they in the md files?

Bear in mind that the website publishing pipeline moves things around so the docs generated locally aren't necessarily representative of the ones on the website: https://github.com/llvm/circt-www

Thanks for the PR!

@VMois
Copy link
Contributor Author

VMois commented Apr 13, 2023

Are links/images in the doxygen-generated html broken, or are they in the md files?

In md files. If you try to build docs with no changes, doxygen will fail and generate empty HTML (due to wrong absolute paths). I fixed some of those issues (more like patching to make something work), but it might be unnecessary now that you told me there is a different repository handling docs and the website.

I think some global prefix docs/ for doxygen might help with issues.

Bear in mind that the website publishing pipeline moves things around so the docs generated locally aren't necessarily representative of the ones on the website: https://github.com/llvm/circt-www

Thanks for this lead.

I don't think this docs issue is urgent. If that is okay, I will look at it in the next few days.

Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

I think this will work, thanks for looking at it.

@darthscsi
Copy link
Contributor

Any reason this is a draft? Should we move on it or drop it?

@VMois VMois changed the title [docs] Fix broken links [docs] Fix broken docs links in PyCDE and diagrams on Getting Started page Apr 30, 2023
@VMois
Copy link
Contributor Author

VMois commented Apr 30, 2023

Any reason this is a draft? Should we move on it or drop it?

Hi. Not sure if making a global prefix is a good solution, as in other places, docs work fine. I checked the docs, found one more place with a broken link, and fixed typos. I think the PR is ready.

P.S I haven't tested if it fixes the links cause I couldn't find a time. For diagrams, I followed a main page that has them working correctly - https://circt.llvm.org/. And for PyCDE page, if "/" is removed it will resolve to "/docs/PyCDE" instead of "/PyCDE" so should work.

@VMois VMois marked this pull request as ready for review April 30, 2023 20:22
@mikeurbach
Copy link
Contributor

I think we can merge this. The links are already broken, so if this doesn't fix it we can do some more in depth testing. I'm not sure if anyone here knows a better method offhand, and using the global path prefix seems reasonable to me.

@mikeurbach mikeurbach merged commit b142ded into llvm:main May 1, 2023
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.

4 participants