bazel: build and package man pages as runfiles#9766
bazel: build and package man pages as runfiles#9766alokkumardalei-wq wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new Bazel rule to build and package man pages, making them available to the openroad binary as runfiles. This is a valuable addition that improves the build process. My review focuses on enhancing the robustness and maintainability of the new Bazel rule and improving the portability of the C++ code that locates the generated documentation at runtime.
|
clang-tidy review says "All clean, LGTM! 👍" |
b02b33d to
cb4284b
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
abb37f2 to
4f974a0
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
4f974a0 to
20a70f9
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
20a70f9 to
57a1471
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
57a1471 to
693980d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
When I try this I get: and no files are created in the runfiles. |
|
clang-tidy review says "All clean, LGTM! 👍" |
068e6ad to
0070f75
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @gadfort I think this is due to PATH environment problem in Bazel's sandboxed execution on macOS. I have fixed that I want you to have look into this when you have time and happy to address further feedback. Thank you! |
|
Hello @gadfort all checks are passing now ,please have a look onto this when you have time . |
|
It builds, but |
|
clang-tidy review says "All clean, LGTM! 👍" |
0070f75 to
693980d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @gadfort, Please have a look onto this and happy to address any further feedback. Thank you! |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @maliberty and @gadfort thanks for your feedback and time, all checks are passing and I have addressed all your Thank you ! |
|
fec1f74 to
982e88e
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
ced4ab9 to
44a564d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
6ee2f47 to
d073a29
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Hello , @maliberty now you can check the same , it is fixed |
|
Hello @maliberty @gadfort , all checks are passing ,please have a look onto this when you get time. Thank you ! |
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Bazel rule, man_pages, to automate the generation of man pages and HTML documentation from module READMEs, integrating with the existing documentation Makefile. It also updates the OpenRoad class to locate these documents within the Bazel runfiles directory and refactors runfiles management in regression tests. Review feedback focuses on improving the robustness of the shell commands within the Bazel rule—specifically by avoiding silent failures and handling empty output directories—and notes the fragility of hardcoding the workspace name in the runfiles path lookup.
| cd docs | ||
|
|
||
| # Symlink module READMEs into md/man2/ | ||
| bash src/scripts/link_readmes.sh 2>/dev/null || true |
There was a problem hiding this comment.
Masking errors with || true and redirecting stderr to /dev/null can lead to silent failures during the documentation generation process. If the script fails, the resulting man pages might be incomplete or incorrect. It is better to allow the script to fail the build so that issues can be identified and fixed.
| bash src/scripts/link_readmes.sh 2>/dev/null || true | |
| bash src/scripts/link_readmes.sh |
| cp -r docs/cat/* {cat_out}/ | ||
| cp -r docs/html/* {html_out}/ |
There was a problem hiding this comment.
Using the * glob in the cp command will cause the shell to fail if the source directory is empty (which might happen if the make command fails to produce output but doesn't return a non-zero exit code). Using . is a more robust way to copy the contents of the directory.
| cp -r docs/cat/* {cat_out}/ | |
| cp -r docs/html/* {html_out}/ | |
| cp -r docs/cat/. {cat_out}/ | |
| cp -r docs/html/. {html_out}/ |
|
|
||
| // Check Bazel runfiles layout first | ||
| const std::filesystem::path runfiles_docs | ||
| = std::filesystem::path(exe + ".runfiles") / "_main" / "docs"; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d073a29ad8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| man_pages( | ||
| name = "man_pages", |
There was a problem hiding this comment.
Add //docs:man_pages to the openroad data deps
This target is defined but never consumed by the //:openroad binary, so the new getDocsPath() probe for <exe>.runfiles/_main/docs will not find generated docs during bazel run //:openroad. I checked for //docs:man_pages references and it only appears in this file, which means the man pages are built in isolation but not packaged with the executable’s runfiles; man/GUI help remains unavailable in Bazel-run binaries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This matches my experience, man in the application doesn't find anything
|
|
||
| ctx.actions.run_shell( | ||
| outputs = [cat_dir, html_dir], | ||
| inputs = ctx.files.docs_srcs + ctx.files.scripts + ctx.files.readmes, |
There was a problem hiding this comment.
Declare man3 messages files as Bazel action inputs
md_roff_compat.py generates man3 pages from ../src/*/messages.txt and ../messages.txt, but this action only declares docs_srcs, scripts, and readmes as inputs. In Bazel sandboxing those message files are missing, so man3() skips them and cat3/html3 outputs are incomplete, which breaks lookups for message-ID documentation. Add the messages files as explicit attrs and include them in inputs.
Useful? React with 👍 / 👎.
d073a29 to
1638325
Compare
|
Hello @maliberty , Fixed- the man pages target was being built but never added to the binary's runfiles, so Please have a look onto this when you get time. Thank you ! |
|
clang-tidy review says "All clean, LGTM! 👍" |
f9da33f to
d96b6de
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@maliberty what's the best way to reach you - discord/slack, email, or here on GitHub? |
aadcb10 to
a973981
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
a973981 to
cdd3028
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
What does this PR do?
Fixes #7117 — Bazel build now generates and packages man pages
The Bazel build was missing man pages for both the man command and the GUI help browser.
Added a man_pages Bazel rule (
man_pages.bzl
) that runs the existing docs Makefile to generate cat/ and html/ pages from the module READMEs. The output is packaged as runfiles alongside the openroad binary.
Updated getDocsPath() to check for the Bazel runfiles layout (.runfiles/openroad/docs/) before falling back to the installed path.
Verification/Testing
Expected output: