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

Check if the hierarchy files exist before generating includes. #147

Merged

Conversation

clalancette
Copy link
Collaborator

This reverts commit 0d1a8a8.

With this in place, I was getting errors of the kind:

docs_build/rcl_lifecycle/rcl_lifecycle/default_sphinx_project/generated/index.rst:3: CRITICAL: Problems with "include" directive path:
InputError: [Errno 2] No such file or directory: 'default_sphinx_project/generated/page_view_hierarchy.rst.include'.

That is, because final_data_string is empty, we were never creating page_view_hiearchy.rst.include, but we were still trying to .. include it. Revert this patch, which seems to make things happier. @roehling FYI. @svenevs What do you think?

@roehling
Copy link
Contributor

This revert will break Debian packages which use exhale, because dh_installdocs refuses to install empty documentation files.

This avoids warnings for Sphinx trying to include files that
don't exist.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette force-pushed the clalancette/revert-no-empty-hierarchies branch from 3a343ea to d65df03 Compare January 24, 2022 18:18
@clalancette
Copy link
Collaborator Author

This revert will break Debian packages which use exhale, because dh_installdocs refuses to install empty documentation files.

OK, thanks for the feedback. In that case, please check out d65df03

@clalancette clalancette changed the title Revert "Do not write empty hierarchies (#134)" Check if the hierarchy files exist before generating includes. Jan 24, 2022
@svenevs
Copy link
Owner

svenevs commented Jan 25, 2022

Thanks for the patch, will take a look this weekend. If you can link a project that was affected that'd be really helpful 🙂

@clalancette
Copy link
Collaborator Author

Thanks for the patch, will take a look this weekend. If you can link a project that was affected that'd be really helpful slightly_smiling_face

The project that I saw this with is rosdoc2: https://github.com/ros-infrastructure/rosdoc2 . rosdoc2, however, is really a wrapper around doxygen/sphinx/breathe/exhale, so that may not be entirely helpful to you. If you'd like me to try to extract an exact sphinx configuration that rosdoc2 generated that caused this problem, I can do that; just let me know.

@svenevs
Copy link
Owner

svenevs commented Jan 28, 2022

Yeah that could be helpful, or link me to a known project using it so I can test on that as well. Thanks!

@svenevs
Copy link
Owner

svenevs commented Feb 1, 2022

Adding note to myself, where I got turned around

# Make sure every document expected to be .. include::'ed in the library root

Those tests are issued for every test class via test_common() which gets injected via base testing class. Need to get repro or fix that section of the testing code.

@svenevs
Copy link
Owner

svenevs commented Feb 8, 2022

Tested things with #148 (comment) and some slight change to merge them both in, AFAICT this is good to go.

RUN git config --global user.email "[email protected]"
RUN git config --global user.name "Not, APerson"
RUN git fetch origin pull/148/head:pr-148
RUN git merge pr-148
RUN git fetch origin pull/147/head:pr-147
RUN git merge pr-147

@roehling Do the current changes here work for your project?

@roehling
Copy link
Contributor

roehling commented Feb 8, 2022

LGTM

@svenevs svenevs merged commit 36665ce into svenevs:master Feb 9, 2022
@svenevs
Copy link
Owner

svenevs commented Feb 9, 2022

Righto thanks for checking in 🙂

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.

3 participants