holdenk opened a new pull request, #59899:
URL: https://github.com/apache/spark/pull/59899
### What changes were proposed in this pull request?
Two pre-push checks added to `dev/merge_spark_pr.py`, both offered
interactively before the push to the Apache remote and wired into `merge_pr`
and `_do_cherry_pick`:
1. **Optional clean SBT compile** (`maybe_compile_check`). Offers to run
`build/sbt <CI profiles> ;clean;compile;Test/compile` on the merged (or
cherry-picked) tree -- exactly what would be pushed -- so a mis-resolved
conflict or a PR that bit-rotted against the target branch after its CI ran is
caught before it lands. Declining skips the check; a build that does not come
back green asks before continuing rather than aborting on its own, because a
non-zero sbt is as easily a JDK the branch rejects or a failed download as it
is the backport. Build output streams to the terminal so a long compile does
not look like a hang.
The profile list is copied from the "Build Spark" step in
`.github/workflows/build_and_test.yml`, not curated: CI is what rejects the
push, so the check is worth only as much as it matches CI. Without the flags
SBT builds only its default projects, and a merge touching `resource-managers/`
would pass having never been compiled. `-Psbt` is deliberately *not* in the
list -- it would put guava and pmml-model on every module's compile classpath,
which CI does not, turning a CI failure into a local pass.
2. **Pre-4.2 `ConfigBindingPolicy` backport warning**
(`maybe_binding_policy_warning`). A merge or cherry-pick onto
`branch-4.1`/`4.0`/`3.x` that adds `.withBindingPolicy(...)` or
`ConfigBindingPolicy` will not compile there (the API arrived in SPARK-55928).
A git-diff + grep check scoped to `*.scala`/`*.java`, scanning only the lines
the change **added** (a leading `+` that is not the `+++ ` header) for
`\.withBindingPolicy\s*\(` and `\bConfigBindingPolicy\b`; on a hit it prints
the remedy and asks the committer to confirm. The diff is forced plain with
`--no-color --no-ext-diff`, since with `color.ui = always` no line starts with
`+` and the check would silently find nothing. A git failure only warns -- like
`branches_with_merge_footer`, this runs after `master` has already been pushed,
so it must not be the thing that aborts. No-op on branches that carry the API,
so `master`/`branch-4.x` are unaffected, and it runs before the compile check
(cheap grep before a slow build).
Declining either check inside `_do_cherry_pick` raises `SkipCherryPick`
rather than exiting, so an already-pushed `branch-M.x` pick and the JIRA
resolution survive; `continue_maybe` now attempts `cherry-pick --abort` only
when git says one is actually in progress.
### Why are the changes needed?
Both checks target breakage that is routine in the merge/backport flow but
easy to miss until after the push:
- A clean compile on the exact tree being pushed catches mis-resolved
conflicts and bit-rotted PRs that their own CI never exercised against the
current target branch.
- A backport that drags in the 4.2+ `ConfigBindingPolicy` API fails to
compile on a pre-4.2 branch. The remedy -- drop the `.withBindingPolicy(...)`
call and the `ConfigBindingPolicy` import -- is easy to forget during a routine
cherry-pick, and a targeted warning is cheaper than discovering it from a
failed backport build after the fact.
### Does this PR introduce _any_ user-facing change?
No. This is a committer-facing dev tool change only.
### How was this patch tested?
Used to merge https://github.com/apache/spark/pull/58901
Inline doctests cover both checks, run via `python3 -m doctest
dev/merge_spark_pr.py` -- 281 items pass (the whole file; this PR's additions
plus the existing suite): the compile check's offer/accept/green/red/override
paths plus `KeyboardInterrupt`, `OSError` and the cherry-pick skip; and the
binding-policy check's branch classification, added-token scanning, pinned git
invocation, and no-op / accept / decline-abort / decline-skip / git-failure
paths. `ruff check` and `ruff format --diff` pass.
Verified against a throwaway git repo rather than only against mocks: the
`*.scala`/`*.java` pathspec really does scope the diff (a `withBindingPolicy`
mention in `README.md` is excluded), the warning fires on `branch-4.1` and is
silent on `branch-4.2`, it still fires under `git config color.ui always`, and
`_cherry_pick_in_progress` reports `False`/`True`/`False` across a real
conflicting cherry-pick and its abort. Verified against the real build: all ten
profile-gated projects resolve under the new flag list (`spark-yarn` ...
`spark-kubernetes-integration-tests`), a profile a branch does not define is
ignored rather than fatal, and `test:` is confirmed deprecated in sbt 1.12.13
("sbt 0.13 shell syntax is deprecated; use slash syntax instead").
The binding-policy check is a no-op on `master`/`branch-4.x`, and the
compile check is opt-in (default skip), so the common merge/backport path is
unaffected when the committer declines.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor
Generated-by: Claude Code (Opus 4.8)
Generated-by: Cursor (GLM 5.2)
--
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]
