[SPARK-56636][INFRA] Decouple scalastyle from compile + clean up unidoc log noise#55563
Closed
cloud-fan wants to merge 35 commits into
Closed
[SPARK-56636][INFRA] Decouple scalastyle from compile + clean up unidoc log noise#55563cloud-fan wants to merge 35 commits into
cloud-fan wants to merge 35 commits into
Conversation
… PR annotations
Today, three of the most common Spark CI failures cascade into ~7 red checks
each, all surfacing only a generic "Process completed with exit code 1" — the
actual file:line lives buried in a multi-megabyte sbt log. This PR teaches
each of the three to emit GitHub Actions `::error` annotations so the real
culprit shows up inline on the PR's "Files changed" tab.
Changes:
1. `dev/scalastyle` — when running under GitHub Actions, parse scalastyle's
native `error file=PATH message=... line=N` console output and re-emit it
as `::error file=...,line=...,title=Scalastyle::...`. A single violation
in catalyst that previously cascaded into Linters/Java 17 Maven/Java 25
Maven/Documentation generation/sparkr/Docker/TPC-DS now appears as one
inline annotation.
2. `docs/_plugins/build_api_docs.rb` — extend SPARK-56630's
`diagnose_unidoc_failure` so its third branch (javadoc died before per-class
HTML generation began, so no `Generating .../X.html` line is available to
pin a culprit) also runs a small PR-scope scan. The scan diffs the changed
files against the master tip (using the docs job's existing `Merged commit`
layout) and reports doc-tag patterns that have previously crashed the
standard doclet's tree builder. Today only one pattern is checked
(block-form `@inheritDoc` — which is not a valid Javadoc tag and Spark's
custom block tag is the lowercase `@inheritdoc` registered via
`SparkBuild.scala`'s `-tag inheritdoc`); the structure makes adding more
patterns trivial. Each hit is also emitted as a GitHub annotation.
3. `dev/run-tests.py` — `exec_sbt` already streams sbt output line by line
to stdout. Add a side-channel that, in CI, matches sbt's canonical
`[error] PATH:LINE[:COL]: message` shape and re-emits it as
`::error file=...,line=...,title=Compile error::...`. genjavadoc-generated
stub paths under `target/java/...` are filtered out — those errors are
intentionally non-fatal (`--ignore-source-errors` is set for unidoc) and
would otherwise drown the actually-actionable annotations.
All three sites use the same workflow command syntax, so any failing job
annotated this way shows the exact violation inline next to the offending
line — no full job log needed, no cross-job navigation needed.
### Why are the changes needed?
Cascade failures are the dominant source of CI debug time on this project.
A single scalastyle violation in catalyst caused 7 red checks earlier today,
each surfacing only a generic "exit code 1" annotation; the user (me) had
to grep through a multi-megabyte job log to find the actual file:line. Same
shape for compile errors and the recent block-form-`@inheritDoc` unidoc
crash. GitHub Actions has structured annotation support for exactly this
case — Spark just isn't using it yet for these three failure modes.
### Does this PR introduce _any_ user-facing change?
No -- CI-only ergonomics. No change to how the checks succeed or fail; only
the in-PR rendering of failure detail.
### How was this patch tested?
Each parser was unit-checked locally with representative real samples taken
from prior CI runs:
- scalastyle: `dev/scalastyle`'s post-processor against scalastyle's native
console output — confirmed conversion to `::error file=...` syntax.
- unidoc hazard scan: the regex was checked against the pattern that caused
the SPARK-52729 unidoc crash (block `@inheritDoc`) plus near-miss forms
that must NOT match (`{@inheritdoc}` inline, lowercase `@inheritdoc`,
trailing text).
- sbt compile errors: matcher confirmed against real Spark `[error] PATH:N:C: msg`
lines and against genjavadoc stubs (`target/java/...`) that must be filtered.
Will validate end-to-end on the PR's own CI run; if any of the three
emitters mis-classifies real output, the worst case is an extra noisy
annotation -- the underlying tool output is unchanged.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude (Anthropic)
…p ANSI, simplify target-path filter Two small follow-ups on the compile-error annotator added in this PR: - Strip ANSI color codes before matching, mirroring the unidoc helper. Today sbt under subprocess.PIPE detects no TTY and skips colors, so `^[error]` matches as-is. But if sbt color is ever forced (`-Dsbt.color=always`, custom CI shell), `\e[31m` would silently kill every annotation. Cheap to harden. - Simplify the genjavadoc-stub filter to just `"/target/java/" in file_path`. The previous expression `A or B and C` relied on Python's `and`-binds- tighter precedence, which is correct but easy to misread, and the second clause was broader than the surrounding comment claimed (it would also filter any generated `.java` under any `/target/`). genjavadoc's output dir is the SBT default `target/java/`, so the first clause already covers it. Co-authored-by: Isaac
…ude column=N scalastyle's TextOutput appends `column=N` after `line=N` when a checker reports a column (Spark enables WhitespaceEndOfLineChecker, which does). The previous regex anchored `line=([0-9]+)$` to end-of-line, so those violations slipped through un-annotated. Add an optional `column=N` tail to the anchor. Co-authored-by: Isaac
…notation emission Adds a deliberate 162-char comment line in Aggregator.scala to trigger scalastyle's LineLengthChecker, exercising the new `dev/scalastyle` GitHub-annotation emitter end-to-end on this PR's CI run. The annotation should render inline on the "Files changed" tab of this PR. Will be reverted before merge; this commit must not land in master. Co-authored-by: Isaac
…d drop misattributed unidoc-crash citation
Two follow-ups uncovered while validating SPARK-56636 end-to-end on a planted
scalastyle violation:
1. `dev/scalastyle`: extend the annotation regex to ALSO match sbt's logger
format. The original regex only matched scalastyle's native console writer:
error file=<path> message=<text> line=<n> [column=<n>]
But the explicit `scalastyle` / `test:scalastyle` tasks run AFTER the
`scalaStyleOnCompile` hook on `Compile / compile` -- which fires first as
a build dependency and emits violations through sbt's `[error]` logger:
[error] <path>:<line>: <message>
In CI the compile-time hook short-circuits before the explicit tasks run,
so we only ever see the second format. Without the second branch, the
PR-annotation path silently does nothing -- exactly what happened on the
validation run, which cascaded into 7 red checks but emitted zero
annotations.
Also strip ANSI color codes before matching, mirroring the same
hardening already done for `dev/run-tests.py`'s annotator. Today sbt
under awk's pipe is not a TTY and skips color, but `-Dsbt.color=always`
would silently kill every annotation otherwise.
The two regex branches are precise enough to not catch near-misses:
sbt's "task failed" lines (no `:line:`), bare exception traces, and
regular Scala compile errors with their `:line:col:` triple-colon shape
are all correctly skipped.
2. `docs/_plugins/build_api_docs.rb`: the inline comment for the
`@inheritDoc` block-tag hazard claimed javadoc 17 had been observed to
hard-exit during "Building tree" when this pattern was present, citing
SPARK-56636 itself. Subsequent debugging on the v1-v2 branch showed the
crash had a different (still-being-investigated) trigger -- the
block-form fix did not green doc gen. Reframe the comment to describe
what is actually true: block-form `@inheritDoc` produces no inherited
content and is silently dropped, which is reason enough to surface it
during the unidoc-failure window so reviewers catch it in one round.
Co-authored-by: Isaac
Tier-1 structural fix that the annotation work in this PR has been
band-aiding. Three changes; together they collapse the cascade entirely
for style violations.
1. SBT (`project/SparkBuild.scala`): drop the
`(Compile / compile) := { scalaStyleOnCompile.value; ... }` and matching
`(Test / compile)` hooks from `enableScalaStyle`. The standalone
`scalaStyleOnCompile` / `scalaStyleOnTest` task definitions remain so
`dev/lint-scala` -> `dev/scalastyle` -> `sbt scalastyle test:scalastyle`
continues to work; only the compile-time auto-invocation is removed.
Also drops the `noLintOnCompile` env-var gating: with no compile hook to
gate, `NOLINT_ON_COMPILE` was a no-op. Always include `enableScalaStyle`
in `sharedSettings` so the standalone tasks are always defined.
2. Workflow (`.github/workflows/build_and_test.yml`): remove the now-no-op
`NOLINT_ON_COMPILE: true` / `false` env vars from the Build modules,
pyspark, sparkr, lint, and docs jobs. Pure cleanup.
3. Maven (`pom.xml`): remove the default `<execution>` from
`scalastyle-maven-plugin` and add an opt-in `scalastyle` profile that
re-binds the `check` goal. Default Maven builds (`mvn install`,
`mvn package`, etc.) no longer trigger scalastyle; activate the profile
(`mvn ... -Pscalastyle ...`) or invoke the goal directly
(`mvn scalastyle:check`) to run it.
After this change, scalastyle runs in exactly one place in CI: the dedicated
lint job (which goes through SBT via `dev/lint-scala`). A style violation
fails ONE check with the inline file:line annotation already shipped in
this PR; every other CI job is unaffected and stays green. No `needs: lint`
chaining is needed -- the cascade is gone at the source.
Compile-error cascades remain (a real compile error in catalyst still
fails every job that depends on catalyst), but each job now surfaces an
inline `Compile error` annotation pointing at the same file:line, so the
duplication is just visual noise rather than a debugging cost.
Co-authored-by: Isaac
…rt validation
Removes two pieces of the original PR after a discussion of marginal value:
1. `dev/run-tests.py` compile-error annotation filter -- reverted. sbt's
`[error] PATH:LINE:COL: msg` is already greppable in any failing job's
log; an inline annotation saves a click but is not a 10x improvement
like the lint-job case is.
2. `docs/_plugins/build_api_docs.rb` PR-scope hazard scan -- reverted. The
single registered pattern (block-form `@inheritDoc`) was hypothesised
as the v1-v2 unidoc crash trigger and turned out to be wrong (the
block->inline fix did not green doc gen). The scan also only fires on
a very specific failure mode (the `Building tree` predates-HTML branch
of `diagnose_unidoc_failure`). With one speculative pattern and a
narrow trigger condition, the framework is more aspirational than
actionable. SPARK-56630's existing diagnostic stays untouched on master.
3. Validation commit (`Aggregator.scala` planted scalastyle violation) --
reverted. End-to-end annotation rendering was already confirmed on a
prior CI run; the violation has served its purpose.
Final PR shape after this trim:
Part A: structural decoupling of style from compile (SBT + Maven).
Part B: `dev/scalastyle` annotations only (the high-value piece -- makes
the lint job's "exit 1" actionable as inline file:line on the
PR's Files changed tab).
Co-authored-by: Isaac
…ale env var, refresh comment
Three small follow-ups on top of the decoupling commit:
1. `project/SparkBuild.scala`: the previous commit removed
`val noLintOnCompile` but missed a second use site at line 403
(gating `Checkstyle.settings`), so the SBT build failed to load
("not found: value noLintOnCompile"). Drop the conditional and
always include `Checkstyle.settings` -- consistent with how the
same commit always installs `enableScalaStyle`. Verified locally
with `build/sbt show name`.
2. `dev/make-distribution.sh`: drop `export NOLINT_ON_COMPILE=1` --
no-op now that the variable that read it is gone, and consistent
with the env-var cleanup the workflow already does.
3. `dev/scalastyle`: refresh the format-(b) comment. The previous
wording attributed the `[error] <path>:<line>: <msg>` format to
the `scalaStyleOnCompile` hook attached to `Compile / compile`,
but that hook is removed in the decoupling commit. Re-attribute
to the explicit `scalastyle` / `test:scalastyle` tasks that
`dev/scalastyle` actually invokes -- both the previous compile
hook and these explicit tasks go through the same
`streams.value.log.error(...)` path, so the format is unchanged;
only the citation needed updating.
Co-authored-by: Isaac
…pling end-to-end Adds a 182-char comment line in Aggregator.scala to trigger scalastyle's LineLengthChecker. Two things to confirm on this PR's CI run: 1. Decoupling: ONLY the Linters job should fail. Build modules, Java 17/25 Maven, Documentation generation, sparkr, Docker integration tests, and TPC-DS should all stay green -- they no longer have scalastyle attached to (Compile / compile), so a style violation in catalyst (or here, in core, which they all transitively depend on) does not abort their compile. 2. Annotation emitter: the lint job's failure should surface as an inline ::error annotation on the PR's Files-changed tab pointing at line 31 of core/src/main/scala/org/apache/spark/Aggregator.scala. Will be reverted before merge; this commit must not land on master. Co-authored-by: Isaac
HyukjinKwon
approved these changes
Apr 28, 2026
…fy decoupling end-to-end" This reverts commit 4e1f1fe.
Contributor
Author
|
End-to-end validation on this branch (since reverted): pushed a deliberate 182-char comment in
(One unrelated flake on |
…doc diagnostic + document doc-gen debug tips Two related improvements for unidoc-failure debugging, learned the hard way on PR apache#51419: 1. `docs/_plugins/build_api_docs.rb` -- extend the existing `diagnose_unidoc_failure` (added by SPARK-56630) with a primary "filter and surface" pass: pick out the `[error]` lines whose path is NOT under `target/java/...` and print them prominently in the diagnostic banner. The genjavadoc-stub errors (intentionally non-fatal via `--ignore-source-errors`) are filtered out; what's left is almost always 0-3 lines, and they're typically the doclint violations that actually caused exit 1 (heading-out-of-sequence, malformed @link, etc). Also softens the previous "Javadoc crashed while generating: X.html" message -- it was assertive about a "crash mid-generation" when the doclet had often actually finished all HTML output and only returned non-zero because of error count. Reword to "may be where javadoc crashed mid-generation, OR may just be the last class processed before a non-zero exit driven by error count." 2. `AGENTS.md` -- add a "Doc-gen (unidoc) failures" subsection under "Investigating PR CI Failures" explaining: (a) the genjavadoc-stub noise that looks fatal but isn't, (b) the misleading `Building tree -> exit 1` shape that's not actually a tree-builder crash, and (c) JVM-level instrumentation (`-J-Xlog:exceptions=info`, `class+load=info`, javadoc `-verbose`) as fallback only -- the visible exception traces are usually JDK-internal noise, and the `CompletionFailure` storms we'd see are symptoms of unrelated downstream tooling (scaladoc's internal javac) running after the main javadoc has completed. The diagnostic improvement would have surfaced the `RelationCatalog.java:36: error: heading used out of sequence` line that caused PR apache#51419's failure directly in the CI banner; the AGENTS.md doc captures the gotchas so the next agent doesn't chase the same red herrings. Co-authored-by: Isaac
…tic banner The previous follow-up added both an in-banner filter (in `build_api_docs.rb`) and an AGENTS.md subsection explaining the same gotchas. With the banner now printing the actual fatal `[error]` lines directly (with a one-line note about doclint as the most common cause), the AGENTS.md text is redundant -- agents debugging a unidoc failure just read the banner and have what they need. Keep the banner improvement; drop the docs paragraph. Co-authored-by: Isaac
…erify unidoc banner Plants `<h3>Validation</h3>` in the class-level Javadoc of `ColumnarMap.java` -- a small, isolated file -- to trigger the same heading-out-of-sequence doclint error that hit `RelationCatalog.java` on PR apache#51419. javadoc 17 reports this as ColumnarMap.java:26: error: heading used out of sequence: <H3>, compared to implicit preceding heading: <H1> with `--ignore-source-errors` having no effect (it's a doclint check, not a source error). The Documentation generation job's diagnostic banner from `docs/_plugins/build_api_docs.rb` should now surface this line under "Likely fatal error(s) (genjavadoc stubs filtered out):" directly, instead of leaving it buried among ~100 genjavadoc-stub errors as before. Will be reverted before merge once the banner output is confirmed. Co-authored-by: Isaac
…n validation prose
Two fixes uncovered by the earlier validation push (`11170ea06d9`):
1. `docs/_plugins/build_api_docs.rb`: the previous regex was too loose and
too narrow at the same time. It matched sbt stack-trace lines like
[error] \tat java.base/...FutureTask.run(FutureTask.java:264)
because they contain `\.java:\d+`, while missing the actual user-source
doclint violations because sbt classifies them under `[warn]` rather
than `[error]`. The validation run on `11170ea06d9` showed both
problems: the banner printed five stack-trace lines under "Likely
fatal error(s)" and missed the planted `ColumnarMap.java` heading
violation entirely.
New regex requires:
- `[error]` OR `[warn]` prefix (sbt classifies inconsistently).
- `path/X.java:LINE:` with a colon AFTER the line number, which is
how doclint diagnostics are formatted; sbt stack traces use
`(File.java:LINE)` (paren-wrapped) and so don't match.
- Path NOT under `/target/java/` (genjavadoc stubs).
- Message NOT "Could not find any member to link" (benign scaladoc
warning, not what causes exit 1).
Unit-checked locally against 10 representative samples (3 must match,
7 must reject); all 10 classify correctly.
2. `sql/catalyst/.../ColumnarMap.java`: drop the literal `<h3>` tokens
from the validation commit's prose. The previous version contained
"Deliberate heading-out-of-sequence violation: <h3> directly under
the..." and javadoc parsed those literal tags in the prose as
additional unclosed headings, so the planted violation surfaced as
`unbalanced or unclosed heading` rather than the
`heading-out-of-sequence` shape we wanted to reproduce. Rewriting the
prose without literal tags lets doclint see exactly one violation
from the `<h3>...</h3>` heading at the wrong level.
This commit is staged on top of the earlier validation; once CI confirms
the diagnostic banner now prints the ColumnarMap line directly under
"Likely fatal error(s) (genjavadoc stubs filtered out)", the validation
commit (`11170ea06d9`) plus this one will both be reverted before merge.
Co-authored-by: Isaac
… compile-time doclint and stub-line log filter
Pivots the unidoc-failure diagnostic strategy from post-hoc log
forensics (a Ruby banner that grepped the captured sbt log for
`[error] path/X.java:LINE:` lines and explained what to look for)
to two simpler, deterministic moves plus a log-hygiene filter.
Move A -- doclint at compile time. `-Xdoclint:all -Xdoclint:-missing`
moves from `(Compile / doc / javacOptions)` up to `(Compile /
javacOptions)`, so every javac invocation runs the same checks the
`doc` task runs. Heading-out-of-sequence, broken `{@link}`,
malformed `@param` and similar issues in real `.java` sources fail
the compile step with a native javac diagnostic
(`X.java:LINE: error: ...`), well before unidoc runs and well
before the genjavadoc-stub completion cascade can swallow per-file
output. The original v1-v2 failure (`RelationCatalog.java:36:
heading used out of sequence: <H3>`) would have been caught here.
Move B -- doclint inside unidoc. `JavaUnidoc / unidoc /
javacOptions` uses `:=` (assign), which blocks scope inheritance,
so the same `-Xdoclint:all -Xdoclint:-missing` flags are listed
explicitly inside the unidoc option list. Catches doclint
violations originating in Scala doc comments, where genjavadoc
emits the heading verbatim into the stub at
`<module>/target/java/...`.
Move C -- suppress genjavadoc-stub diagnostic noise from CI stdout.
javadoc emits ~3500 `[error]` lines per unidoc run on stubs under
`target/java/` (`cannot find symbol` / `illegal combination of
modifiers` on stub `T1, T2, ...` type variables) -- all benign
because `--ignore-source-errors` is set, but they bury everything
else. `build_api_docs.rb` now wraps the unidoc invocation with a
small inline state machine that drops the stub header line plus
its `[error|warn]`-prefixed continuations (snippet, caret,
symbol/location) from stdout. Real `src/main/java/` diagnostics
are unaffected.
The previous follow-up's banner (`diagnose_unidoc_failure`) and
log-file capture helper (`stream_and_capture`) are removed: with
A+B+C, the visible CI log is small enough that the cause of any
remaining failure is one Ctrl-F away. The banner mainly extracted
information already on screen, and its regex proved fragile across
sbt/javadoc output variants (matched stack traces, missed `[warn]`
classification, etc.). The validation prose planted in
`ColumnarMap.java` for the previous regex test is reverted.
Co-authored-by: Isaac
…erify Move B + C
Plants `<h3>Validation</h3>` directly under the implicit class title
in `core/src/main/scala/org/apache/spark/Partition.scala`. With
Move A+B+C in place, the expected CI shape is:
- Compile passes (Scala compile does not enforce javadoc-style
doclint).
- genjavadoc emits `core/target/java/.../Partition.java` with the
heading preserved verbatim from the Scaladoc comment.
- Move B's `-Xdoclint:all` on `JavaUnidoc / unidoc / javacOptions`
runs doclint against the stub during unidoc, which should fail
with `Partition.java:LINE: error: heading used out of sequence:
<H3>, ...`.
- Move C suppresses the ~3500 benign genjavadoc-stub `[error]`
blocks from CI stdout, so the visible log should be markedly
shorter than the previous validation runs and the surviving
diagnostic should be easy to spot.
A separate follow-up validation will plant the same shape in a real
`.java` source (`ColumnarMap.java`) to verify Move A catches it at
compile time. Both validation commits will be reverted before merge.
Co-authored-by: Isaac
…d by compile-time -Xdoclint
Two real Java doc issues that hid because `Compile / doc / javacOptions`
was never run in CI (only `sbt compile` and `sbt unidoc` are; the `doc`
task wasn't in the standard build path). Move A's lift of `-Xdoclint:all
-Xdoclint:-missing` from `Compile / doc / javacOptions` to `Compile /
javacOptions` exposes them at compile time.
`launcher/.../LauncherServer.java`: the class-level Javadoc contains an
ASCII-art architecture diagram with `<-`, `<--`, and `<per-app channel>`
tokens that doclint reads as malformed HTML tags. Escape `<` as `<`
and `>` as `>` on the three offending lines. Rendering is unchanged
(the diagram already only "works" when reading the source -- it's not
wrapped in `<pre>`, so HTML rendering has always collapsed whitespace).
`common/utils-java/.../QueryContext.java`: `{@link SparkThrowable}` --
`SparkThrowable` lives in the `common/utils` module which is not a
dependency of `common/utils-java`, so doclint cannot resolve the
reference at this module's compile classpath. Switch to `{@code
SparkThrowable}` -- the docs still mention the type, just without an
auto-generated hyperlink that would have been broken anyway.
These two fixes were uncovered by the validation push on `79c12210f0d`,
where compile aborted at these files before unidoc could run; with them
fixed, validation of Moves B (doclint inside unidoc against genjavadoc
stubs) and C (stub-line filter) can proceed.
Co-authored-by: Isaac
… <pre>{@code} instead of escaping
Previous commit's `<`/`>` escapes shifted source columns by 3 chars
each (4 chars `<` vs 1 char `<`), breaking the ASCII art's visual
alignment when reading the source. Wrap the whole diagram in
`<pre>{@code ... }</pre>` instead: `{@code}` tells doclint to treat
contents as literal (no HTML parsing, so `<` is fine), and `<pre>`
preserves whitespace in the rendered HTML output. Source alignment is
fully restored, and as a bonus the diagram now renders correctly in
the generated Javadoc instead of having all whitespace collapsed.
Co-authored-by: Isaac
…etwork-common, variant, kvstore)
Continuing the cleanup uncovered by Move A. Compile on `74a376046fa`
aborted at four more files past LauncherServer/QueryContext:
- `network-common/.../SaslEncryption.java:170`: `<code>transferred() <
count()</code>` -- doclint reads the `<` inside `<code>` as a tag.
Switch to `{@code transferred() < count()}` (literal, no HTML
parsing).
- `variant/.../Variant.java:41`: `{@link
org.apache.spark.unsafe.types.VariantVal}` -- `common/variant` does
not depend on `common/unsafe`, so the reference cannot be resolved
on this module's classpath. Switch to `{@code ...}`; the type is
still mentioned, just without an auto-generated hyperlink that
would have been broken.
- `kvstore/.../LevelDBTypeInfo.java:69-70` and
`kvstore/.../RocksDBTypeInfo.java:71-72`: prose references to the
literal patterns `"+<something>"` / `"-<something>"`. doclint reads
`<something>` as an unknown HTML tag. Switch to
`{@code +<something>}` / `{@code -<something>}`; renders as code
style which is more semantically accurate anyway.
Co-authored-by: Isaac
…; revert non-public fixes
Earlier follow-ups (`c4e9d1475db`, `74a376046fa`, `81964b682cd`) treated
every doclint error surfaced by Move A as real debt to fix. Most of
them were spurious: they're internal/package-private classes that
unidoc never documented (because unidoc passes `-public`) and so were
never linted before.
Root cause of the difference:
- `JavaUnidoc / unidoc / javacOptions` passes `-public`. Javadoc's
`-public` restricts both the documented set AND doclint's check-set
to public APIs. Internal classes are filtered out before doclint
runs.
- `Compile / javacOptions` (Move A) runs javac, which has no `-public`
flag. `-Xdoclint:all` runs on every class javac compiles --
including package-private and private internals that were never
documented.
Fix: switch both Move A and Move B to `-Xdoclint:all/public
-Xdoclint:-missing/public`. The `/public` access modifier scopes
doclint to the public API surface only, matching what unidoc
documents and lints. Now the compile-time check covers exactly the
classes that should be doc-clean (because they ship in the published
Javadoc), and skips internal helpers that were never on the doc
surface.
Reverts non-public file fixes (back to `upstream/master`):
- `launcher/.../LauncherServer.java` -- package-private `class`.
- `network-common/.../SaslEncryption.java` -- package-private.
- `kvstore/.../LevelDBTypeInfo.java`, `RocksDBTypeInfo.java` --
package-private.
Keeps public-API fixes:
- `utils-java/.../QueryContext.java` -- public interface; `{@link
SparkThrowable}` -> `{@code ...}` because `common/utils-java` does
not depend on `common/utils`.
- `variant/.../Variant.java` -- public final class; `{@link
...VariantVal}` -> `{@code ...}` (cross-module).
Adds a new public-API fix:
- `unsafe/.../VariantVal.java` -- public class; four `{@link
org.apache.spark.sql.catalyst...}` references that point at
`sql/catalyst` types not on `common/unsafe`'s classpath; switch
all four to `{@code ...}` for the same reason.
Co-authored-by: Isaac
…roup; revert reference-only fixes
Continued from `855fcfebcb4`'s scope-to-/public adjustment.
Even with `/public`, javac's doclint catches every Java-`public`
class regardless of package; Spark filters its documented surface
further via `ignoreUndocumentedPackages` (`SparkBuild.scala:1613`),
which excludes `org/apache/spark/unsafe/*` (except `CalendarInterval`),
`org/apache/spark/types/variant`, and a long list of others. So
`VariantVal` and `Variant`, despite being `public class`, are not on
the documented surface; the cross-module `{@link}` errors that fired
on them at compile time are noise from the documentation perspective.
Rather than ratchet through fixes for classes the docs never expose,
narrow Move A to the `html` doclint group only:
-Xdoclint:html/public
This catches the actual original v1-v2 motivation
(heading-out-of-sequence `<H3>` after implicit `<H1>` in
`RelationCatalog.java`) and other malformed-HTML issues, while:
- Skipping `reference` group: avoids cross-module
`{@link}` failures that only fire on the per-module narrow compile
classpath. Unidoc itself runs reference checks against the
aggregated classpath where these references resolve cleanly.
- Skipping `syntax` group: avoids `unknown tag: <something>` noise on
prose containing literal `<x>` that's perfectly fine for the docs
(`LevelDBTypeInfo`, `RocksDBTypeInfo` had this).
Reverts the three reference-group fixes from earlier follow-ups:
- `utils-java/.../QueryContext.java`
- `variant/.../Variant.java`
- `unsafe/.../VariantVal.java`
The references resolve at unidoc time when the full aggregated
classpath is available, so Move B (unidoc-time `-Xdoclint:all/public
-Xdoclint:-missing/public`) catches any genuine doc bugs there
without these spurious classpath misses.
Co-authored-by: Isaac
…RRAY<INT>> in {@code}
Move A's `-Xdoclint:html/public` caught the generated
`SqlBaseLexer.java:464:46: error: unknown tag: INT` -- the comment
in `SqlBaseLexer.g4` line 106 contains `MAP<INT, ARRAY<INT>>` as
prose, and antlr emits it verbatim into the generated lexer's
javadoc, where doclint parses `<INT>` as an unknown HTML tag.
Fix at the grammar source: wrap the example in `{@code ...}` so
doclint treats it as literal code (no HTML parsing). antlr will
regenerate the lexer with the wrapped comment on the next compile.
This is the only `html`-group violation Move A surfaces on the
documented surface today.
Co-authored-by: Isaac
… with <p> Move A's `-Xdoclint:html/public` caught `XXH64.java:28:4: error: self-closing element not allowed`. The class-level Javadoc uses `<p/>` (self-closing); HTML5 / doclint require `<p>` (without slash). Fix is a single-character drop. Co-authored-by: Isaac
…evert source fixes that only existed for it
Move A's `Compile / javacOptions += -Xdoclint:html/public` could not
match unidoc's documented surface. javac's doclint scope filters by
access modifier (public/protected/...), but Spark's documented surface
is "public class AND in a path not filtered by
`ignoreUndocumentedPackages`" (`SparkBuild.scala:1615`). The filter
list now excludes ~30 packages including `sql/catalyst`, `sql/connect`,
`sql/classic`, `sql/execution`, `sql/internal`, `sql/scripting`,
`sql/ml`, `sql/hive`, `unsafe/*` (except `CalendarInterval`),
`network/`, `rpc/`, `executor/`, `memory/`, `internal/`, etc. -- so
many Java-`public` classes never reach unidoc, yet javac's `/public`
scope still lints them.
The mismatch surfaced as an unbounded fix-and-retry: each failing CI
run revealed one or two more `public class` files in undocumented
packages (`XXH64.java`, `SqlBaseLexer.java`, etc.), each requiring a
fix in code that doesn't appear in any published Javadoc. Because
modules contain both documented and undocumented files in the same
compile unit (e.g. `catalyst` has documented `RelationCatalog` and
undocumented `XXH64`), per-module javacOptions overrides can't separate
them.
Drop Move A and the source fixes that only existed to satisfy it:
- Restore `(Compile / doc / javacOptions)` block to its pre-PR shape.
- Drop `-Xdoclint:html/public` from `(Compile / javacOptions)`.
- Revert `XXH64.java` (`<p/>` -> `<p>`): catalyst is undocumented.
- Revert `SqlBaseLexer.g4` (`MAP<INT, ARRAY<INT>>` wrapping in
`{@code}`): the generated lexer is in undocumented `sql/api/parser`.
Move B (`JavaUnidoc / unidoc / javacOptions += -Xdoclint:all/public
-Xdoclint:-missing/public`) and Move C (genjavadoc-stub log filter
in `build_api_docs.rb`) remain. Unidoc applies
`ignoreUndocumentedPackages` to its source set first, so doclint there
operates on exactly the documented surface -- no scope mismatch.
Co-authored-by: Isaac
…avacOptions doclint flags The earlier comment said the flags "Mirror Compile / javacOptions doclint settings". After dropping compile-time doclint, that mirror no longer exists, so the comment was misleading. Rewrite to describe what the flags do and why the `/public` scope matches the surrounding `-public` flag (post-`ignoreUndocumentedPackages` filter). Co-authored-by: Isaac
…aUnidoc -Xdoclint -- javadoc rejects it
CI surfaced:
error: Access qualifiers not permitted for -Xdoclint arguments
The `/access` modifier on `-Xdoclint` is a javac-only syntax. javadoc
17 rejects it and exits 2 before processing any source. The earlier
`/public` scoping addition was based on misreading -- access filtering
in javadoc already comes from the `-public` flag (already set on this
options block) plus `ignoreUndocumentedPackages` applied to the source
set, so the per-flag access modifier is both invalid and unnecessary.
Restore the original form that the standalone `Compile / doc /
javacOptions` block uses successfully:
-Xdoclint:all -Xdoclint:-missing
Co-authored-by: Isaac
…ge text, not just path
The previous filter matched any `[error|warn] /...target/java/...\.java:LINE:`
line, suppressing every diagnostic on a stub path. That hid legitimate
doclint diagnostics too: when a Scala doc comment has a violation
(e.g. heading-out-of-sequence, malformed HTML), genjavadoc preserves
it into the stub at `<module>/target/java/...`, and javadoc emits the
diagnostic with that stub path. The path matched the suppression
filter, so the actual signal we wanted from unidoc-time doclint was
hidden alongside the noise.
Tighten the filter to discriminate by message text: only suppress
the three known-benign genjavadoc structural errors:
- `error: cannot find symbol` (stubs reference type variables `T1, T2,
...` outside their declaring scope)
- `error: illegal combination of modifiers` (stubs declare `abstract
static` methods)
- `error: non-static type variable` (same root cause, different
diagnostic)
Anything else on a `target/java/` path is echoed -- including doclint
diagnostics like `error: heading used out of sequence`, `error:
malformed HTML`, `error: self-closing element not allowed`, etc.
Sanity-checked locally against 8 representative samples (3 must
match, 5 must reject including doclint-on-stub-path cases); all
classify correctly.
Co-authored-by: Isaac
…; reference group bails on genjavadoc stubs
Latest validation run on `97057772b6a` showed `JavadocGenerationFailed`
without any per-file diagnostic visible. Tracing the log:
[error] /__w/spark/spark/connector/kinesis-asl/target/java/.../KinesisBackedBlockRDD.java:6:87:
error: BlockRDD is not public in org.apache.spark.rdd; cannot be accessed from outside package
[error] /__w/spark/spark/core/target/java/.../TaskContext.java:126:83:
error: Task is not public in org.apache.spark.scheduler; cannot be accessed from outside package
... 28 such errors, all on genjavadoc-stub paths under target/java/ ...
[error] sbt.inc.Doc$JavadocGenerationFailed
These are `reference`-group doclint errors fired by `-Xdoclint:all`:
genjavadoc generates Java stubs that reference package-private Scala
types from outside their package, and the `reference` group enforces
those access rules. `--ignore-source-errors` does NOT suppress them.
javadoc bails on the first batch before reaching the planted heading
violation in `Partition.java`, so doclint's `html` checks (the actual
target) never run.
Master's doc gen passes because no `-Xdoclint` flag is passed to
unidoc -- javadoc's behavior without explicit `-Xdoclint` is more
lenient about these access errors than `-Xdoclint:all`.
Narrow Move B from `-Xdoclint:all -Xdoclint:-missing` to
`-Xdoclint:html` only. This:
- Catches the original failure class (heading-out-of-sequence
`<H3>` after implicit `<H1>` on `RelationCatalog.java`) -- in the
`html` group.
- Catches malformed HTML, self-closing elements, unknown tags --
all `html` group.
- Skips `reference` group: avoids the genjavadoc stub access-error
bail-out. We lose `{@link}` resolution checks, but those would
fire on real source paths and are easy to spot; they're not the
silent-fail mode this PR was about.
- Skips `syntax` group: avoids `unknown tag: <X>` noise on prose
that happens to contain literal `<X>` (kvstore had this).
- Skips `accessibility` and `missing` groups: not relevant here.
Co-authored-by: Isaac
…source to real .java
Previous validation planted `<h3>` in `Partition.scala`. Doc gen on
`d6dfb63bdc7` succeeded -- the heading was never detected. Likely
reason: genjavadoc's translation of Scala doc comments into the
`target/java/` stub goes through a path that doclint's `html` checks
don't reach (the comment may be stripped or the stub flagged for
relaxed handling). Either way, that's not the case Move B was
designed for.
Move B's actual scope is heading-out-of-sequence (and similar `html`
group violations) in **real `.java` sources** -- the case that
originally motivated this PR (`RelationCatalog.java:36` on PR
51419). Replant the violation in `ColumnarMap.java` to match.
Expected next-run shape:
- Compile passes (Move A is gone).
- JavaUnidoc fails with a clean
`ColumnarMap.java:LINE: error: heading used out of sequence: <H3>`
on stdout, surviving the genjavadoc-stub-noise filter (Move C).
- Visible CI log scannable.
Will be reverted before merge.
Co-authored-by: Isaac
… appears unfiltered
Validation run on `4a5d47b4feb` showed unidoc fails with the planted
heading in `ColumnarMap.java`, but the diagnostic line itself
(`ColumnarMap.java:LINE: error: heading used out of sequence`) is
invisible in stdout. Two hypotheses:
1. Move C's filter is suppressing it incorrectly.
2. javadoc isn't emitting it at all (or emits to a stream we don't
capture).
Temporarily replace the filtered streamer with a plain pass-through
to test (1). If the diagnostic appears with the filter off, the
filter has a bug. If it doesn't, the issue is upstream and we need
a different approach (custom doclet, separate javadoc invocation,
etc.).
Will be reverted regardless of outcome.
Co-authored-by: Isaac
…oc diagnostics to stdout
Attempt to surface doclint diagnostics that javadoc 17 was dropping
on its way to `exit 1` (the failure surfaced as a generic
`javadoc exited with exit code 1` with no file/line context, even
when the build clearly failed because of a heading-out-of-sequence
violation in a real `.java` source).
The doclet (`tools/src/main/java/org/apache/spark/tools/SparkUnidocDoclet.java`)
extends `jdk.javadoc.doclet.StandardDoclet` and wraps the `Reporter`
passed to `init()`. Every `Reporter.print()` overload is overridden to
mirror the diagnostic to stdout with a `[unidoc-doclet]` prefix
BEFORE delegating to the original reporter. Whatever javadoc does
with the diagnostic afterwards (drop, queue, format), our copy is
already on the wire.
Wired up in `JavaUnidoc / unidoc / javacOptions`:
-doclet org.apache.spark.tools.SparkUnidocDoclet
-docletpath <tools/Compile/classDirectory>
The `(LocalProject("tools") / Compile / compile).value` call inside
the option block creates the build-graph dependency so the doclet
class is compiled before unidoc runs.
Move C's stub-line filter is also extended to recognize the
`X is not public in <package>; cannot be accessed from outside
package` shape (~30 of these per run). Together with the existing
three patterns, that covers the full known set of genjavadoc-stub
structural noise. Diagnostic mirror lines from the doclet use the
`[unidoc-doclet]` prefix and don't match either filter regex, so
they always pass through.
Reverts the debug bypass from `2fa7629cae4`.
Co-authored-by: Isaac
… present in JDK 17 Reporter
Compile failure on `4eebc14d5f6`:
SparkUnidocDoclet.java:86: error: method does not override or implement a method from a supertype
SparkUnidocDoclet.java:90: error: incompatible types: DocTreePath cannot be converted to FileObject
The Reporter overloads with `(Kind, DocTreePath, int, int, int, String)`,
`(Kind, FileObject, int, int, int, String)`, `getStandardWriter()`,
and `getDiagnosticWriter()` were added in JDK 18. Spark targets JDK
17, so the JDK 17 Reporter has only the three core `print` overloads
(`Kind`+`String`, `Kind`+`DocTreePath`+`String`, `Kind`+`Element`+`String`).
Drop the four extra overrides and their now-unused imports
(`FileObject`, `PrintWriter`). The mirror still covers every
diagnostic emission path on JDK 17.
Co-authored-by: Isaac
…rdWriter/getDiagnosticWriter
Latest validation showed the doclet IS invoked (`[unidoc-doclet]`
prefix surfaces), but immediately hits:
[unidoc-doclet] ERROR: An internal exception has occurred.
java.lang.NullPointerException: Cannot invoke
"java.io.PrintWriter.println(String)" because the return value
of "jdk.javadoc.doclet.Reporter.getDiagnosticWriter()" is null
JDK 17 Reporter has `getStandardWriter()` and `getDiagnosticWriter()`
as `default` methods returning null. The standard doclet calls them on
whatever Reporter we install; without overrides, our MirrorReporter
inherits the null-returning defaults. The actual reporter passed to
init() (which we wrap) has the real PrintWriters; we just need to
delegate.
(The previous commit dropped these along with the JDK 18+ overloads
because all of them were grouped together as "extra overrides"; only
the int-int-int and FileObject ones were JDK 18+, the writer
accessors are JDK 17.)
Co-authored-by: Isaac
…iter/getDiagnosticWriter writers, not just delegate Previous run with writer delegation succeeded (no NPE) but emitted no `[unidoc-doclet]` lines for the planted heading violation. The standard doclet emits per-file diagnostics through these writers, not through the `Reporter.print()` overloads -- so just delegating the writer gives the real writer back to the doclet, which writes to it directly, bypassing our mirror. Wrap each writer in a `MirrorWriter` (extends `PrintWriter`) that overrides `write(String, int, int)` and `write(char[], int, int)` to mirror to `System.out` with a `[unidoc-doclet diag]` / `[unidoc-doclet stdout]` prefix on each newline-terminated chunk before forwarding to the underlying writer. PrintWriter's `print/println/printf` paths all funnel through these `write` overloads, so we catch every emission without overriding every overload individually. Internal buffering (via a StringBuilder) handles partial writes: the doclet may emit a diagnostic in multiple `write` calls before the trailing newline, so we accumulate until we see `\n`, emit one mirrored line per newline, and keep the unfinished tail. Co-authored-by: Isaac
…ope PR to scalastyle decoupling + log-hygiene PR apache#55581 (apache#55581) identified the real root cause of the silent unidoc-failure problem we were trying to solve in section 3 of this PR: javadoc's default `-Xmaxerrs 100` caps error reporting at 100. Spark's genjavadoc stubs alone produce ~100 inert "is not public" / "cannot find symbol" errors before any real source is processed, so javadoc bails before reaching the diagnostics we actually care about (heading-out-of-sequence, broken `{@link}`, etc.). `-Xmaxwarns 100` similarly clips the per-link output once HTML generation completes. None of our `-Xdoclint:html`, custom-doclet, or wrapping-Reporter approaches could reach past that cap -- we were layering interception on top of a pipeline that had already exited. PR apache#55581's fix is much simpler: bump `-Xmaxerrs` and `-Xmaxwarns` to 999999, add `-verbose`, and extend the SPARK-56630 banner to scan for `error: reference not found`. Together they pinpoint the broken `{@link}` in a way the doclet API simply could not. This commit therefore drops the doc-gen experiments from this PR so we don't conflict with apache#55581: - `project/SparkBuild.scala`: drop `-Xdoclint:html`, `-doclet org.apache.spark.tools.SparkUnidocDoclet`, `-docletpath ...`, and the `tools / Compile / compile.value` task-graph hook from `JavaUnidoc / unidoc / javacOptions`. Restored to its pre-this-PR shape. - `tools/src/main/java/org/apache/spark/tools/SparkUnidocDoclet.java`: removed entirely. The doclet wrapper cannot reach diagnostics that flow through `Log` / `DiagnosticListener` rather than the `Reporter` API. - `sql/catalyst/.../ColumnarMap.java`: revert validation plant. What this PR still delivers: - **Sections 1 + 2** (scalastyle decoupling and inline annotations) are unchanged and continue to deliver real wins. - **Move C** (genjavadoc-stub log filter in `docs/_plugins/build_api_docs.rb`) stays. It is independent log hygiene that drops ~3500 inert `[error]` lines from the CI log on every unidoc run, complementary to apache#55581. Co-authored-by: Isaac
Contributor
Author
|
the test timeout is unrelated, thanks for review, merging to master! |
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
2 participants
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.
What changes were proposed in this pull request?
Three related changes that fix the most common CI debug pain on this project: a single root cause cascading into many red checks, each surfacing only "Process completed with exit code 1" with no file:line.
1. Structural decoupling — style is no longer attached to compile.
Today, scalastyle runs as a side effect of
(Compile / compile)in SBT and as the default<execution>ofscalastyle-maven-pluginbound to the Mavenverifyphase. A single style violation incoreaborts the compile ofcoreand every transitive dependent, so every CI job that recompiles those sources fails — Build modules, Documentation generation, Java 17/25 Maven build, sparkr, Docker integration tests, TPC-DS — each with no file/line in its annotation. The dedicated lint job is just one of seven jobs that fail; nothing tells the user it's the root cause.This PR removes the coupling at both build systems:
project/SparkBuild.scala: drop the(Compile / compile) := { scalaStyleOnCompile.value; … }(and matching(Test / compile)) hooks fromenableScalaStyle. The standalonescalaStyleOnCompile/scalaStyleOnTesttask definitions remain sodev/lint-scala→dev/scalastyle→sbt scalastyle test:scalastylecontinues to work; only the compile-time auto-invocation is removed.NOLINT_ON_COMPILEenv-var gating is dropped (no-op once the hook is gone).pom.xml: remove the default<execution>fromscalastyle-maven-pluginand add an opt-inscalastyleprofile that re-bindscheck. Default Maven builds (mvn install,mvn package, etc.) no longer trigger scalastyle; activate the profile (mvn ... -Pscalastyle) or invoke the goal directly (mvn scalastyle:check) to run it..github/workflows/build_and_test.yml: remove the now-no-opNOLINT_ON_COMPILEenv vars from Build modules / pyspark / sparkr / lint / docs jobs.After this change, scalastyle runs in exactly one place in CI: the dedicated lint job (via SBT through
dev/lint-scala). A style violation fails ONE check; every other CI job stays green because they don't see the violation. Noneeds: lintchaining is needed — the cascade is gone at the source. Style coverage is unchanged (the lint job still scans every Scala file with the same config).2. Surface scalastyle violations as inline PR annotations.
Even with the cascade removed, the lint job's natural output is verbose sbt output where the actual file:line of a violation is buried.
dev/scalastylenow parses scalastyle's output and re-emits each violation as a GitHub Actions::error file=...,line=...,title=Scalastyle::...annotation when running under CI. The violation appears inline on the PR's "Files changed" tab — no log scrolling needed.Two output formats are handled:
error file=<path> message=<text> line=<n> [column=<n>], used by the explicitscalastyle/test:scalastyletasks.[error] <path>:<line>: <message>, used whenTasks.doScalastyleis invoked through sbt's logger (the format we actually see in CI today).The two regex branches are precise enough to skip near-miss lines: sbt's "task failed" summaries, exception traces, and regular Scala compile errors with their
:LINE:COL:triple-colon shape are correctly not matched.3. Strip genjavadoc-stub diagnostic noise from the CI log.
docs/_plugins/build_api_docs.rbwraps thebuild/sbt unidocinvocation with a small inline state machine that drops genjavadoc-stub[error]blocks from CI stdout. javadoc emits ~3500 such lines per unidoc run (cannot find symbol/illegal combination of modifiers/non-static type variable/X is not public in org.apache.spark.Y; cannot be accessed from outside packageon stubT1, T2, ...type variables and stub references to package-private Scala types) — all benign because--ignore-source-errorsis set, but they bury everything else. Each diagnostic is a header line followed by 3–5[error|warn]-prefixed continuation lines (snippet, caret, symbol/location); the state machine drops both.The filter matches by message text, not just by
target/java/path, so legitimate doclint diagnostics on stub paths (e.g. a heading-out-of-sequence in a Scala doc comment that genjavadoc preserves into the stub) survive. Realsrc/main/java/diagnostics are untouched.Note: this PR previously also experimented with
-Xdoclint:htmland a custom doclet to surface the per-file diagnostic when unidoc fails. That work was dropped after PR #55581 (#55581) identified the real root cause: javadoc's default-Xmaxerrs 100caps error reporting at 100, so the diagnostics we wanted never reached our interception layer at all. PR #55581's-Xmaxerrs/-Xmaxwarnsbump and-verboseaddition is the right fix and is complementary to the log filter shipped here.Why are the changes needed?
A single scalastyle violation in catalyst recently caused 7 red checks at once, each surfacing only a generic "exit code 1" annotation; finding the actual file:line required grepping through a multi-megabyte job log. Decoupling collapses the style cascade to one red check; the scalastyle annotation makes that check actionable on the PR's Files-changed tab; the genjavadoc-stub log filter cuts ~3500 lines of inert noise from every unidoc run so the rest of the doc-gen output stays scannable.
Does this PR introduce any user-facing change?
No SQL or runtime changes. Two developer-facing changes:
mvn install(or any Maven invocation that hits theverifyphase) no longer runs scalastyle by default. Activate thescalastyleprofile (mvn ... -Pscalastyle) or invokemvn scalastyle:checkexplicitly. The lint job in CI continues to enforce style.sbt compileno longer triggers scalastyle. Runsbt scalastyle test:scalastyle(ordev/lint-scala) explicitly to check style locally; CI still enforces it.How was this patch tested?
:LINE:COL:shape) are correctly skipped.sbt scalastyle test:scalastyleexactly as before; the same scalastyle config (scalastyle-config.xml) governs which violations fire.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude (Anthropic)