viirya commented on code in PR #7112:
URL: https://github.com/apache/datafusion-comet/pull/7112#discussion_r4316403667
##########
docs/generate-versions.py:
##########
@@ -151,9 +171,31 @@ def generate_docs(snapshot_version: str,
latest_released_version: str, previous_
major_minor = get_major_minor_version(version)
insert_warning_after_asf_header(f"temp/user-guide/{major_minor}",
warning)
+def alias_latest(html_root: str):
+ """Serve the newest release's built user guide at user-guide/latest as
well.
+
+ This copies the finished HTML rather than building a second copy of the
sources, so Sphinx
+ sees each document once. It is a copy and not a symlink because the site
is published by
+ rsync into the asf-site branch, and ASF's web server is not known to
follow symlinks. Every
+ link in the pages is relative, so the copy works unchanged one directory
over.
+ """
+ user_guide = Path(html_root) / "user-guide"
+ source = user_guide / get_major_minor_version(LATEST_RELEASED_VERSION)
+ if not source.is_dir():
+ raise FileNotFoundError(f"{source} was not built, so there is nothing
to alias as latest")
+ target = user_guide / "latest"
+ if target.exists():
Review Comment:
If a page is added under `docs/source/user-guide/latest/` after this lands,
Sphinx builds it into `user-guide/latest/` with only a "not in any toctree"
warning, and this `rmtree` then deletes it, so the page is silently missing
from the site. That could come from an in-flight PR or from old habits. Nothing
in PR CI runs Sphinx, so nothing would flag it before merge. A check here, or
in the lint job, would make it fail visibly:
```python
if (Path(__file__).parent / "source/user-guide/latest").exists():
raise RuntimeError("docs/source/user-guide/latest is reserved for the
release alias; use user-guide/dev")
```
##########
docs/source/conf.py:
##########
@@ -207,13 +207,6 @@
"user-guide/operators.html": "latest/operators.html",
"user-guide/source.html": "latest/source.html",
"user-guide/tuning.html": "latest/tuning.html",
- # The compatibility guide became a directory of pages in #4055.
- "user-guide/latest/compatibility.html": "compatibility/index.html",
Review Comment:
The description says the removed `latest/` redirects are covered because 1.1
has real pages at those paths. That's true for `compatibility/scans.html`,
`tuning/scans.html` and `iceberg-writes.html`. But branch-1.1 has no
`compatibility.html` (only `compatibility/index.md`) and no
`compatibility/parquet-iceberg-known-issues.html`.
The second was a dev-only page, so a 404 fits the stated policy. This one
was added deliberately in #6245, and it will now 404. No released jar prints it
(0.x jars print `user-guide/compatibility.html`, which still redirects into
1.1), so only existing external links are affected.
Keeping the entry wouldn't help as the code stands. sphinx-reredirects
writes its stubs during `html-collect-pages`, and `alias_latest` then removes
the whole `latest/` directory. If you want to keep this one,
`shutil.copytree(source, target, dirs_exist_ok=True)` would leave stubs for
paths 1.1 lacks and overwrite the ones it has; `build.sh` already starts from
an empty `build/`. Otherwise, just correcting the description is fine.
##########
docs/generate-versions.py:
##########
@@ -132,11 +152,11 @@ def publish_released_version(version: str):
def generate_docs(snapshot_version: str, latest_released_version: str,
previous_versions: list[str]):
# Strip release-only content; keep snapshot-only content for the latest
(snapshot) docs
Review Comment:
nit: this comment still describes these as the "latest (snapshot) docs";
they're the dev docs now.
##########
docs/generate-versions.py:
##########
@@ -111,16 +127,20 @@ def insert_warning_after_asf_header(root: str, warning:
str):
file.write_text("".join(new_lines), encoding="utf-8")
-def get_user_guide_dir(major_minor: str):
- if major_minor == "0.8" or major_minor == "0.9":
- return "docs/source/user-guide"
- else:
- return "docs/source/user-guide/latest"
+def get_user_guide_dir(clone_dir: str):
+ # Where a release branch keeps its user guide depends on when it was cut:
directly under
+ # user-guide/ before 0.10, under user-guide/latest/ until the 1.1 branch,
and under
+ # user-guide/dev/ after that.
+ for candidate in ("docs/source/user-guide/dev",
"docs/source/user-guide/latest"):
+ if (Path(clone_dir) / candidate).is_dir():
+ return candidate
+ return "docs/source/user-guide"
Review Comment:
This fallback only covers pre-0.10 branches, and the oldest published
version is now 0.13. The only other way to reach it is when the `git clone`
above fails, which `os.system` ignores, or when a future branch moves the guide
again. In those cases it copies the whole `docs/source/user-guide` tree into
the versioned directory without any error. Raising here instead would be
simpler and would fail visibly.
##########
spark/src/test/scala/org/apache/spark/sql/benchmark/CometIcebergWriteBenchmark.scala:
##########
@@ -61,7 +61,7 @@ import org.apache.comet.CometSparkSessionExtensions
* pay equally. It dilutes the writer's factor rather than inflating it.
* - The warehouse is a local temporary directory, so no object-store
latency is included.
* - The two writers choose different file roll points (see the accepted
divergences in
- * `docs/source/user-guide/latest/iceberg-writes.md`), so the resulting
file layouts are not
+ * `docs/source/user-guide/dev/iceberg-writes.md`), so the resulting file
layouts are not
Review Comment:
nit, not from this PR: `iceberg-writes.md` was merged into `iceberg.md` in
#7090, so this path doesn't exist under `dev/`. Since the line is touched
anyway, it could point at `docs/source/user-guide/dev/iceberg.md` (the
"Accepted divergences" section). The same line is in
`CometIcebergEndToEndBenchmark.scala:64`.
##########
.ai/skills/review-comet-expression-pr/SKILL.md:
##########
@@ -240,7 +240,7 @@ merge. Never ask the contributor to hand-edit them. Check
what feeds them:
### Hand-edited, the PR should update these
-- `docs/source/user-guide/latest/expressions.md`, the supported-expressions
list. A new expression
+- `docs/source/user-guide/dev/expressions.md`, the supported-expressions list.
A new expression
belongs here.
- Other `latest/compatibility/` pages such as `floating-point.md`, `regex.md`,
`operators.md`.
Review Comment:
nit: one leftover from the rename. This line still says
`latest/compatibility/`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
