spark-test commented on code in PR #59899:
URL: https://github.com/apache/spark/pull/59899#discussion_r4408045771
##########
dev/merge_spark_pr.py:
##########
@@ -946,9 +1569,68 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref):
else:
print("Cherry-pick already completed manually; continuing with the
backport.")
- continue_maybe(
- "Pick complete (local ref %s). Push to %s?" % (pick_branch_name,
PUSH_REMOTE_NAME)
+ maybe_binding_policy_warning(pick_ref, pick_head, cherry=True)
+ maybe_compile_check(pick_branch_name, cherry=True)
+
+ choice = get_input(
+ "Pick complete (local ref %s). How should it land?\n"
+ " [d] Push directly to %s/%s\n"
+ " [p] Push to your fork (%s) and open a draft backport PR against %s
(for CI)\n"
+ " [s] Skip this branch (default)\n"
+ "Choose [d/p/s]: "
+ % (pick_branch_name, PUSH_REMOTE_NAME, pick_ref, FORK_REMOTE_NAME,
pick_ref),
+ {"d": ["d", "direct"], "p": ["p", "pr"], "s": ["s", "skip", ""]},
)
+ if choice == "s":
+ # Restore the original checkout and drop the temp PICK_PR_* branches
before skipping,
+ # as the continue_maybe decline path does. Otherwise this branch stays
checked out and a
+ # later pick of the same ref fails when `git fetch` tries to update
the checked-out branch.
+ print("Skipping this branch; the merge continues.")
+ clean_up()
+ raise SkipCherryPick()
+
+ if choice == "p":
+ if FORK_REMOTE_NAME == PUSH_REMOTE_NAME:
Review Comment:
**Non-blocking (P2):** This guard only fires when the two remote *names*
match. `FORK_REMOTE_NAME` defaults to `origin` (73), and `_fork_owner` returns
`'apache'` for both `https://github.com/apache/spark.git` and
`[email protected]:apache/spark.git` (I ran the pattern at 1496). With
`FORK_OWNER` set, it doesn't read the URL at all. So in a clone of apache/spark
whose `origin` was never repointed (the script's header asks only for the
`apache-github` and `apache` remotes), `[p]` runs `git push origin
...:backport-pr<N>-branch-4.2` and then
`create_pr(head='apache:backport-pr<N>-branch-4.2')`. For a committer with
write access, that creates a branch and a draft PR in apache/spark, starts a
full CI run there (`build_main.yml` runs on pushes to every branch), and mails
commits@. That is exactly what the guard's message says it prevents.
Suggested fix: before pushing, resolve the fork remote's URL into
host/owner/repo and refuse apache/spark, or any repository matching the push or
PR remote. Do this whether or not `FORK_OWNER` is set, since `FORK_OWNER` only
shapes the PR head, not where the push goes.
See **Shared repair plan 2** in the review body.
##########
dev/merge_spark_pr.py:
##########
@@ -1080,14 +1767,14 @@ def cherry_pick(pr_num, merge_hash, default_branch,
branch_names, target_ref, al
# reflect it.
picked = []
try:
- picked.append(_do_cherry_pick(pr_num, merge_hash, sibling_x))
- picked.append(_do_cherry_pick(pr_num, merge_hash, pick_ref))
+ picked.append(_do_cherry_pick(pr_num, merge_hash, sibling_x,
title=title))
+ picked.append(_do_cherry_pick(pr_num, merge_hash, pick_ref,
title=title))
Review Comment:
**Non-blocking (P2):** `[p]` returns `(ref, PR URL)` or `(ref, 'fork:...')`
through the same channel as a pushed pick, and `main()` still appends every
returned ref to `merged_refs`/`picked_refs` (3292, 3410), which become
`already_picked` (3289, 3407). So once branch-4.x has gone to a draft PR,
`_upstream_first_sibling('master', 'branch-4.2', ['branch-4.x', 'branch-4.2'],
('master', 'branch-4.x'))` returns `None` (I ran it). No Upstream-First prompt
appears, and `[d]` pushes branch-4.2 while branch-4.x has only an open draft
PR. The `[b]oth` path does the same within one prompt: this line runs right
after branch-4.x returned a pending marker, and its landing prompt offers
`[d]`. `cherry_pick`'s own contract says to prompt whenever branch-M.x "has not
already received this commit" (1724).
The result is that branch-M.N can carry a change that branch-M.x lacks. That
is permanent if the draft PR is abandoned, and if the PR lands after 4.3.0 is
cut, a 4.2.x release ships a fix that 4.3.0 lacks. JIRA is resolved from
`landed_refs` `[master, branch-4.2]` to `['5.0.0', '4.2.1']`, the
maintenance-only record an explicit `[o]nly` answer produces, although nobody
chose `[o]nly`. The landed/pending split this PR added only reaches the JIRA
input and the merge comment.
See **Shared repair plan 1** in the review body.
##########
dev/merge_spark_pr.py:
##########
@@ -901,12 +1462,74 @@ def merge_pr(pr_num, target_ref, title, body,
pr_repo_desc, pr_author, co_author
return merge_hash
-def _do_cherry_pick(pr_num, merge_hash, pick_ref):
+def _backport_pr_branch(pr_num, pick_ref):
+ """Fork branch name for a backport PR: stable, PR-traceable,
branch-suffixed.
+
+ >>> _backport_pr_branch(123, "branch-4.2")
+ 'backport-pr123-branch-4.2'
+ >>> _backport_pr_branch(9, "branch-3.5")
+ 'backport-pr9-branch-3.5'
+ """
+ return "backport-pr%s-%s" % (pr_num, pick_ref)
+
+
+def _backport_pr_title(pick_ref, title):
+ """PR title for a backport PR: the merged title tagged with its target
branch.
+
+ >>> _backport_pr_title("branch-4.2", "[SPARK-1234][SQL] Fix a thing")
+ '[BACKPORT-branch-4.2] [SPARK-1234][SQL] Fix a thing'
+ """
+ return "[BACKPORT-%s] %s" % (pick_ref, title)
Review Comment:
**Non-blocking (P2):** I ran the extracted `Title.parse` on generated
titles. `[BACKPORT-branch-4.2] [SPARK-1234][SQL] Fix a thing`,
`[BACKPORT-branch-4.2] [MINOR] Fix typo` and `[BACKPORT-branch-4.2] Revert
"..."` are all rejected with `title must start with [SPARK-NNNNN], [MINOR], or
[TRIVIAL]`, which `main()` turns into `fail('Malformed PR title: ...')`. The
Revert form also no longer matches `main()`'s `startswith('Revert "')` check,
so revert handling is lost as well. Every `[p]` PR therefore has to be retitled
by hand before the script will merge it. If someone squash-merges it on GitHub
instead (enabled in `.asf.yaml` with `PR_TITLE_AND_DESC`), the `[BACKPORT-...]`
subject becomes permanent branch history.
The parser already accepts a target-line marker: `[SPARK-1234][4.2][SQL] Fix
a thing` parses, and `main()` moves version tags to the front of the
components. That gives the same 'easy to spot' visibility. Revert/Reapply
titles could stay verbatim, with the target branch named in the body.
**Verification:**
- **Regression:** Titles generated from a SPARK-id original and a [MINOR]
original are accepted by the merge path's title parser, and a title generated
from a Revert original is still detected as a revert.
##########
dev/merge_spark_pr.py:
##########
@@ -946,9 +1569,68 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref):
else:
print("Cherry-pick already completed manually; continuing with the
backport.")
- continue_maybe(
- "Pick complete (local ref %s). Push to %s?" % (pick_branch_name,
PUSH_REMOTE_NAME)
+ maybe_binding_policy_warning(pick_ref, pick_head, cherry=True)
+ maybe_compile_check(pick_branch_name, cherry=True)
+
+ choice = get_input(
+ "Pick complete (local ref %s). How should it land?\n"
+ " [d] Push directly to %s/%s\n"
+ " [p] Push to your fork (%s) and open a draft backport PR against %s
(for CI)\n"
+ " [s] Skip this branch (default)\n"
+ "Choose [d/p/s]: "
+ % (pick_branch_name, PUSH_REMOTE_NAME, pick_ref, FORK_REMOTE_NAME,
pick_ref),
+ {"d": ["d", "direct"], "p": ["p", "pr"], "s": ["s", "skip", ""]},
)
+ if choice == "s":
+ # Restore the original checkout and drop the temp PICK_PR_* branches
before skipping,
+ # as the continue_maybe decline path does. Otherwise this branch stays
checked out and a
+ # later pick of the same ref fails when `git fetch` tries to update
the checked-out branch.
+ print("Skipping this branch; the merge continues.")
+ clean_up()
+ raise SkipCherryPick()
+
+ if choice == "p":
+ if FORK_REMOTE_NAME == PUSH_REMOTE_NAME:
+ fail(
+ "FORK_REMOTE_NAME (%s) must differ from PUSH_REMOTE_NAME (%s);
otherwise "
+ "the backport-PR path pushes straight to the Apache remote.
Set "
+ "FORK_REMOTE_NAME to your fork remote and retry."
+ % (FORK_REMOTE_NAME, PUSH_REMOTE_NAME)
+ )
+ backport_branch = _backport_pr_branch(pr_num, pick_ref)
+ owner = _fork_owner()
Review Comment:
**Nit (P3):** The checks at 1593-1601 run only after the pick, any manual
conflict resolution, the binding-policy prompt and the optional compile. Every
failure from there ends the whole run:
- `fail()` on `FORK_REMOTE_NAME == PUSH_REMOTE_NAME`;
- `fail()` from `_fork_owner` on a URL it can't parse, such as an SSH host
alias like `git@github-work:owner/spark.git`;
- an uncaught `CalledProcessError` from `git remote get-url` when the remote
doesn't exist (e.g. the fork remote is named `fork` and `FORK_REMOTE_NAME` is
left at `origin`);
- `fail()` on a rejected fork push;
- a `URLError` from `create_pr`, which only catches `HTTPError`.
Both `fail()` and the top-level `except BaseException` run `clean_up()`,
which runs `git branch -D` on the `PR_TOOL_PICK` branch holding the unpushed
pick, and the remaining branches are never offered. The PR's own comment at
1168-1170 names this exact harm for the compile check.
Suggested fix: check the fork target before anything is pushed. On any of
these failures, return to the landing prompt without `[p]`, so the committer
can still push directly or skip while the run continues. Once the fork push has
succeeded, handle any PR-creation error the way the existing HTTP-error path
does: record the `fork:` outcome and print the manual-PR hint.
**Verification:**
- **Regression:** With a missing fork remote, an unparsable fork URL or a
failing fork push, choosing the backport-PR route leaves the pick available for
a direct push or a skip, and the remaining branch prompts still run.
- **Behavior:** A URLError from PR creation after a successful fork push
records the fork-only outcome and prints the manual-PR instruction.
##########
dev/merge_spark_pr.py:
##########
@@ -111,6 +119,32 @@
SKIP_VERSION_CHECK = os.environ.get("SKIP_VERSION_CHECK", "")
# Path of this script relative to the repo root, used to fetch the canonical
copy from master.
MERGE_SCRIPT_REPO_PATH = "dev/merge_spark_pr.py"
+# Optional pre-push check offered by maybe_compile_check: a clean SBT compile
of the main and
+# test sources run on the merged tree, i.e. exactly what would be pushed. Slow
on a cold build,
+# but catches mis-resolved conflicts and PRs that bit-rotted after their CI
ran. `clean` deletes
+# every module's build output, so the offer says so -- declining is a
reasonable answer.
+COMPILE_CHECK_CMD = ["build/sbt"] + [";clean;compile;Test/compile"]
Review Comment:
**Non-blocking (P2):** With no `-P` argument, `build/sbt` never exports
`SBT_MAVEN_PROFILES` (`enableProfile` in `build/sbt-launch-lib.bash` is reached
only from the `-P*)` case). SparkBuild then defaults to `Seq("sbt")`
(`project/SparkBuild.scala:142-148`), and the `sbt` profile in pom.xml adds
guava and pmml-model at compile scope, while CI's Build Spark step always
passes explicit `-P` flags. So with `SBT_MAVEN_PROFILES` unset (the usual
case), this check runs in exactly the configuration the PR description says
turns 'a CI failure into a local pass'. For example, a backport that adds a
`com.google.common` import to a module without its own guava dependency
compiles green here and fails CI. The doctest at 1066-1067 can't see this,
because it only checks that the literal `-Psbt` is absent from argv.
Narrowing to a smoke test was your call (211f912d511, a23a43bd1da), and
that's fine. Any explicit `-P` selection, even a short list of non-flaky
profiles, avoids the fallback. The docs need to match too: 'with the CI
profiles and no `-Psbt`' (1047-1048), 'covers everything' (1195-1196) and
'exactly what would be pushed' (123; it is the working tree that gets compiled,
not the commit) no longer hold.
**Verification:**
- **Behavior:** The compile-check command carries an explicit Maven profile
selection, so SparkBuild's sbt default is not used when SBT_MAVEN_PROFILES is
unset.
- **Inspection:** The compile-check docstring, the command comment and the
binding-policy docstring describe the actual module coverage and make no
CI-parity or complete-coverage claim.
##########
dev/merge_spark_pr.py:
##########
@@ -946,9 +1569,68 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref):
else:
print("Cherry-pick already completed manually; continuing with the
backport.")
- continue_maybe(
- "Pick complete (local ref %s). Push to %s?" % (pick_branch_name,
PUSH_REMOTE_NAME)
+ maybe_binding_policy_warning(pick_ref, pick_head, cherry=True)
+ maybe_compile_check(pick_branch_name, cherry=True)
+
+ choice = get_input(
+ "Pick complete (local ref %s). How should it land?\n"
+ " [d] Push directly to %s/%s\n"
+ " [p] Push to your fork (%s) and open a draft backport PR against %s
(for CI)\n"
+ " [s] Skip this branch (default)\n"
+ "Choose [d/p/s]: "
+ % (pick_branch_name, PUSH_REMOTE_NAME, pick_ref, FORK_REMOTE_NAME,
pick_ref),
+ {"d": ["d", "direct"], "p": ["p", "pr"], "s": ["s", "skip", ""]},
)
+ if choice == "s":
+ # Restore the original checkout and drop the temp PICK_PR_* branches
before skipping,
+ # as the continue_maybe decline path does. Otherwise this branch stays
checked out and a
+ # later pick of the same ref fails when `git fetch` tries to update
the checked-out branch.
+ print("Skipping this branch; the merge continues.")
+ clean_up()
+ raise SkipCherryPick()
+
+ if choice == "p":
+ if FORK_REMOTE_NAME == PUSH_REMOTE_NAME:
+ fail(
+ "FORK_REMOTE_NAME (%s) must differ from PUSH_REMOTE_NAME (%s);
otherwise "
+ "the backport-PR path pushes straight to the Apache remote.
Set "
+ "FORK_REMOTE_NAME to your fork remote and retry."
+ % (FORK_REMOTE_NAME, PUSH_REMOTE_NAME)
+ )
+ backport_branch = _backport_pr_branch(pr_num, pick_ref)
+ owner = _fork_owner()
+ try:
+ git.push(FORK_REMOTE_NAME, pick_branch_name, backport_branch)
+ except Exception as e:
+ fail("Exception while pushing to fork: %s" % e)
+ head = "%s:%s" % (owner, backport_branch)
+ # Reuse the backport commit's own message as the PR description -- the
same body GitHub
+ # would prefill from this single-commit branch -- rather than a bare
one-liner, and tag
+ # the title with the target branch. The @ symbols were stripped when
the merge commit
+ # was built, so this does not fire mention mails; fall back to a short
descriptor only
+ # if the commit somehow carries no body.
+ pr_title = _backport_pr_title(pick_ref, title)
+ pr_body = git.run(["git", "log", "-1", "--format=%b",
pick_branch_name]).strip()
+ if not pr_body:
+ pr_body = "Backport of #%s to `%s`." % (pr_num, pick_ref)
+ created = github.create_pr(pick_ref, head, pr_title, pr_body)
+ pick_hash = git.run("git rev-parse %s" % pick_branch_name).strip()
+ clean_up()
+ if created and created.get("html_url"):
+ pr_url = created["html_url"]
+ print("Backport PR opened: %s" % pr_url)
+ print("Pull request #%s backported to %s via %s" % (pr_num,
pick_ref, pr_url))
+ return pick_ref, pr_url
+ # PR creation failed (no token / HTTP error); the cherry-pick is on
the fork, not on
+ # the Apache branch, so return a `fork:` marker (not a bare hash) so
the merge comment
+ # and the JIRA fix-version logic both treat this branch as
not-yet-landed.
+ print_error(
+ "Backport PR was not opened (see above). The cherry-pick is on
%s:%s; open the "
+ "PR manually against %s." % (FORK_REMOTE_NAME, backport_branch,
pick_ref)
+ )
+ print("Pull request #%s picked into %s (PR open pending)!" % (pr_num,
pick_ref))
+ print("Pick hash: %s" % pick_hash)
+ return pick_ref, "fork:%s:%s" % (owner, backport_branch)
Review Comment:
**Nit (P3):** The startup doctests run on every invocation, but none of them
reaches this branch. If this return gave `(pick_ref, pick_hash)` instead of the
`fork:` marker, every doctest would still pass. Meanwhile `_landed_pick_refs`
would count the fork-only branch as landed, JIRA would get a fix version for a
branch that never received the change, and the merge comment would link a
non-existent apache/spark commit. The same is true if `DryRunGitHub.create_pr`
were removed and a dry run reached the real API. The environment-driven modes
are uncovered too: no `GITHUB_OAUTH_KEY`, `DRY_RUN`, `FORK_OWNER` precedence,
and the fork==push guard.
Mocked doctests in the style already used for `maybe_compile_check` would
cover them: Enter mapping to skip, success returning the PR URL, no-token or
failed creation returning `fork:`, the dry-run stub, and `FORK_OWNER`.
**Verification:**
- **Regression:** Each listed regression (the fallback returning a hash, the
dry-run stub removed, FORK_OWNER ignored, Enter mapped to a direct push) causes
a test failure.
- **Behavior:** The success, no-token fallback and dry-run cases each assert
the recorded outcome, and that no real push or PR creation happens.
##########
dev/merge_spark_pr.py:
##########
@@ -828,13 +1007,395 @@ def clean_up():
git.run("git branch -D %s" % branch)
+def maybe_compile_check(ref_name: str, cherry: bool = False) -> None:
+ """Offer to run a clean SBT compile of main and test sources before
pushing `ref_name`.
+
+ `ref_name` is checked out, so the working tree is the merge (or
cherry-pick) about to be
+ pushed and a clean compile here catches breakage -- a mis-resolved
conflict, or a PR that
+ bit-rotted against the target branch after its CI ran -- before it lands
on the Apache
+ repo. Declining skips the check. A build that does not come back green
does not abort on
+ its own: it asks, because a non-zero sbt is as easily a JDK the branch
rejects or a failed
+ download as it is the backport, and `cherry` decides whether declining
skips this branch or
+ exits (see continue_maybe). The build's output streams to the terminal so
a long compile
+ does not look like a hang.
+
+ What is compiled is the working tree, not the commit: a file re-edited
after the merge
+ commit, or an untracked source file left over from a manual conflict
resolution, compiles
+ here and is absent from the push. (A resolved-but-never-`git add`ed
conflict is NOT the
+ case -- the commit itself fails while files are unmerged, so the check is
never reached.)
+ So warn when the tree is dirty rather than report a green that is about
something else,
+ and name the remedy (`git add` + `git commit --amend`) since the fix is
the commit, not
+ the build.
+
+ Declining runs no build and no git -- the git.run mock raises if a
regression moves a
+ git call above the prompt, which otherwise would run real git in the
committer's repo on
+ every startup (doctest.testmod() runs on invocation):
+
+ >>> from contextlib import redirect_stdout
+ >>> from io import StringIO
+ >>> from unittest.mock import call, patch
+ >>> with (
+ ... patch("builtins.input", return_value=""),
+ ... patch.object(git, "run", side_effect=AssertionError("should not
run git")),
+ ... patch("subprocess.call") as run,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ >>> run.call_count
+ 0
+
+ Accepting runs the SBT compile once, from the repo root, with the CI
profiles and no
+ `-Psbt`; a green build returns quietly. The two git calls are pinned too
-- transposing
+ them or dropping --porcelain would otherwise stay green:
+
+ >>> with (
+ ... patch("builtins.input", return_value="y"),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]) as grun,
+ ... patch("subprocess.call", return_value=0) as run,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ >>> grun.call_args_list[0]
+ call(['git', 'rev-parse', '--show-toplevel'])
+ >>> grun.call_args_list[1]
+ call(['git', 'status', '--porcelain'])
+ >>> run.call_count
+ 1
+ >>> run.call_args[0][0][0], run.call_args[0][0][-1]
+ ('build/sbt', ';clean;compile;Test/compile')
+ >>> "-Psbt" in run.call_args[0][0]
+ False
+ >>> run.call_args[1]["cwd"]
+ '/repo'
+
+ A dirty tree still builds, but warns first -- and the warning names the
remedy
+ (`git add` + `git commit --amend`) alongside the porcelain output:
+
+ >>> with (
+ ... patch("builtins.input", return_value="y"),
+ ... patch.object(git, "run", side_effect=["/repo\\n", " M
foo.scala\\n"]),
+ ... patch("subprocess.call", return_value=0),
+ ... redirect_stdout(StringIO()) as out,
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ >>> "git commit --amend" in out.getvalue()
+ True
+ >>> "M foo.scala" in out.getvalue() # porcelain output is strip()ed
before printing
+ True
+ >>> "Compile check passed." in out.getvalue()
+ True
+
+ A red build aborts the merge (SystemExit from fail) when the override is
declined:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", ""]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ ... aborted = False
+ ... except SystemExit:
+ ... aborted = True
+ >>> aborted
+ True
+
+ The experts-only override continues past a red build:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", "y"]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+
+ Ctrl-C during the build, and a build/sbt that cannot be run at all, are
"did not pass" and
+ get the same override prompt -- never a traceback, which would reach the
top-level handler
+ and let clean_up() delete the unpushed merge commit:
+
+ >>> for boom in (KeyboardInterrupt(), OSError("no build/sbt")):
+ ... with (
+ ... patch("builtins.input", side_effect=["y", "y"]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", side_effect=boom),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+
+ In a cherry-pick, declining skips just that branch instead of exiting:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", ""]),
+ ... patch.object(
+ ... git, "run", side_effect=["/repo\\n", "",
subprocess.CalledProcessError(1, "g")]
+ ... ),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_compile_check("PR_TOOL_PICK_PR_1_BRANCH-4.1",
cherry=True)
+ ... skipped = False
+ ... except SkipCherryPick:
+ ... skipped = True
+ >>> skipped
+ True
+ """
+ # The command is too long to read inside a prompt, so print it on its own
line first.
+ # shlex.join quotes the ";clean;..." argument, so the printed line can be
copy-pasted
+ # into a shell without the semicolons splitting it into four commands.
+ print("Optional pre-push check: %s" % shlex.join(COMPILE_CHECK_CMD))
+ print("Note: 'clean' deletes every module's build output, including under
--dry-run.")
+ prompt = "Run an optional clean compile on %s before pushing? (y/N): " %
ref_name
+ if get_input(prompt, ["y", "n", ""]) != "y":
+ return
+ # build/sbt is repo-relative and SPARK_HOME defaults to the cwd, which is
dev/ when the
+ # script is run as ./merge_spark_pr.py per its own usage line. Ask git for
the root instead.
+ root = git.run(["git", "rev-parse", "--show-toplevel"]).strip()
+ dirty = git.run(["git", "status", "--porcelain"]).strip()
+ if dirty:
+ print_error(
+ "Working tree is not clean, so what compiles below is not exactly
the commit that "
+ "would be pushed -- the uncommitted or untracked changes below
compile here but "
+ "are not in the push. If any of them belong in the merge, 'git
add' them and "
+ "'git commit --amend' before pushing; if they are unrelated
scratch, ignore this "
+ "and carry on:\n%s" % dirty
+ )
+ try:
+ status = subprocess.call(COMPILE_CHECK_CMD, cwd=root)
+ except KeyboardInterrupt:
+ # Interrupting the build must not unwind into the top-level handler:
clean_up() there
+ # force-deletes the PR_TOOL_* branch holding the unpushed commit and
any hand-resolved
+ # conflicts. Treat it as "the check did not finish" and let the
committer decide.
+ print_error("Compile check interrupted.")
+ except OSError as e:
+ print_error("Could not run %s in %s: %s" % (COMPILE_CHECK_CMD[0],
root, e))
+ else:
+ if status == 0:
+ print("Compile check passed.")
+ return
+ print_error(
+ "Compile check exited %d. That is the backport, or your toolchain
-- a JDK this "
+ "branch rejects and a failed dependency download look the same
here." % status
+ )
+ continue_maybe(
+ "Compile check did not pass on %s. Push anyway? (experts only!)" %
ref_name, cherry
+ )
+
+
+def _is_pre_binding_policy_branch(ref: str) -> bool:
+ """True if `ref` is a Spark branch that predates the ConfigBindingPolicy
API (4.2+).
+
+ master and branch-M.x (M >= 4) carry the API; branch-4.0/branch-4.1 and
any branch-3.x
+ do not. Ranking is semver_branch_rank's job -- branch-M.x sorts above
every branch-M.N,
+ and a ref it cannot classify as a maintenance branch gets its (-1, -1)
sentinel. Those
+ refs are treated as carrying the API: fail-open, so master, a tag, or a
custom ref never
+ raises a false warning. The committer sees nothing in that case, which is
the right trade:
+ a backport additionally gets maybe_compile_check right after this -- the
check that covers
+ everything rather than one known API -- and the fail-open refs on a direct
merge (master,
+ a tag, a custom ref) are not pre-4.2 branches in the first place.
+
+ >>> _is_pre_binding_policy_branch("master")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.x")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.2")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.1")
+ True
+ >>> _is_pre_binding_policy_branch("HEAD")
+ False
+ >>> _is_pre_binding_policy_branch("my-custom-ref")
+ False
+ """
+ rank = semver_branch_rank(ref)
+ return rank != (-1, -1) and rank < (4, 2)
+
+
+def _added_binding_policy_tokens(diff: str) -> list[str]:
+ """Return the sorted subset of BINDING_POLICY_PATTERNS matched on added
(``+``) lines.
+
+ Only scans lines the diff added (a leading ``+`` that is not the ``+++ ``
file header --
+ the trailing space keeps an added line whose own text starts with ``++``
counted, apart
+ from the ``++ <space>`` case no real Scala/Java line starts with), so a
cherry-pick that
+ *removes* a stray .withBindingPolicy is not flagged. Matching uses the
+ precise patterns from BINDING_POLICY_PATTERNS, so a comment that merely
mentions
+ ``withBindingPolicy`` (no leading dot, no call) is not flagged;
``ConfigBindingPolicy`` in
+ a comment still can match, which is fine -- the prompt is advisory.
+
+ >>> _added_binding_policy_tokens(
+ ... "+++ b/foo\\n"
+ ... "+.withBindingPolicy(X)\\n"
+ ... "-.withBindingPolicy(Y)\\n"
+ ... "+import ...ConfigBindingPolicy\\n"
+ ... "+// see withBindingPolicy usage\\n"
+ ... " context\\n"
+ ... )
+ ['ConfigBindingPolicy', 'withBindingPolicy']
+ >>> _added_binding_policy_tokens("-.withBindingPolicy(X)\\n")
+ []
+ >>> _added_binding_policy_tokens("+// mentions ConfigBindingPolicy in a
comment\\n")
+ ['ConfigBindingPolicy']
+ >>> _added_binding_policy_tokens("")
+ []
+ """
+ found = set()
+ for line in diff.splitlines():
+ if line.startswith("+") and not line.startswith("+++ "):
+ for token, pattern in BINDING_POLICY_PATTERNS:
+ if pattern.search(line):
+ found.add(token)
+ return sorted(found)
+
+
+def maybe_binding_policy_warning(ref: str, base_head: str, cherry: bool =
False) -> None:
+ """Warn before pushing to a pre-4.2 `ref` a change that adds
ConfigBindingPolicy.
+
+ A merge or cherry-pick onto branch-4.1/4.0/3.x that introduces
``.withBindingPolicy(...)``
+ or ``ConfigBindingPolicy`` will not compile there (the API arrived in
SPARK-55928, 4.2+).
+ This is a git-diff + grep pre-push check: it diffs `base_head` (the branch
tip before the
+ merge or pick) against HEAD restricted to Scala/Java sources, scans only
the added lines
+ for the precise patterns in BINDING_POLICY_PATTERNS, and on a hit prints
the remedy and
+ interactively asks the committer to confirm the push. No-op on branches
that carry the
+ API, so master/branch-4.x are unaffected. `cherry` decides whether
declining skips just
+ this branch or exits (see continue_maybe).
+
+ The diff is forced plain: without --no-color a committer with ``color.ui =
always`` gets
+ added lines behind an ANSI escape, no line starts with "+", and the check
reports nothing
+ while looking like it ran. Like branches_with_merge_footer, a git failure
only warns: on
+ the cherry-pick path this runs after master has already been pushed, and
on the
+ direct-merge path a broken grep must not be the thing that blocks a good
merge either.
+
+ `never` below is how these tests assert "does not prompt": a bare input
mock would return a
+ MagicMock that get_input's loop never accepts, so a regressed guard would
hang the script at
+ startup -- doctest.testmod() runs on every invocation -- instead of
failing.
+
+ >>> from contextlib import redirect_stdout
+ >>> from io import StringIO
+ >>> from unittest.mock import patch
+ >>> never = AssertionError("should not prompt")
+
+ No-op on a branch that carries the API (no diff fetched, no prompt):
+
+ >>> with (
+ ... patch.object(git, "run") as run,
+ ... patch("builtins.input", side_effect=never),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_binding_policy_warning("branch-4.2", "deadbeef")
+ >>> run.call_count
+ 0
+
+ No-op when the change added none of the tokens. The git command is pinned
here because
+ nothing else in these tests would notice it being wrong -- transposing the
two endpoints
+ inverts the whole check, and dropping the pathspec or --no-color silently
defeats it:
+
+ >>> with (
+ ... patch.object(git, "run", return_value="+ unrelated change\\n") as
run,
+ ... patch("builtins.input", side_effect=never),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_binding_policy_warning("branch-4.1", "deadbeef")
+ >>> run.call_args[0][0][:6]
+ ['git', 'diff', '--no-color', '--no-ext-diff', 'deadbeef', 'HEAD']
+ >>> run.call_args[0][0][6:]
+ ['--', ':(top)*.scala', ':(top)*.java']
+
+ Warns and prompts when the change adds a token on a pre-4.2 branch;
accepting continues
+ (no abort), so input is called once by the continue_maybe prompt:
+
+ >>> with (
+ ... patch.object(git, "run", return_value="+.withBindingPolicy(X)\\n"),
+ ... patch("builtins.input", return_value="y") as inp,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_binding_policy_warning("branch-4.1", "deadbeef")
+ >>> inp.call_count
+ 1
+
+ Declining aborts the merge (SystemExit from fail), matching the
compile-check override:
+
+ >>> with (
+ ... patch.object(git, "run", return_value="+.withBindingPolicy(X)\\n"),
+ ... patch("builtins.input", return_value=""),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_binding_policy_warning("branch-4.1", "deadbeef")
+ ... aborted = False
+ ... except SystemExit:
+ ... aborted = True
+ >>> aborted
+ True
+
+ In a cherry-pick, declining skips just that branch, so an already-pushed
branch-M.x pick
+ and the JIRA resolution survive (the second git.run is the
CHERRY_PICK_HEAD probe):
+
+ >>> with (
+ ... patch.object(
+ ... git,
+ ... "run",
+ ... side_effect=[
+ ... "+.withBindingPolicy(X)\\n",
+ ... subprocess.CalledProcessError(1, "git"),
+ ... ],
+ ... ),
+ ... patch("builtins.input", return_value=""),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_binding_policy_warning("branch-4.1", "deadbeef",
cherry=True)
+ ... skipped = False
+ ... except SkipCherryPick:
+ ... skipped = True
+ >>> skipped
+ True
+
+ A git failure warns and scans nothing rather than aborting a half-pushed
merge:
+
+ >>> with (
+ ... patch.object(git, "run", side_effect=OSError("git is sad")),
+ ... patch("builtins.input", side_effect=never),
+ ... redirect_stdout(StringIO()) as out,
+ ... ):
+ ... maybe_binding_policy_warning("branch-4.1", "deadbeef")
+ >>> "git is sad" in out.getvalue()
+ True
+ """
+ if not _is_pre_binding_policy_branch(ref):
+ return
+ try:
+ diff = git.run(
+ ["git", "diff", "--no-color", "--no-ext-diff", base_head, "HEAD",
"--"]
+ + list(BINDING_POLICY_PATHSPECS)
+ )
+ except Exception as e:
+ print_error(
+ "Could not diff %s..HEAD; skipping the ConfigBindingPolicy check:
%s" % (base_head, e)
+ )
+ return
+ tokens = _added_binding_policy_tokens(diff)
+ if not tokens:
+ return
+ print_error(
+ "Warning: what would be pushed to %s adds %s, which does not exist
before branch-4.2 "
+ "(SPARK-55928). Drop the .withBindingPolicy(...) call and the
ConfigBindingPolicy "
+ "import. Leave .version(...) alone -- it names the release the config
shipped in -- "
+ "unless the config itself is new on this branch, in which case
retarget it to this "
+ "branch's next release." % (ref, " / ".join(tokens))
+ )
+ continue_maybe("Push to %s anyway? (experts only!)" % ref, cherry)
+
+
# merge the requested PR and return the merge hash
def merge_pr(pr_num, target_ref, title, body, pr_repo_desc, pr_author,
co_authors):
pr_branch_name = "%s_MERGE_PR_%s" % (BRANCH_PREFIX, pr_num)
target_branch_name = "%s_MERGE_PR_%s_%s" % (BRANCH_PREFIX, pr_num,
target_ref.upper())
git.run("git fetch %s pull/%s/head:%s" % (PR_REMOTE_NAME, pr_num,
pr_branch_name))
git.run("git fetch %s %s:%s" % (PUSH_REMOTE_NAME, target_ref,
target_branch_name))
git.run("git checkout %s" % target_branch_name)
+ target_head = git.run("git rev-parse HEAD").strip()
Review Comment:
**Nit (P3):** Nothing reads `target_head` after this line. Its only
consumer, `maybe_binding_policy_warning(target_ref, target_head)`, was dropped
in 702d029344e, and `merge_pr` has no other use of it. Because `Git.run` echoes
every command, each merge now prints a stray `git rev-parse HEAD` and spawns a
process for nothing. It can go together with the merge-path wording that still
describes the removed check.
See **Shared repair plan 3** in the review body.
##########
dev/merge_spark_pr.py:
##########
@@ -946,9 +1569,68 @@ def _do_cherry_pick(pr_num, merge_hash, pick_ref):
else:
print("Cherry-pick already completed manually; continuing with the
backport.")
- continue_maybe(
- "Pick complete (local ref %s). Push to %s?" % (pick_branch_name,
PUSH_REMOTE_NAME)
+ maybe_binding_policy_warning(pick_ref, pick_head, cherry=True)
+ maybe_compile_check(pick_branch_name, cherry=True)
+
+ choice = get_input(
+ "Pick complete (local ref %s). How should it land?\n"
+ " [d] Push directly to %s/%s\n"
+ " [p] Push to your fork (%s) and open a draft backport PR against %s
(for CI)\n"
+ " [s] Skip this branch (default)\n"
+ "Choose [d/p/s]: "
+ % (pick_branch_name, PUSH_REMOTE_NAME, pick_ref, FORK_REMOTE_NAME,
pick_ref),
+ {"d": ["d", "direct"], "p": ["p", "pr"], "s": ["s", "skip", ""]},
)
+ if choice == "s":
+ # Restore the original checkout and drop the temp PICK_PR_* branches
before skipping,
+ # as the continue_maybe decline path does. Otherwise this branch stays
checked out and a
+ # later pick of the same ref fails when `git fetch` tries to update
the checked-out branch.
+ print("Skipping this branch; the merge continues.")
+ clean_up()
+ raise SkipCherryPick()
+
+ if choice == "p":
+ if FORK_REMOTE_NAME == PUSH_REMOTE_NAME:
+ fail(
+ "FORK_REMOTE_NAME (%s) must differ from PUSH_REMOTE_NAME (%s);
otherwise "
+ "the backport-PR path pushes straight to the Apache remote.
Set "
+ "FORK_REMOTE_NAME to your fork remote and retry."
+ % (FORK_REMOTE_NAME, PUSH_REMOTE_NAME)
+ )
+ backport_branch = _backport_pr_branch(pr_num, pick_ref)
+ owner = _fork_owner()
+ try:
+ git.push(FORK_REMOTE_NAME, pick_branch_name, backport_branch)
Review Comment:
**Nit (P3):** `_backport_pr_branch` is deterministic, and `Git.push` is a
plain `git push <remote> <local>:<remote>`. A re-pick produces a new commit
(new committer timestamp, and possibly a moved base) that doesn't contain the
earlier fork commit. The second push is therefore non-fast-forward and goes to
`fail('Exception while pushing to fork: ...')`, which deletes the pick. A later
run re-offers the branch, because `branches_with_merge_footer` scans only
`PUSH_REMOTE_NAME`. The same failure hits a retry after a first attempt without
`GITHUB_OAUTH_KEY`, since `create_pr` checks the token only after the fork push
has already succeeded.
Suggested fix: once the fork target's identity is validated (not before),
treat this branch as script-owned and update it with `--force-with-lease`. Also
reuse an existing backport PR for the same head instead of creating a new one.
**Verification:**
- **Regression:** A second backport-PR attempt for the same PR and branch,
after a first attempt left the fork branch behind, updates that branch and ends
with a PR link or the fork-only outcome rather than a non-fast-forward failure.
##########
dev/merge_spark_pr.py:
##########
@@ -828,13 +1007,395 @@ def clean_up():
git.run("git branch -D %s" % branch)
+def maybe_compile_check(ref_name: str, cherry: bool = False) -> None:
+ """Offer to run a clean SBT compile of main and test sources before
pushing `ref_name`.
+
+ `ref_name` is checked out, so the working tree is the merge (or
cherry-pick) about to be
+ pushed and a clean compile here catches breakage -- a mis-resolved
conflict, or a PR that
+ bit-rotted against the target branch after its CI ran -- before it lands
on the Apache
+ repo. Declining skips the check. A build that does not come back green
does not abort on
+ its own: it asks, because a non-zero sbt is as easily a JDK the branch
rejects or a failed
+ download as it is the backport, and `cherry` decides whether declining
skips this branch or
+ exits (see continue_maybe). The build's output streams to the terminal so
a long compile
+ does not look like a hang.
+
+ What is compiled is the working tree, not the commit: a file re-edited
after the merge
+ commit, or an untracked source file left over from a manual conflict
resolution, compiles
+ here and is absent from the push. (A resolved-but-never-`git add`ed
conflict is NOT the
+ case -- the commit itself fails while files are unmerged, so the check is
never reached.)
+ So warn when the tree is dirty rather than report a green that is about
something else,
+ and name the remedy (`git add` + `git commit --amend`) since the fix is
the commit, not
+ the build.
+
+ Declining runs no build and no git -- the git.run mock raises if a
regression moves a
+ git call above the prompt, which otherwise would run real git in the
committer's repo on
+ every startup (doctest.testmod() runs on invocation):
+
+ >>> from contextlib import redirect_stdout
+ >>> from io import StringIO
+ >>> from unittest.mock import call, patch
+ >>> with (
+ ... patch("builtins.input", return_value=""),
+ ... patch.object(git, "run", side_effect=AssertionError("should not
run git")),
+ ... patch("subprocess.call") as run,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ >>> run.call_count
+ 0
+
+ Accepting runs the SBT compile once, from the repo root, with the CI
profiles and no
+ `-Psbt`; a green build returns quietly. The two git calls are pinned too
-- transposing
+ them or dropping --porcelain would otherwise stay green:
+
+ >>> with (
+ ... patch("builtins.input", return_value="y"),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]) as grun,
+ ... patch("subprocess.call", return_value=0) as run,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ >>> grun.call_args_list[0]
+ call(['git', 'rev-parse', '--show-toplevel'])
+ >>> grun.call_args_list[1]
+ call(['git', 'status', '--porcelain'])
+ >>> run.call_count
+ 1
+ >>> run.call_args[0][0][0], run.call_args[0][0][-1]
+ ('build/sbt', ';clean;compile;Test/compile')
+ >>> "-Psbt" in run.call_args[0][0]
+ False
+ >>> run.call_args[1]["cwd"]
+ '/repo'
+
+ A dirty tree still builds, but warns first -- and the warning names the
remedy
+ (`git add` + `git commit --amend`) alongside the porcelain output:
+
+ >>> with (
+ ... patch("builtins.input", return_value="y"),
+ ... patch.object(git, "run", side_effect=["/repo\\n", " M
foo.scala\\n"]),
+ ... patch("subprocess.call", return_value=0),
+ ... redirect_stdout(StringIO()) as out,
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ >>> "git commit --amend" in out.getvalue()
+ True
+ >>> "M foo.scala" in out.getvalue() # porcelain output is strip()ed
before printing
+ True
+ >>> "Compile check passed." in out.getvalue()
+ True
+
+ A red build aborts the merge (SystemExit from fail) when the override is
declined:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", ""]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ ... aborted = False
+ ... except SystemExit:
+ ... aborted = True
+ >>> aborted
+ True
+
+ The experts-only override continues past a red build:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", "y"]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+
+ Ctrl-C during the build, and a build/sbt that cannot be run at all, are
"did not pass" and
+ get the same override prompt -- never a traceback, which would reach the
top-level handler
+ and let clean_up() delete the unpushed merge commit:
+
+ >>> for boom in (KeyboardInterrupt(), OSError("no build/sbt")):
+ ... with (
+ ... patch("builtins.input", side_effect=["y", "y"]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", side_effect=boom),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+
+ In a cherry-pick, declining skips just that branch instead of exiting:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", ""]),
+ ... patch.object(
+ ... git, "run", side_effect=["/repo\\n", "",
subprocess.CalledProcessError(1, "g")]
+ ... ),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_compile_check("PR_TOOL_PICK_PR_1_BRANCH-4.1",
cherry=True)
+ ... skipped = False
+ ... except SkipCherryPick:
+ ... skipped = True
+ >>> skipped
+ True
+ """
+ # The command is too long to read inside a prompt, so print it on its own
line first.
+ # shlex.join quotes the ";clean;..." argument, so the printed line can be
copy-pasted
+ # into a shell without the semicolons splitting it into four commands.
+ print("Optional pre-push check: %s" % shlex.join(COMPILE_CHECK_CMD))
+ print("Note: 'clean' deletes every module's build output, including under
--dry-run.")
+ prompt = "Run an optional clean compile on %s before pushing? (y/N): " %
ref_name
+ if get_input(prompt, ["y", "n", ""]) != "y":
+ return
+ # build/sbt is repo-relative and SPARK_HOME defaults to the cwd, which is
dev/ when the
+ # script is run as ./merge_spark_pr.py per its own usage line. Ask git for
the root instead.
+ root = git.run(["git", "rev-parse", "--show-toplevel"]).strip()
+ dirty = git.run(["git", "status", "--porcelain"]).strip()
+ if dirty:
+ print_error(
+ "Working tree is not clean, so what compiles below is not exactly
the commit that "
+ "would be pushed -- the uncommitted or untracked changes below
compile here but "
+ "are not in the push. If any of them belong in the merge, 'git
add' them and "
+ "'git commit --amend' before pushing; if they are unrelated
scratch, ignore this "
+ "and carry on:\n%s" % dirty
+ )
+ try:
+ status = subprocess.call(COMPILE_CHECK_CMD, cwd=root)
+ except KeyboardInterrupt:
+ # Interrupting the build must not unwind into the top-level handler:
clean_up() there
+ # force-deletes the PR_TOOL_* branch holding the unpushed commit and
any hand-resolved
+ # conflicts. Treat it as "the check did not finish" and let the
committer decide.
+ print_error("Compile check interrupted.")
+ except OSError as e:
+ print_error("Could not run %s in %s: %s" % (COMPILE_CHECK_CMD[0],
root, e))
+ else:
+ if status == 0:
+ print("Compile check passed.")
+ return
+ print_error(
+ "Compile check exited %d. That is the backport, or your toolchain
-- a JDK this "
+ "branch rejects and a failed dependency download look the same
here." % status
+ )
+ continue_maybe(
+ "Compile check did not pass on %s. Push anyway? (experts only!)" %
ref_name, cherry
+ )
+
+
+def _is_pre_binding_policy_branch(ref: str) -> bool:
+ """True if `ref` is a Spark branch that predates the ConfigBindingPolicy
API (4.2+).
+
+ master and branch-M.x (M >= 4) carry the API; branch-4.0/branch-4.1 and
any branch-3.x
+ do not. Ranking is semver_branch_rank's job -- branch-M.x sorts above
every branch-M.N,
+ and a ref it cannot classify as a maintenance branch gets its (-1, -1)
sentinel. Those
+ refs are treated as carrying the API: fail-open, so master, a tag, or a
custom ref never
+ raises a false warning. The committer sees nothing in that case, which is
the right trade:
+ a backport additionally gets maybe_compile_check right after this -- the
check that covers
+ everything rather than one known API -- and the fail-open refs on a direct
merge (master,
+ a tag, a custom ref) are not pre-4.2 branches in the first place.
+
+ >>> _is_pre_binding_policy_branch("master")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.x")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.2")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.1")
Review Comment:
**Nit (P3):** `branch-4.1` is now the only pre-4.2 case, although the
docstring promises branch-4.0, branch-4.1 and any branch-3.x. The classifier is
right today: I ran it, and branch-3.5, branch-3.x (rank `(3, inf)`) and
branch-4.0 all return `True`. But rewriting `rank < (4, 2)` as `rank[0] == 4
and rank[1] < 2` would keep every remaining doctest green while branch-3.x
backports stop being warned. Restoring `branch-3.5`, `branch-3.x` and
`branch-4.0` next to `branch-4.1` takes three lines.
**Verification:**
- **Regression:** Branch classification is asserted pre-4.2 for branch-3.5,
branch-3.x and branch-4.0 as well as branch-4.1, so a comparison that checks
only the minor version fails.
##########
dev/merge_spark_pr.py:
##########
@@ -901,12 +1462,74 @@ def merge_pr(pr_num, target_ref, title, body,
pr_repo_desc, pr_author, co_author
return merge_hash
-def _do_cherry_pick(pr_num, merge_hash, pick_ref):
+def _backport_pr_branch(pr_num, pick_ref):
+ """Fork branch name for a backport PR: stable, PR-traceable,
branch-suffixed.
+
+ >>> _backport_pr_branch(123, "branch-4.2")
+ 'backport-pr123-branch-4.2'
+ >>> _backport_pr_branch(9, "branch-3.5")
+ 'backport-pr9-branch-3.5'
+ """
+ return "backport-pr%s-%s" % (pr_num, pick_ref)
+
+
+def _backport_pr_title(pick_ref, title):
+ """PR title for a backport PR: the merged title tagged with its target
branch.
+
+ >>> _backport_pr_title("branch-4.2", "[SPARK-1234][SQL] Fix a thing")
+ '[BACKPORT-branch-4.2] [SPARK-1234][SQL] Fix a thing'
+ """
+ return "[BACKPORT-%s] %s" % (pick_ref, title)
+
+
+def _fork_owner():
+ """GitHub owner of the fork remote, for the `head` field of a backport PR.
+
+ FORK_OWNER wins when set; otherwise the owner is parsed from the fork
remote URL
+ ([email protected]:owner/spark.git or https://github.com/owner/spark.git). A
non-GitHub
+ remote fails with a clear message, rather than silently sending a bad
`head` to the
+ API, so the committer can set FORK_OWNER.
+ """
+ if FORK_OWNER:
+ return FORK_OWNER
+ url = git.run(["git", "remote", "get-url", FORK_REMOTE_NAME]).strip()
+ m = re.search(r"github\.com[:/]([^/]+)/", url)
+ if not m:
+ fail(
+ "Could not parse a GitHub owner from %s remote URL '%s'. "
+ "Set FORK_OWNER to your GitHub login and retry." %
(FORK_REMOTE_NAME, url)
+ )
+ return m.group(1)
+
+
+def _landed_pick_refs(picks):
+ """Refs from `picks` whose change has actually landed on the Apache branch.
+
+ A direct push returns a bare commit hash (landed); the backport-PR paths
return a PR URL
+ (`https://`) or a `fork:` marker (PR not opened), neither of which has
landed yet. Used to
+ keep JIRA fix versions tied to landed commits rather than open backport
PRs.
+
+ >>> _landed_pick_refs([("master", "abc123"), ("branch-4.2",
"https://github.com/apache/spark/pull/9";)])
+ ['master']
+ >>> _landed_pick_refs(
+ ... [("branch-4.x", "deadbeef"), ("branch-4.2",
"fork:o:backport-pr1-branch-4.2")]
+ ... )
+ ['branch-4.x']
+ >>> _landed_pick_refs([])
+ []
+ """
+ return [ref for ref, marker in picks if not marker.startswith(("https://";,
"fork:"))]
+
+
+def _do_cherry_pick(pr_num, merge_hash, pick_ref, title=""):
"""Cherry-pick `merge_hash` onto `pick_ref` and push.
- Returns the (pushed ref, pushed commit hash) pair. Raises `SkipCherryPick`
if the
- cherry-pick conflicts and the committer declines to resolve it, after
attempting to
- abort the cherry-pick and restore the working tree.
+ Returns the (pushed ref, marker) pair. The marker is the pushed commit
hash for a
+ direct push, or the backport PR URL when the committer chose to land on a
fork branch
Review Comment:
**Nit (P3):** This contract lists only 'the pushed commit hash ... or the
backport PR URL'. It omits the `fork:<owner>:<branch>` marker returned at 1633,
and as `SkipCherryPick` causes it names only a declined conflict and `[s]`,
leaving out the declined binding-policy and compile-check prompts (1572-1573).
The summary line still says '... and push.' (1525). The same assumption shows
up elsewhere: `cherry_pick`'s docstring says the two-branch list 'holds only
what landed' (1730-1731), and the `[b]oth` comment says branch-M.x is returned
so 'JIRA/comment reflect it' (1766-1767), although `main()` keeps pending picks
out of `landed_refs`. `post_merge_comment` says it records 'every branch the
change landed on' (794-795), and `main()` still says 'Ordered (branch,
commit_hash) pairs' (3380) and 'every branch that successfully received the
change' (3423). Readers, and future consumers, will take every returned pick as
landed, which is exactly the assumption behind the Upstream-First by
pass.
See **Shared repair plan 1** in the review body.
##########
dev/merge_spark_pr.py:
##########
@@ -709,9 +743,28 @@ def comment(self, pr_num, body):
print_error("Failed to comment on PR #%s: HTTP %s %s" % (pr_num,
e.code, e.reason))
return None
+ def create_pr(self, base, head, title, body):
Review Comment:
**Nit (P3):** Adding `create_pr` makes the `GitHub` class docstring false:
it still says 'only the two mutations (closing the PR and posting a comment)
live here' (710-711). The neighbouring lists miss the new effects as well. The
`DRY_RUN_ENV` comment (105-106) and the `Git` docstring (899-900) cover only
the push to `PUSH_REMOTE_NAME`. The `--dry-run` help (3085-3086) and the
runtime banner (3101-3102) list 'GitHub PR close/comment' but not PR creation,
and the banner names only the push to the Apache remote. Suppression itself
works (`DryRunGit.push`, `DryRunGitHub.create_pr`), so the fix is just to
update those five texts to mention fork pushes and draft PR creation.
**Verification:**
- **Inspection:** The seam docstrings, the DRY_RUN_ENV comment, the
--dry-run help and the dry-run banner each name fork pushes and draft PR
creation alongside the existing effects.
##########
dev/merge_spark_pr.py:
##########
@@ -828,13 +1007,395 @@ def clean_up():
git.run("git branch -D %s" % branch)
+def maybe_compile_check(ref_name: str, cherry: bool = False) -> None:
+ """Offer to run a clean SBT compile of main and test sources before
pushing `ref_name`.
+
+ `ref_name` is checked out, so the working tree is the merge (or
cherry-pick) about to be
+ pushed and a clean compile here catches breakage -- a mis-resolved
conflict, or a PR that
+ bit-rotted against the target branch after its CI ran -- before it lands
on the Apache
+ repo. Declining skips the check. A build that does not come back green
does not abort on
+ its own: it asks, because a non-zero sbt is as easily a JDK the branch
rejects or a failed
+ download as it is the backport, and `cherry` decides whether declining
skips this branch or
+ exits (see continue_maybe). The build's output streams to the terminal so
a long compile
+ does not look like a hang.
+
+ What is compiled is the working tree, not the commit: a file re-edited
after the merge
+ commit, or an untracked source file left over from a manual conflict
resolution, compiles
+ here and is absent from the push. (A resolved-but-never-`git add`ed
conflict is NOT the
+ case -- the commit itself fails while files are unmerged, so the check is
never reached.)
+ So warn when the tree is dirty rather than report a green that is about
something else,
+ and name the remedy (`git add` + `git commit --amend`) since the fix is
the commit, not
+ the build.
+
+ Declining runs no build and no git -- the git.run mock raises if a
regression moves a
+ git call above the prompt, which otherwise would run real git in the
committer's repo on
+ every startup (doctest.testmod() runs on invocation):
+
+ >>> from contextlib import redirect_stdout
+ >>> from io import StringIO
+ >>> from unittest.mock import call, patch
+ >>> with (
+ ... patch("builtins.input", return_value=""),
+ ... patch.object(git, "run", side_effect=AssertionError("should not
run git")),
+ ... patch("subprocess.call") as run,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ >>> run.call_count
+ 0
+
+ Accepting runs the SBT compile once, from the repo root, with the CI
profiles and no
+ `-Psbt`; a green build returns quietly. The two git calls are pinned too
-- transposing
+ them or dropping --porcelain would otherwise stay green:
+
+ >>> with (
+ ... patch("builtins.input", return_value="y"),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]) as grun,
+ ... patch("subprocess.call", return_value=0) as run,
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ >>> grun.call_args_list[0]
+ call(['git', 'rev-parse', '--show-toplevel'])
+ >>> grun.call_args_list[1]
+ call(['git', 'status', '--porcelain'])
+ >>> run.call_count
+ 1
+ >>> run.call_args[0][0][0], run.call_args[0][0][-1]
+ ('build/sbt', ';clean;compile;Test/compile')
+ >>> "-Psbt" in run.call_args[0][0]
+ False
+ >>> run.call_args[1]["cwd"]
+ '/repo'
+
+ A dirty tree still builds, but warns first -- and the warning names the
remedy
+ (`git add` + `git commit --amend`) alongside the porcelain output:
+
+ >>> with (
+ ... patch("builtins.input", return_value="y"),
+ ... patch.object(git, "run", side_effect=["/repo\\n", " M
foo.scala\\n"]),
+ ... patch("subprocess.call", return_value=0),
+ ... redirect_stdout(StringIO()) as out,
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ >>> "git commit --amend" in out.getvalue()
+ True
+ >>> "M foo.scala" in out.getvalue() # porcelain output is strip()ed
before printing
+ True
+ >>> "Compile check passed." in out.getvalue()
+ True
+
+ A red build aborts the merge (SystemExit from fail) when the override is
declined:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", ""]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+ ... aborted = False
+ ... except SystemExit:
+ ... aborted = True
+ >>> aborted
+ True
+
+ The experts-only override continues past a red build:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", "y"]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+
+ Ctrl-C during the build, and a build/sbt that cannot be run at all, are
"did not pass" and
+ get the same override prompt -- never a traceback, which would reach the
top-level handler
+ and let clean_up() delete the unpushed merge commit:
+
+ >>> for boom in (KeyboardInterrupt(), OSError("no build/sbt")):
+ ... with (
+ ... patch("builtins.input", side_effect=["y", "y"]),
+ ... patch.object(git, "run", side_effect=["/repo\\n", ""]),
+ ... patch("subprocess.call", side_effect=boom),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... maybe_compile_check("PR_TOOL_MERGE_PR_1_MASTER")
+
+ In a cherry-pick, declining skips just that branch instead of exiting:
+
+ >>> with (
+ ... patch("builtins.input", side_effect=["y", ""]),
+ ... patch.object(
+ ... git, "run", side_effect=["/repo\\n", "",
subprocess.CalledProcessError(1, "g")]
+ ... ),
+ ... patch("subprocess.call", return_value=1),
+ ... redirect_stdout(StringIO()),
+ ... ):
+ ... try:
+ ... maybe_compile_check("PR_TOOL_PICK_PR_1_BRANCH-4.1",
cherry=True)
+ ... skipped = False
+ ... except SkipCherryPick:
+ ... skipped = True
+ >>> skipped
+ True
+ """
+ # The command is too long to read inside a prompt, so print it on its own
line first.
+ # shlex.join quotes the ";clean;..." argument, so the printed line can be
copy-pasted
+ # into a shell without the semicolons splitting it into four commands.
+ print("Optional pre-push check: %s" % shlex.join(COMPILE_CHECK_CMD))
+ print("Note: 'clean' deletes every module's build output, including under
--dry-run.")
+ prompt = "Run an optional clean compile on %s before pushing? (y/N): " %
ref_name
+ if get_input(prompt, ["y", "n", ""]) != "y":
+ return
+ # build/sbt is repo-relative and SPARK_HOME defaults to the cwd, which is
dev/ when the
+ # script is run as ./merge_spark_pr.py per its own usage line. Ask git for
the root instead.
+ root = git.run(["git", "rev-parse", "--show-toplevel"]).strip()
+ dirty = git.run(["git", "status", "--porcelain"]).strip()
+ if dirty:
+ print_error(
+ "Working tree is not clean, so what compiles below is not exactly
the commit that "
+ "would be pushed -- the uncommitted or untracked changes below
compile here but "
+ "are not in the push. If any of them belong in the merge, 'git
add' them and "
+ "'git commit --amend' before pushing; if they are unrelated
scratch, ignore this "
+ "and carry on:\n%s" % dirty
+ )
+ try:
+ status = subprocess.call(COMPILE_CHECK_CMD, cwd=root)
+ except KeyboardInterrupt:
+ # Interrupting the build must not unwind into the top-level handler:
clean_up() there
+ # force-deletes the PR_TOOL_* branch holding the unpushed commit and
any hand-resolved
+ # conflicts. Treat it as "the check did not finish" and let the
committer decide.
+ print_error("Compile check interrupted.")
+ except OSError as e:
+ print_error("Could not run %s in %s: %s" % (COMPILE_CHECK_CMD[0],
root, e))
+ else:
+ if status == 0:
+ print("Compile check passed.")
+ return
+ print_error(
+ "Compile check exited %d. That is the backport, or your toolchain
-- a JDK this "
+ "branch rejects and a failed dependency download look the same
here." % status
+ )
+ continue_maybe(
+ "Compile check did not pass on %s. Push anyway? (experts only!)" %
ref_name, cherry
+ )
+
+
+def _is_pre_binding_policy_branch(ref: str) -> bool:
+ """True if `ref` is a Spark branch that predates the ConfigBindingPolicy
API (4.2+).
+
+ master and branch-M.x (M >= 4) carry the API; branch-4.0/branch-4.1 and
any branch-3.x
+ do not. Ranking is semver_branch_rank's job -- branch-M.x sorts above
every branch-M.N,
+ and a ref it cannot classify as a maintenance branch gets its (-1, -1)
sentinel. Those
+ refs are treated as carrying the API: fail-open, so master, a tag, or a
custom ref never
+ raises a false warning. The committer sees nothing in that case, which is
the right trade:
+ a backport additionally gets maybe_compile_check right after this -- the
check that covers
+ everything rather than one known API -- and the fail-open refs on a direct
merge (master,
+ a tag, a custom ref) are not pre-4.2 branches in the first place.
+
+ >>> _is_pre_binding_policy_branch("master")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.x")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.2")
+ False
+ >>> _is_pre_binding_policy_branch("branch-4.1")
+ True
+ >>> _is_pre_binding_policy_branch("HEAD")
+ False
+ >>> _is_pre_binding_policy_branch("my-custom-ref")
+ False
+ """
+ rank = semver_branch_rank(ref)
+ return rank != (-1, -1) and rank < (4, 2)
+
+
+def _added_binding_policy_tokens(diff: str) -> list[str]:
+ """Return the sorted subset of BINDING_POLICY_PATTERNS matched on added
(``+``) lines.
+
+ Only scans lines the diff added (a leading ``+`` that is not the ``+++ ``
file header --
+ the trailing space keeps an added line whose own text starts with ``++``
counted, apart
+ from the ``++ <space>`` case no real Scala/Java line starts with), so a
cherry-pick that
+ *removes* a stray .withBindingPolicy is not flagged. Matching uses the
+ precise patterns from BINDING_POLICY_PATTERNS, so a comment that merely
mentions
+ ``withBindingPolicy`` (no leading dot, no call) is not flagged;
``ConfigBindingPolicy`` in
+ a comment still can match, which is fine -- the prompt is advisory.
+
+ >>> _added_binding_policy_tokens(
+ ... "+++ b/foo\\n"
+ ... "+.withBindingPolicy(X)\\n"
+ ... "-.withBindingPolicy(Y)\\n"
+ ... "+import ...ConfigBindingPolicy\\n"
+ ... "+// see withBindingPolicy usage\\n"
+ ... " context\\n"
+ ... )
+ ['ConfigBindingPolicy', 'withBindingPolicy']
+ >>> _added_binding_policy_tokens("-.withBindingPolicy(X)\\n")
+ []
+ >>> _added_binding_policy_tokens("+// mentions ConfigBindingPolicy in a
comment\\n")
+ ['ConfigBindingPolicy']
+ >>> _added_binding_policy_tokens("")
+ []
+ """
+ found = set()
+ for line in diff.splitlines():
+ if line.startswith("+") and not line.startswith("+++ "):
+ for token, pattern in BINDING_POLICY_PATTERNS:
+ if pattern.search(line):
+ found.add(token)
+ return sorted(found)
+
+
+def maybe_binding_policy_warning(ref: str, base_head: str, cherry: bool =
False) -> None:
+ """Warn before pushing to a pre-4.2 `ref` a change that adds
ConfigBindingPolicy.
+
+ A merge or cherry-pick onto branch-4.1/4.0/3.x that introduces
``.withBindingPolicy(...)``
+ or ``ConfigBindingPolicy`` will not compile there (the API arrived in
SPARK-55928, 4.2+).
+ This is a git-diff + grep pre-push check: it diffs `base_head` (the branch
tip before the
+ merge or pick) against HEAD restricted to Scala/Java sources, scans only
the added lines
+ for the precise patterns in BINDING_POLICY_PATTERNS, and on a hit prints
the remedy and
+ interactively asks the committer to confirm the push. No-op on branches
that carry the
+ API, so master/branch-4.x are unaffected. `cherry` decides whether
declining skips just
+ this branch or exits (see continue_maybe).
+
+ The diff is forced plain: without --no-color a committer with ``color.ui =
always`` gets
+ added lines behind an ANSI escape, no line starts with "+", and the check
reports nothing
+ while looking like it ran. Like branches_with_merge_footer, a git failure
only warns: on
+ the cherry-pick path this runs after master has already been pushed, and
on the
+ direct-merge path a broken grep must not be the thing that blocks a good
merge either.
Review Comment:
**Nit (P3):** `merge_pr` calls neither check now. The only callers are at
1572-1573, both with `cherry=True`, and nothing imports this script. But the
text added in this PR still describes a merge caller: 'A merge or cherry-pick
onto branch-4.1/4.0/3.x' (1255), the 'direct-merge path' sentence here
(1267-1268), 'the fail-open refs on a direct merge' (1196-1197), 'A merge or
backport to a pre-4.2 branch' (127), and 'the merge (or cherry-pick) about to
be pushed' (1013-1014). The `cherry=False` defaults (1010, 1252), whose decline
exits the script, are used only by the doctests 'A red build aborts the merge'
(1088) and 'Declining aborts the merge' (1317). If skipping direct merges is
intended, as 702d029344e says, drop that wording, the `cherry=False` mode and
those doctests, along with `target_head`. Otherwise restore the merge-path call.
See **Shared repair plan 3** in the review body.
##########
dev/merge_spark_pr.py:
##########
@@ -901,12 +1462,74 @@ def merge_pr(pr_num, target_ref, title, body,
pr_repo_desc, pr_author, co_author
return merge_hash
-def _do_cherry_pick(pr_num, merge_hash, pick_ref):
+def _backport_pr_branch(pr_num, pick_ref):
+ """Fork branch name for a backport PR: stable, PR-traceable,
branch-suffixed.
+
+ >>> _backport_pr_branch(123, "branch-4.2")
+ 'backport-pr123-branch-4.2'
+ >>> _backport_pr_branch(9, "branch-3.5")
+ 'backport-pr9-branch-3.5'
+ """
+ return "backport-pr%s-%s" % (pr_num, pick_ref)
+
+
+def _backport_pr_title(pick_ref, title):
+ """PR title for a backport PR: the merged title tagged with its target
branch.
+
+ >>> _backport_pr_title("branch-4.2", "[SPARK-1234][SQL] Fix a thing")
+ '[BACKPORT-branch-4.2] [SPARK-1234][SQL] Fix a thing'
+ """
+ return "[BACKPORT-%s] %s" % (pick_ref, title)
+
+
+def _fork_owner():
+ """GitHub owner of the fork remote, for the `head` field of a backport PR.
+
+ FORK_OWNER wins when set; otherwise the owner is parsed from the fork
remote URL
+ ([email protected]:owner/spark.git or https://github.com/owner/spark.git). A
non-GitHub
+ remote fails with a clear message, rather than silently sending a bad
`head` to the
+ API, so the committer can set FORK_OWNER.
+ """
+ if FORK_OWNER:
+ return FORK_OWNER
+ url = git.run(["git", "remote", "get-url", FORK_REMOTE_NAME]).strip()
+ m = re.search(r"github\.com[:/]([^/]+)/", url)
Review Comment:
**Nit (P3):** For `ssh://[email protected]:443/<owner>/spark.git`, the
port-443 form GitHub documents for networks that block port 22, this pattern
captures `443`. I ran it; the scp, https and `ssh://[email protected]/...` forms
parse correctly. The push still works, because git uses the URL, but
`create_pr` is sent `head='443:backport-pr<N>-<branch>'`. GitHub rejects that,
so the fallback records `fork:443:...`, and the public merge comment says
'backport pushed to <branch> 443:backport-... (PR not opened)'. Allowing an
optional `:port`, or parsing the URL properly as part of the
destination-identity fix, resolves it. Setting `FORK_OWNER` is the workaround.
See **Shared repair plan 2** in the review body.
--
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]
