docs: build historical multiversion docs with Java 8#909
Draft
dkropachev wants to merge 1 commit into
Draft
Conversation
📝 WalkthroughWalkthroughDocs workflows now trigger on changes to their own YAML files, add JDK 8 setup steps, and run a published multiversion build. Javadoc-generation scripts were hardened; a new simplified multiversion-javadoc.sh selects Java 8/11, cleans output, and invokes javadoc.sh. multiversion.sh runs from the repository root. ChangesDocumentation Multi-version Build System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
dkropachev
force-pushed
the
dk/fix-docs-multiversion-jdk-selection
branch
from
af6b211 to
dba3d8e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/_utils/multiversion-javadoc.sh (2)
43-51: 💤 Low valueConsider defensive validation before deleting output directory.
While the
rm -rfon line 50 is generally safe withset -upreventing unset variables, an explicit safety check would make the code more obviously correct and prevent accidental deletion if the directory path logic changes in the future.🛡️ Add explicit path validation
fi + # Safety check: ensure we're not deleting something unexpected + if [[ "$output_dir" != */api ]]; then + echo "Error: Unexpected output directory path: $output_dir" + exit 1 + fi + rm -rf "$output_dir" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/_utils/multiversion-javadoc.sh` around lines 43 - 51, In clean_javadoc_output(), add a defensive validation before calling rm -rf on the computed output_dir: verify output_dir is non-empty, not equal to "/" or ".", and matches the expected local path pattern (e.g., starts with "docs/_build" or ends with "/api") or confirm it resolves under the repository directory; if the validation fails, log an error and exit non-zero instead of deleting. Ensure you reference the computed variable output_dir and the environment override SPHINX_MULTIVERSION_OUTPUTDIR when performing the checks so the safety guard runs for both default and overridden paths.
57-57: ⚡ Quick winThe
bash -einvocation is redundant.Since
javadoc.shnow begins withset -euo pipefail(line 2), explicitly invoking it withbash -eis redundant. The script will handle its own error exits.♻️ Simplify invocation
-bash -e ./docs/_utils/javadoc.sh +bash ./docs/_utils/javadoc.shAlternatively, if you want to ensure fail-fast even for legacy checkouts where
javadoc.shmight not have been hardened yet:-bash -e ./docs/_utils/javadoc.sh +bash -euo pipefail ./docs/_utils/javadoc.sh🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/_utils/multiversion-javadoc.sh` at line 57, The invocation in multiversion-javadoc.sh is redundant: replace the line calling bash -e with a plain executable call to ./docs/_utils/javadoc.sh (the javadoc.sh script already contains set -euo pipefail), i.e., update the call in multiversion-javadoc.sh to invoke javadoc.sh directly so error handling is delegated to javadoc.sh; if you must keep compatibility for older checkouts, add a short comment explaining why you would retain bash -e, otherwise remove it.docs/_utils/javadoc.sh (1)
20-20: ⚡ Quick winVerify glob expansion succeeds before moving.
The unquoted glob
core/target/site/apidocs/*will fail if the directory is empty or ifjavadoc:javadocdoesn't generate files. Withset -e, the script will exit, but the error message may be unclear.🛡️ Add explicit check for generated files
# Generate javadoc mvn javadoc:javadoc -T 1C if [[ -d "$OUTPUT_DIR" ]]; then rm -r "$OUTPUT_DIR" fi mkdir -p "$OUTPUT_DIR" +if ! compgen -G "core/target/site/apidocs/*" > /dev/null; then + echo "Error: Javadoc generation produced no files in core/target/site/apidocs/" + exit 1 +fi mv -f core/target/site/apidocs/* "$OUTPUT_DIR"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/_utils/javadoc.sh` at line 20, The mv invocation in javadoc.sh uses an unquoted glob (core/target/site/apidocs/*) which will fail when no files are generated; update the script to check for files before moving: test whether the apidocs directory exists and contains at least one file (e.g. using a glob check or a nullglob-aware conditional) and only run the mv to "$OUTPUT_DIR" when files are present, otherwise log a clear message or create an empty placeholder; reference the existing mv command and the OUTPUT_DIR variable when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/_utils/javadoc.sh`:
- Line 20: The mv invocation in javadoc.sh uses an unquoted glob
(core/target/site/apidocs/*) which will fail when no files are generated; update
the script to check for files before moving: test whether the apidocs directory
exists and contains at least one file (e.g. using a glob check or a
nullglob-aware conditional) and only run the mv to "$OUTPUT_DIR" when files are
present, otherwise log a clear message or create an empty placeholder; reference
the existing mv command and the OUTPUT_DIR variable when making the change.
In `@docs/_utils/multiversion-javadoc.sh`:
- Around line 43-51: In clean_javadoc_output(), add a defensive validation
before calling rm -rf on the computed output_dir: verify output_dir is
non-empty, not equal to "/" or ".", and matches the expected local path pattern
(e.g., starts with "docs/_build" or ends with "/api") or confirm it resolves
under the repository directory; if the validation fails, log an error and exit
non-zero instead of deleting. Ensure you reference the computed variable
output_dir and the environment override SPHINX_MULTIVERSION_OUTPUTDIR when
performing the checks so the safety guard runs for both default and overridden
paths.
- Line 57: The invocation in multiversion-javadoc.sh is redundant: replace the
line calling bash -e with a plain executable call to ./docs/_utils/javadoc.sh
(the javadoc.sh script already contains set -euo pipefail), i.e., update the
call in multiversion-javadoc.sh to invoke javadoc.sh directly so error handling
is delegated to javadoc.sh; if you must keep compatibility for older checkouts,
add a short comment explaining why you would retain bash -e, otherwise remove
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df7f0882-64b1-4bff-acec-59136fea4a34
📒 Files selected for processing (5)
.github/workflows/docs-pages.yml.github/workflows/docs-pr.ymldocs/_utils/javadoc.shdocs/_utils/multiversion-javadoc.shdocs/_utils/multiversion.sh
dkropachev
force-pushed
the
dk/fix-docs-multiversion-jdk-selection
branch
2 times, most recently
from
07064fb to
ece08ca
Compare
dkropachev
changed the title
The docs publish workflow failed in the Build docs step because it runs make -C docs multiversion, while PR docs CI only ran make -C docs test. That left the published-docs multiversion/Javadoc path uncovered until after merge. The multiversion build renders historical scylla-* branches and runs each checkout's branch-local docs/_utils/javadoc.sh as a sphinx-multiversion post-build hook. Those historical checkouts should keep using their own docs and Maven config, but that config was written for the Java 8-era build. Running it under Java 11 exposed old Error Prone/Javadoc assumptions, stale external Javadoc links, and legacy scripts that could fail only after Maven left no API docs to copy. Install Java 8 alongside Java 11 in the docs workflows. Java 11 remains the workflow default for normal docs. The current branch multiversion script selects Java 8 for historical refs before delegating to the branch-local Javadoc script, preserving same-ref docs config instead of patching temporary historical checkouts. Also run make -C docs multiversion in PR CI, include docs workflow files in the docs workflow path filters, and make the current branch Javadoc script fail fast while avoiding install-time Javadoc attachment.
dkropachev
force-pushed
the
dk/fix-docs-multiversion-jdk-selection
branch
from
ece08ca to
91b0937
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Docs / Publishfailed in theBuild docsstep for https://github.com/scylladb/java-driver/actions/runs/26452394479/job/77876624864. That job runsmake -C docs multiversion, but the PR docs workflow only ranmake -C docs test, so the published-docs multiversion/Javadoc path could break after merge without being exercised in PR CI.make -C docs multiversionrenders historicalscylla-*branches and runs each checkout's branch-localdocs/_utils/javadoc.sh. Those historical checkouts should keep using their own docs and Maven config, but that config was written for the Java 8-era build. Running it under Java 11 exposed old Error Prone/Javadoc assumptions, stale external Javadoc links, and legacy scripts that can fail after Maven leaves no API docs to copy.Some historical Sphinx refs also print non-fatal warnings/
CRITICALdiagnostics, but the publish-breaking exit was the historical Javadoc path under the wrong Java runtime.Fix
docs/_utils/multiversion.shselect Java 8 for historicalscylla-*/tag refs before the post-build Javadoc step../docs/_utils/javadoc.sh; historical checkout docs/Maven config is not patched.make -C docs multiversionin PR CI so the publish path is covered before merge.docs/_utils/javadoc.shfail fast, skip install-time Javadoc attachment, and handle API output directories safely.Testing
bash -n docs/_utils/javadoc.sh docs/_utils/multiversion.shgit diff --checkorigin/scylla-3.10.2.xusing the exact command generated bydocs/_utils/multiversion.sh; it used the branch-localdocs/_utils/javadoc.shand passedJAVA_HOME=$HOME/.sdkman/candidates/java/11.0.29-tem JAVA_HOME_8_X64=$HOME/.sdkman/candidates/java/8.0.472-tem JAVA_HOME_11_X64=$HOME/.sdkman/candidates/java/11.0.29-tem PATH=$HOME/.sdkman/candidates/java/11.0.29-tem/bin:$PATH make -C docs multiversion