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

doc: add path to 'manpages' to conf.py #5855

Merged
merged 1 commit into from Apr 3, 2024

Conversation

jameshcorbett
Copy link
Member

I ran into an issue building the documentation while following the README, sphinx-build couldn't find the manpages module. Adding the path to sys.path manually or on a one-off basis is easy enough but I think adding this will be a bit more foolproof?

@grondo
Copy link
Contributor

grondo commented Apr 3, 2024

I hadn't looked at the README before, I wonder why it doesn't just say to run make or make html?

Anyway, would this change allow removal of the PYTHONPATH modification in Makefile.am?

e.g. on line 430 here:

flux-core/doc/Makefile.am

Lines 428 to 433 in 9ecf7b2

$(MAN_FILES): manpages.py conf.py $(RST_FILES)
$(sphinx_man) \
PYTHONPATH=$(PYTHONPATH):$(abs_srcdir) \
SPHINX_BUILDDIR=$(abs_builddir) $(PYTHON) \
-m sphinx $(sphinx_verbose_flags) -b man $(srcdir) ./man \
$(STDERR_DEVNULL)

@jameshcorbett
Copy link
Member Author

jameshcorbett commented Apr 3, 2024

Hmm, good question, I didn't think about that. Maybe so people don't have to build the code / run configure to build the documentation?

Edit: which is kinda nice for people who notice a problem with the documentation and don't want to build the whole project to open a PR!

Yeah it would allow us to drop the PYTHONPATH thing, I'll fix that and force-push.

@grondo
Copy link
Contributor

grondo commented Apr 3, 2024

Edit: which is kinda nice for people who notice a problem with the documentation and don't want to build the whole project to open a PR!

Good point! Though you don't have to build everything to build the docs, just ./configure, then cd docs && make or make html.

However, this looks like a good improvement!

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

LGTM!

@jameshcorbett
Copy link
Member Author

Thanks! Setting MWP.

Problem: sphinx-build cannot find the 'manpages' module which is in
the same directory as conf.py.

Add the directory to sys.path so it can be loaded.
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Merging #5855 (d7ee975) into master (5299547) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5855      +/-   ##
==========================================
- Coverage   83.28%   83.27%   -0.01%     
==========================================
  Files         513      513              
  Lines       82727    82727              
==========================================
- Hits        68898    68893       -5     
- Misses      13829    13834       +5     

see 16 files with indirect coverage changes

@mergify mergify bot merged commit 4598d41 into flux-framework:master Apr 3, 2024
35 checks passed
@jameshcorbett jameshcorbett deleted the doc-conf-path branch April 3, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants