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.

Sign up for GitHub

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

Jump to bottom

fix(docs): fix most sphinx warnings #6916

Merged
merged 27 commits into from
Sep 30, 2024
Merged

fix(docs): fix most sphinx warnings #6916

merged 27 commits into from
Sep 30, 2024

Conversation

vwheeler63
Copy link
Contributor

cc: @kisvegabor

In PR #6542 Kevin made a number of valuable changes to .RST files the eliminate
a large number of Sphinx errors that he encountered. Those were cherry picked
and reviewed and brought here. On a test docs generation a few more cropped
up and they were handled here as well.

What it leaves UNhandled because it has something to do with breathe that I don't
quite get yet is a large number of warnings that look like this (all "Cannot find file"):

<tmpdir>\API/others/gridnav/lv_gridnav.rst:8: WARNING: doxygenfile: Cannot find file "lv_gridnav.h

and

<tmpdir>\widgets\led.rst:7: WARNING: duplicate label lv_led, other instance in <tmpdir>\API/widgets/led/lv_led.rst

which appears to be caused by an error in whatever preps the API .RST files for Sphinx. In the above example, I believe either this file: <tmpdir>\API/widgets/led/lv_led.rst or this file <tmpdir>\widgets\led.rst is in error, not sure which.

I am hoping Kevin ( @kdschlosser ) might know what to do with these.

Notes

  • Update the Documentation if needed. n/a
  • Add Examples if relevant. n/a
  • Add Tests if applicable. n/a
  • If you added new options to lv_conf_template.h run lv_conf_internal_gen.py and update Kconfig. n/a
  • Run scripts/code-format.py (astyle v3.4.12 needs to installed by running cd scripts; ./install_astyle.sh) and follow the Code Conventions. n/a
  • Mark the Pull request as Draft while you are working on the first version, and mark is as Ready when it's ready for review. (is ready)
  • When changes were requested, re-request review to notify the maintainers. Done.
  • Help us to review this Pull Request! Anyone can approve or request changes.

@vwheeler63 vwheeler63 force-pushed the fix/fix_most_sphinx_warnings_1 branch 3 times, most recently from 000b5bc to 1a73037 Compare September 23, 2024 16:28
vwheeler63
vwheeler63 commented Sep 23, 2024
View reviewed changes
Copy link
Contributor Author

@vwheeler63 vwheeler63 left a comment

Choose a reason for hiding this comment

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

I have reviewed each file and can assert that it is as it needs to be for correct formatting and eliminating Sphinx errors. The lv_conf_template.h and lv_conf_internal.h are minor formatting updates that got overlooked previously.

@vwheeler63 vwheeler63 force-pushed the fix/fix_most_sphinx_warnings_1 branch from 1a73037 to 4f63e36 Compare September 23, 2024 16:43
@kdschlosser
Copy link
Contributor

the duplicate labels are happening because there is a duplicate reference. It is a crappy error/warning that sphinx uses and it really needs to be changed upstream in sphinx.

If you look at the hard coded .rst files in the docs directory you will see .. _some-reference: above the header. That is a "label" It is actually an anchor for a reference. In the example you have posted above the problem needs to be fixed in the generation code for the API...

lvgl/docs/doc_builder.py

Line 1099 in 0227f7d

f.write('.. _{0}:'.format(name))

needs to be changed to

f.write('.. _{0}_h:'.format(name))

and

lvgl/docs/doc_builder.py

Line 1334 in 0227f7d

':ref:`{0}`\n'.format(inc)
needs to be changed to

':ref:{0}_h\n'.format(inc)

That will stop the label collisions.

There has been a massive change to the API naming and that includes filenames. If the hard coded .rst files have specific information that aligns with a specific piece of the API the filename omf the .rst file MUST match the header filename that the information is about. This is what links the hard coded .rst file with the proper API section in the documentation.

@kdschlosser
Copy link
Contributor

the problem with the not finding files is the reason why I went looking for a breathe replacement. Breathe is designed to read one header file at a time. The problem with that is the use of a configuration header file that exposes the various portions of the API. In order to allow everything to work properly each and every single header file in LVGL would need to include the config header file. How LVGL works currently is it includes the config header file a single time when lvgl.h is included and the config settings propagate because the compiler sees the macros from the config file because that file was loaded. But if using the compiler to read a single header file that information is now unavailable. It is a limitation of breathe and it is something that is not going to be fixed upstream in breathe unless the guy gets paid to fix it.

The other issue with breathe is it doesn't use full paths when generating documentation using the .. doxygenfile:: directive which is what the doc build system is using. So if you have header files that have the same name you run into problems.

@FASTSHIFT FASTSHIFT changed the title Fix Most Sphinx Warnings fix(docs): fix most sphinx warnings Sep 24, 2024
@vwheeler63
Copy link
Contributor Author

@FASTSHIFT

changed the title Fix Most Sphinx Warnings fix(docs): fix most sphinx warnings

Got it. 😉

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Sep 24, 2024 •
edited
Loading

@kisvegabor Ready with cherry-picked PR if you hadn't seen this already. Co-authored by Kevin if that part is not automated.

@vwheeler63 vwheeler63 mentioned this pull request Sep 24, 2024
Closed
@vwheeler63
Copy link
Contributor Author

The duplicate labels are happening because...

Thank you for the tips! It works!

If the hard coded .rst files have specific information that aligns with a specific piece of the API the filename of the .rst file MUST match the header filename that the information is about.

Got it. Handled.

The problem with the not finding files is the reason why I went looking for a breathe replacement.

Let's move this to #6924

kisvegabor
kisvegabor previously approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

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

Thank you Victor and Kevin.
Having these in place would make further work on the docs simpler and lighter.

@kisvegabor kisvegabor added the 👀 Review needed Anyone can review a PR label Sep 24, 2024
@vwheeler63
Copy link
Contributor Author

Thank you Victor and Kevin. Having these in place would make further work on the docs simpler and lighter.

I think we can eliminate all the sphinx warnings! As I type this, there is only one type left, and I know there was a point a couple of months ago when I was generating documentation without this type of error, so I think it is possible.

liamHowatt
liamHowatt reviewed Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@liamHowatt liamHowatt left a comment

Choose a reason for hiding this comment

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

Thanks!

docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/integration/chip/nxp.rst Outdated Show resolved Hide resolved
kisvegabor
kisvegabor previously approved these changes Sep 26, 2024
View reviewed changes
liamHowatt
liamHowatt previously approved these changes Sep 26, 2024
View reviewed changes
@liamHowatt
Copy link
Collaborator

Thanks! Looks like there are merge conflicts.

@vwheeler63
Copy link
Contributor Author

Thanks! Looks like there are merge conflicts.

I'm on it.

@vwheeler63 vwheeler63 force-pushed the fix/fix_most_sphinx_warnings_1 branch from bce37ef to 0b7c5c7 Compare September 26, 2024 10:30
@kdschlosser @vwheeler63
Fixes doc WARNING: Unparseable C cross-reference
4814262 
(cherry picked from commit cb25568)
@kdschlosser @vwheeler63
Fixes doc WARNING: Unparseable C++ cross-reference
8806db3 
(cherry picked from commit 46713c0)
@kdschlosser @vwheeler63
Fixes doc WARNING: Bullet list ends without a blank line
ebdda96 
(cherry picked from commit 5b11c5b)

# Conflicts:
#	docs/integration/ide/pc-simulator.rst
@kdschlosser @vwheeler63
Fixes doc ERROR: Unexpected indentation
0d8397a 
(cherry picked from commit 57bcff3)

# Conflicts:
#	docs/overview/renderers/arm2d.rst
#	docs/widgets/label.rst
@kdschlosser @vwheeler63
Fixes doc WARNING: Inline literal start-string without end-string
4ea5605 
(cherry picked from commit b224555)

# Conflicts:
#	docs/porting/indev.rst
vwheeler63 and others added 9 commits September 26, 2024 05:12
@vwheeler63
adds docs/README.md
78f59c3 
Guidelines for updating documentation.

(cherry picked from commit dcb9725)
@vwheeler63
fix: fix formatting errors
0b11489
@vwheeler63
fix: indentation errors
642d78f
@vwheeler63
fix: Fix most Sphinx warnings and misc formatting
b5de46f
@vwheeler63 @liamHowatt
Update docs/README.md
36fcb0b 
Co-authored-by: Liam <30486941+liamHowatt@users.noreply.github.com>
@vwheeler63 @liamHowatt
Update docs/README.md
e80b317 
Co-authored-by: Liam <30486941+liamHowatt@users.noreply.github.com>
@vwheeler63 @liamHowatt
Update docs/README.md
b38d3f5 
Co-authored-by: Liam <30486941+liamHowatt@users.noreply.github.com>
@vwheeler63
fix: error from outdated code in cherry-pick
22ca4ba
@vwheeler63
fix: another error from outdated cherry-pick
45f27e0
@vwheeler63 vwheeler63 force-pushed the fix/fix_most_sphinx_warnings_1 branch from 0b7c5c7 to 45f27e0 Compare September 26, 2024 11:13
@vwheeler63
Copy link
Contributor Author

@liamHowatt @kisvegabor

Conflicts handled and re-submitted. FYI, the underline lengths previously discussed also match the titles in this one.

@kisvegabor
Copy link
Member

There is a new conflict. 🙁

@vwheeler63
Merge branch 'master' into fix/fix_most_sphinx_warnings_1
d67bf88 
# Conflicts:
#	docs/overview/renderers/arm2d.rst
@vwheeler63
Copy link
Contributor Author

There is a new conflict. 🙁

No problem! Conflict fixed & re-submitted.

liamHowatt
liamHowatt approved these changes Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@liamHowatt liamHowatt left a comment

Choose a reason for hiding this comment

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

Thank you. I did not check thoroughly. I will assume the conflict resolution was ok 🙂

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Sep 27, 2024 •
edited
Loading

Thank you. I did not check thoroughly. I will assume the conflict resolution was ok 🙂

Thank you. Indeed. @kisvegabor Over to you, kind sir!

@kisvegabor
Copy link
Member

kisvegabor commented Sep 30, 2024 •
edited
Loading

Thank you!
I found these long dash lines in an MD files a little bit odd, but let's merge this PR now and fix it later.

@kisvegabor kisvegabor merged commit 0458acd into lvgl:master Sep 30, 2024
21 checks passed
@kdschlosser
Copy link
Contributor

@kisvegabor

The long dashed lines get changed into a horizontal line or hrule

@vwheeler63 vwheeler63 deleted the fix/fix_most_sphinx_warnings_1 branch September 30, 2024 13:09
@kisvegabor
Copy link
Member

The long dashed lines get changed into a horizontal line or hrule

I know, but I think we don't need these custom decorations, but for consistency we should rely on the markdown viewer's style.

vwheeler63 added a commit to vwheeler63/lvgl that referenced this pull request Oct 2, 2024
@vwheeler63
fix: README.md in ./docs/ reworked to handle...
5064a83 
- Remove horizontal rules as requested.
- Grammar corrected.
- Edited for clarity.
- Artificial line wraps (as `.md` files produce when the internal text
  is readable) removed, so that line wrapping will occur with
  browser width.
- Stated requirement for 2 blank lines after lists and code blocks
  removed as these do not always improve readability.
- 2 blank lines over headings requirement allowed to remain,
  since these always improve readability.
- File content examples outdented to left edge to more accurately
  portray file content.
- Several clarifications and added.
- Accurate reStructuredText vocabulary used.
- Error of only one colon after `.. code-block::` directive fixed
  (all directives other than link targets must have 2 colons).
- "Paragraph" clarified as "Sub Sub Sub Section" since not everyone
  will know Doxygen's @paragraph command marks the lowest-level
  Doxygen section heading.
- Examples for linking to code in API pages clarified with solutions.

Addresses request expressed by Gábor at end of PR lvgl#6916.
@vwheeler63 vwheeler63 mentioned this pull request Oct 2, 2024
Merged
@vwheeler63
Copy link
Contributor Author

The long dashed lines get changed into a horizontal line or hrule

I know, but I think we don't need these custom decorations, but for consistency we should rely on the markdown viewer's style.

Done! PR #6992

@kdschlosser
Copy link
Contributor

I know, but I think we don't need these custom decorations, but for consistency we should rely on the markdown viewer's style.

markdown viewer or ReST viewer?

@vwheeler63
Copy link
Contributor Author

I know, but I think we don't need these custom decorations, but for consistency we should rely on the markdown viewer's style.

markdown viewer or ReST viewer?

Markdown.

rodb70 pushed a commit to rodb70/lvgl that referenced this pull request Nov 8, 2024
@vwheeler63 @kdschlosser @liamHowatt
fix(docs): fix most sphinx warnings (lvgl#6916)
2d626fa 
Co-authored-by: Kevin Schlosser <kdschlosser@users.noreply.github.com>
Co-authored-by: Liam <30486941+liamHowatt@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdschlosser kdschlosser kdschlosser left review comments

@liamHowatt liamHowatt liamHowatt approved these changes

@kisvegabor kisvegabor kisvegabor approved these changes

Assignees
No one assigned
Labels
👀 Review needed Anyone can review a PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vwheeler63 @kdschlosser @liamHowatt @kisvegabor