Reduce build warnings #81
Conversation
underscore used by mistake causing a missing reference warning Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
warning indicated that linux_rpc_demo moved location so correct link Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
'remoteproc code implimentation' occurred twice likely due to copy paste error, causing a non critical warning but correct anyway to reduce warning count.
There was a problem hiding this comment.
Pull request overview
This PR reduces build warnings in the Sphinx documentation by excluding build directories and virtual environments from documentation parsing, while also correcting legitimate documentation issues that were flagged by warnings.
Key changes:
- Excluded
_doxygenand.venv*directories from Sphinx source parsing - Fixed incorrect reStructuredText cross-references and labels
- Corrected duplicate or mismatched type references in documentation
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| conf.py | Added exclusions for build/venv directories and suppressed duplicate declaration warnings |
| protocol_details/system_considerations.rst | Fixed malformed cross-reference syntax |
| protocol_details/rpmsg_protocol.rst | Corrected reference label name |
| protocol_details/lifecyclemgmt_creation.rst | Updated section heading titles for clarity |
| openamp/overview.rst | Fixed file path case sensitivity and improved link text descriptions |
| openamp/meeting_notes.rst | Removed entire file (obsolete meeting notes page) |
| docs/rpmsg_design.rst | Corrected duplicate typedef reference |
| docs/remoteproc_design.rst | Removed duplicate documentation section and added blank line |
| docs/index.rst | Removed entire file (appears to be unused master file) |
| docs/data_structures_content.rst | Clarified link text for code implementation |
| demos/linux_rpc.rst | Updated path to reflect repository restructuring |
| demos/docker_images.rst | Fixed malformed cross-reference syntax |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/rpmsg_design.rst
Outdated
| * RPMsg endpoint name service release callback. If user defines such callback, | ||
| when there is name service destroy arrives, it will call this callback to | ||
| notify the user application about the remote has destroyed the service.: |
There was a problem hiding this comment.
The typedef reference has changed from rpmsg_ns_unbind_cb to rpmsg_ept_release_cb, but the description on line 116-118 still mentions 'name service destroy' which may not accurately describe an endpoint release callback. Consider updating the description to match the new callback type's actual purpose.
| * RPMsg endpoint name service release callback. If user defines such callback, | |
| when there is name service destroy arrives, it will call this callback to | |
| notify the user application about the remote has destroyed the service.: | |
| * RPMsg endpoint release callback. If the user defines such a callback, | |
| it will be called when the associated RPMsg endpoint is released, | |
| allowing the user application to be notified about the endpoint's release.: |
|
@sipke: This is just a test on doc review with Copilot for evaluation purposes. Feel free to ignore it if you find it not relevant. |
|
|
||
| Meeting_Notes_ | ||
|
|
||
| .. _Meeting_Notes: https://github.com/OpenAMP/open-amp/wiki/Meeting-Notes |
There was a problem hiding this comment.
could be useful to keep somewhere in the documentation a reference to the technical meeting and associated note
There was a problem hiding this comment.
Placed under OpenAMP Project --> Links.
| .. doxygenfunction:: remoteproc_set_rsc_table | ||
| :project: openamp_doc_embed | ||
|
|
||
| Configure the remote presented by the remoteproc instance to make it able to load application |
There was a problem hiding this comment.
you should remove the first instance instead remoteproc_config can not be called before remoteproc_init
conf.py
Outdated
| # to avoid spurious warnings which clutter the build output. | ||
| suppress_warnings = ['duplicate_declaration.c', | ||
| 'duplicate_declaration.cpp', | ||
| 'toc.not_included'] |
There was a problem hiding this comment.
what is the risk here to hid some real issue?
There was a problem hiding this comment.
Its a trade-of I guess. With these not added, many warnings could be ignored due to the noise, so if we do not, then maybe we could spend the time to fix each case, then both risk are removed.
There was a problem hiding this comment.
I would be in favor of not masking the issues if it is only warning, and address them later if not blocking.
do you have an estimation of the cost to fix them one by one?
There was a problem hiding this comment.
Removed, so warnings won't be suppressed.
Both warnings are a little odd as in documentation one could want to reference same code from multiple documentation sections/pages and likely one has pages which are not in table of contents. I haven't yet found a way to mark a duplicate declaration warning as ignore when you know it needn't be a warning - that would be ideal.
The duplicate declaration ones in openamp-docs currently its mainly between remoteproc_design and porting_guide, both referencing the same remoteproc structs. These 'duplicate' references makes sense, so we may need to accept the warningm or change to using a doxy link rather than embeded doxygen (with breathe) which causes the warning. Effort in hours rather than days.
The toc ones look all to be in submodules. openamp-docs reference documentation in those, so we can not just filter out the submodules. To address we would need to revisit all submodule documents through to readme's etc. Effort in days and trickier being cross repository, but certainly a desired end goal as it will also unify the documentation. See PR #82 .
I think important to get what we now have (after review changes) to at least remove most of the warnings that crept in.
warnings show that these files are not indexed and they seemed to have been orphaned, so remove them. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
missing empty line causes warning so add it to remove the warning. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
link was incorrect as case was wrong. This removes a warning and corrects the link. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
A few links had duplicate names which results in a warning, so adjust the name to be unique which removes the warning. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
…rning The duplicate headings result in duplicate auto generated references with associated warning, so make them unique to remove this warning. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
warning noted missing :ref: tag, so add it. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
rpmsg-protocol-work-label was used in rpmsg_protocol.rst and rpmsg.rst so change one to remove duplication and associated warning. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
warning showed that there were two references to remoteproc_config so remove the alternate. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
sipke
force-pushed
the
feature/reduce-warnings
branch
from
581ba57 to
d86c443
Compare
docs/rpmsg_design.rst
Outdated
| notify the user application about the remote has destroyed the service.: | ||
| * RPMsg endpoint release callback. If the user defines such a callback, | ||
| it will be called when the associated RPMsg endpoint is released, | ||
| allowing the user application to be notified about the endpoint's release.: |
There was a problem hiding this comment.
it is not the exact use of the rpmsg_ept_release_cb. The CB is called when every pending actions around the endpoint are competed and so the endpoint can be removed.
| allowing the user application to be notified about the endpoint's release.: | |
| RPMsg endpoint release callback. If defined by the user, | |
| it will be called when the associated RPMsg endpoint is released, | |
| allowing the user application to safely remove associated allocation. |
warning showed that rpmsg_ns_unbind_cb was reference twice when the second should be the release rpmsg_ept_release_cb function. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
the README instructions were modified to recommend using a virtual environment which results in sphinx parsing the .venv directory, so add it to exclude_patterns as well as the 'build' folder _doxygen. This removes many unnecessary warnings that were inadvertently introduced. Signed-off-by: Sipke Vriend <sipke@direktembedded.com>
sipke
force-pushed
the
feature/reduce-warnings
branch
from
d86c443 to
5e28436
Compare
2 participants
A large amount of warnings had crept in due to _doxygen build directory and creation of venv directory in recent readme change, as these were treated as document source so sphinx was parsing all document files in python libraries etc.
While removing those, also corrected valid warnings which were actual items to correct.
A couple of warnings had to be suppressed as they are not trivial to correct.
The only remaining warnings are in sub-repositories, which we should probably consider removing also. Maybe create a ticket if we want to remove those also to help find documentation problems via the warnings in future.