-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[OCaml] build ocaml_doc by default to avoid missing install artifacts #154412
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
Ping |
1 similar comment
Ping |
Hi @nikic and @Ericson2314, Sorry to bother you. I was hoping to get a review on this pull request when you have some free time. This PR aims to resolve the long-standing build issues with the OCaml docs (fixes #154411, #125437, #108742). Please let me know if there's anything I can do to help move it forward. Thanks for your time and consideration. |
I believe the way this is supposed to work is that LLVM_BUILD_DOCS controls whether doc targets are part of ALL, while LLVM_INCLUDE_DOCS controls whether they are generated at all. I think we should stick to that, otherwise (if I understand everything correctly) this is going to start generating ocaml docs in the default build configuration (as LLVM_ENABLE_OCAMLDOC and LLVM_INCLUDE_DOCS are enabled by default). |
Hi @nikic, Thanks for the review and the clarification on the build flags. That makes sense. I've pushed a new version that adopts this approach. The ocaml_doc target is now added to the ALL build only when LLVM_BUILD_DOCS is enabled. This resolves the original problem without affecting the default build. I've tested both configurations, and they work as expected. Could you please take another look when you have a moment? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you please update the PR description with the new behavior?
@nikic Done. The description has been updated. Thanks for the review! |
Thanks for catching that. It was a typo on my part. The correct issue number is #125437. I've just corrected it in the PR description. Apologies for the confusion! |
Hi @nikic, Thanks again for the review and approval. I've now rebased and squashed everything into a single commit. Since I don't have write permissions, could you please help merge this when you have a moment? Thanks for your help! |
1f67cca
to
7c0e5a6
Compare
Add the ocaml_doc target to the default ALL build target, but only when LLVM_BUILD_DOCS is enabled. This addresses two issues: 1. It prevents the 'install' target from failing due to a missing ocamldoc output directory by ensuring docs are generated beforehand. 2. It avoids unnecessary build steps when documentation is not required (LLVM_BUILD_DOCS is OFF), saving build time.
Summary
Fixes #154411, #125437 and #108742.
When building with
-DLLVM_BUILD_DOCS=ON -DLLVM_ENABLE_OCAMLDOC=ON -DLLVM_INSTALL_DOCS=ON
, runningcmake --build . --target install
can fail with:
The
install(DIRECTORY …)
rule inllvm/docs/CMakeLists.txt
unconditionally expects theocamldoc/html
directory, but theocaml_doc
custom target that generates it is not part of the normalinstall
dependency chain. As a result,install
may run before docs are generated.Changes
This pull request conditionally adds the ocaml_doc target to the default build targets (ALL) only when LLVM_BUILD_DOCS is enabled.
This ensures that when a user intends to build documentation, the OCaml docs are automatically generated during a normal build (cmake --build .). Consequently, by the time the install target is executed, the ocamldoc/html directory is guaranteed to exist.
Behavior
Testing
On macOS (arm64):
cmake -DLLVM_BUILD_DOCS=ON -DLLVM_ENABLE_OCAMLDOC=ON -DLLVM_INSTALL_DOCS=ON … cmake --build . --target install
→ succeeds; OCaml docs are built and installed as expected.
Rationale