From 7f3583b9b87813717bd1b9e7bc48f463f481f2c4 Mon Sep 17 00:00:00 2001 From: Scott Mabin Date: Mon, 24 Aug 2026 15:00:36 +0100 Subject: [PATCH 1/2] xtask: re-read manifests in execute-plan apply loop The preflight loop parsed every package manifest up front and the apply loop reused those in-memory copies. Bumping a package rewrites the on-disk manifests of its workspace dependents, so a package bumped after its dependencies had those rewrites on disk - but its own bump step then saved its stale pre-bump snapshot, silently reverting them. esp-hal is bumped last and depends on nearly every other released crate, so it lost all of its intra-workspace dependency bumps (e.g. the esp-config build-dependency stayed at the old requirement), which broke the release build. Re-read each manifest from disk immediately before bumping so dependency rewrites from earlier steps are preserved. Regression from #5644, which moved the manifest load into an up-front preflight loop and reused the parsed copies. Co-authored-by: Cursor --- xtask/src/commands/release/execute_plan.rs | 39 ++++++++++++++-------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/xtask/src/commands/release/execute_plan.rs b/xtask/src/commands/release/execute_plan.rs index 6840d088d12..edbfb77edae 100644 --- a/xtask/src/commands/release/execute_plan.rs +++ b/xtask/src/commands/release/execute_plan.rs @@ -3,7 +3,6 @@ use std::{path::Path, process::Command}; use anyhow::{Context, Result, bail, ensure}; use clap::Args; use strum::IntoEnumIterator; -use toml_edit::{Item, Value}; use crate::{ cargo::CargoToml, @@ -62,11 +61,16 @@ pub fn execute_plan(workspace: &Path, args: ApplyPlanArgs) -> Result<()> { ); } - // Preflight: load and validate every package up front, before touching any - // files, so a mismatched version or other plan error aborts without leaving - // the workspace half-edited. The parsed manifests are reused by the apply - // loop below, so each is read and validated exactly once. - let mut manifests = Vec::with_capacity(plan.packages.len()); + // Preflight: validate every package up front, before touching any files, so + // a mismatched version or other plan error aborts without leaving the + // workspace half-edited. + // + // We deliberately do NOT reuse the manifests parsed here in the apply loop + // below. Bumping a package rewrites the on-disk manifests of its workspace + // dependents, so a package bumped after its dependencies must be re-read to + // observe those rewrites. Saving a snapshot taken here would write it back + // stale and silently revert every dependency bump an earlier step applied. + let mut bump_decisions = Vec::with_capacity(plan.packages.len()); for step in plan.packages.iter() { let package = CargoToml::new(workspace, step.package).with_context(|| { format!( @@ -76,13 +80,13 @@ pub fn execute_plan(workspace: &Path, args: ApplyPlanArgs) -> Result<()> { })?; match validate_package(&package, step)? { - Preflight::Bump => manifests.push(Some(package)), + Preflight::Bump => bump_decisions.push(true), Preflight::AlreadyReleased => { println!( "Package {} is already at version {}. Skipping.", step.package, step.new_version ); - manifests.push(None); + bump_decisions.push(false); } } } @@ -107,13 +111,22 @@ pub fn execute_plan(workspace: &Path, args: ApplyPlanArgs) -> Result<()> { println!("Dry run: would merge PR changelog entries into CHANGELOG.md / MIGRATING-*.md"); } - // Make code changes, reusing the manifests validated above. Packages that - // were already at their target version are stored as `None` and skipped. + // Make code changes. Re-read each manifest from disk instead of reusing the + // preflight copies: earlier steps in this loop may have rewritten this + // package's dependency versions on disk, and saving a stale in-memory copy + // would revert them. Packages already at their target version are skipped. let skip_dependent_rewrites = plan.backport.is_some(); - for (step, manifest) in plan.packages.iter_mut().zip(manifests) { - let Some(mut package) = manifest else { + for (step, should_bump) in plan.packages.iter_mut().zip(bump_decisions) { + if !should_bump { continue; - }; + } + + let mut package = CargoToml::new(workspace, step.package).with_context(|| { + format!( + "Couldn't create Cargo.toml in workspace {workspace:?} for {:?}", + step.package + ) + })?; let new_version = update_package( &mut package, From b31271ce646b4dc16678b6f653fdd94b8c9df68d Mon Sep 17 00:00:00 2001 From: Scott Mabin Date: Mon, 24 Aug 2026 15:49:45 +0100 Subject: [PATCH 2/2] xtask: make release PR upsert idempotent find_existing_pr can miss a just-created PR because GitHub's PR listing lags for a moment after execute-plan force-pushes the release branch. On a re-run the lookup returned nothing, so `gh pr create` ran a second time and re-applied every release label and re-requested code-owner reviews, which showed up as duplicated PR timeline entries. Recover from `gh pr create`'s "a pull request ... already exists" error by parsing the existing PR number and editing that PR (title and body only) instead of bailing or opening a duplicate. Labels are still applied exactly once, on first creation, so a maintainer removing one to skip its optional CI check is respected. Co-authored-by: Cursor --- xtask/src/commands/release/execute_plan.rs | 179 ++++++++++++++------- 1 file changed, 123 insertions(+), 56 deletions(-) diff --git a/xtask/src/commands/release/execute_plan.rs b/xtask/src/commands/release/execute_plan.rs index edbfb77edae..c6538d677a9 100644 --- a/xtask/src/commands/release/execute_plan.rs +++ b/xtask/src/commands/release/execute_plan.rs @@ -486,7 +486,11 @@ fn head_spec(upstream_url: &str, branch_name: &str) -> Result { } } -fn gh_stdin(args: &[&str], stdin_data: &str) -> Result { +/// Spawn `gh` with `args`, feed `stdin_data` to its stdin, and return the raw +/// output. A non-zero exit is *not* treated as an error here so callers can +/// inspect stderr (e.g. to detect "a pull request already exists"). Callers +/// that only care about success should use [`gh_stdin`]. +fn gh_run(args: &[&str], stdin_data: &str) -> Result { use std::io::Write; let mut child = Command::new("gh") @@ -504,9 +508,13 @@ fn gh_stdin(args: &[&str], stdin_data: &str) -> Result { .write_all(stdin_data.as_bytes()) .context("Failed to write stdin to gh")?; - let out = child + child .wait_with_output() - .with_context(|| format!("`gh {}` failed", args.join(" ")))?; + .with_context(|| format!("`gh {}` failed", args.join(" "))) +} + +fn gh_stdin(args: &[&str], stdin_data: &str) -> Result { + let out = gh_run(args, stdin_data)?; if !out.status.success() { bail!( "`gh {}` failed: {}", @@ -547,64 +555,100 @@ fn find_existing_pr(branch_name: &str) -> Result> { .and_then(|n| n.as_u64())) } -fn upsert_pull_request(branch: &Branch, plan: &Plan, body: &str) -> Result { - let title = release_subject(plan); - if let Some(num) = find_existing_pr(&branch.name)? { - log::info!("Updating existing release PR #{num}"); - let num_str = num.to_string(); - gh_stdin( - &[ - "pr", - "edit", - &num_str, - "--repo", - UPSTREAM_REPO, - "--title", - &title, - "--body-file", - "-", - ], - body, - )?; - Ok(num) - } else { - let head = head_spec(&branch.upstream, &branch.name)?; - log::info!("Creating release PR (head: {head}, base: {})", plan.base); - let mut args: Vec<&str> = vec![ +/// Extract the numeric PR id from any text containing a `.../pull/` URL. +/// +/// Works for both the URL `gh pr create` prints on success and the +/// "...already exists: " message it prints on failure. A `/pull/new/...` +/// URL (as produced by `git push`) has no number and yields `None`. +fn parse_pr_number(text: &str) -> Option { + let start = text.rfind("/pull/")? + "/pull/".len(); + let digits: String = text[start..] + .chars() + .take_while(|c| c.is_ascii_digit()) + .collect(); + digits.parse().ok() +} + +/// Update the title and body of an existing release PR, leaving labels and +/// reviewers untouched. +fn edit_release_pr(number: u64, title: &str, body: &str) -> Result<()> { + let number = number.to_string(); + gh_stdin( + &[ "pr", - "create", + "edit", + &number, "--repo", UPSTREAM_REPO, - "--base", - &plan.base, - "--head", - &head, "--title", - &title, - ]; - for l in PR_LABELS { - args.push("--label"); - args.push(l); - } - args.push("--body-file"); - args.push("-"); - let stdout = gh_stdin(&args, body)?; - // `gh pr create` prints the PR URL as the last line of stdout. - let url = stdout - .lines() - .rev() - .map(str::trim) - .find(|l| !l.is_empty()) - .unwrap_or(""); - let num = url - .rsplit('/') - .next() - .and_then(|s| s.parse::().ok()) - .with_context(|| { - format!("Failed to parse PR number from `gh pr create` output: {url}") - })?; - Ok(num) + title, + "--body-file", + "-", + ], + body, + )?; + Ok(()) +} + +fn upsert_pull_request(branch: &Branch, plan: &Plan, body: &str) -> Result { + let title = release_subject(plan); + + // Reuse an existing open release PR when we can find one, so re-running the + // release edits the same PR instead of opening (and re-labelling) another. + if let Some(num) = find_existing_pr(&branch.name)? { + log::info!("Updating existing release PR #{num}"); + edit_release_pr(num, &title, body)?; + return Ok(num); + } + + let head = head_spec(&branch.upstream, &branch.name)?; + log::info!("Creating release PR (head: {head}, base: {})", plan.base); + let mut args: Vec<&str> = vec![ + "pr", + "create", + "--repo", + UPSTREAM_REPO, + "--base", + &plan.base, + "--head", + &head, + "--title", + &title, + ]; + // Labels are applied only when the PR is first created. A maintainer may + // later remove one to skip its optional CI check, so the edit path above + // deliberately never re-applies them. + for l in PR_LABELS { + args.push("--label"); + args.push(l); + } + args.push("--body-file"); + args.push("-"); + + let out = gh_run(&args, body)?; + if out.status.success() { + let stdout = String::from_utf8_lossy(&out.stdout); + return parse_pr_number(&stdout).with_context(|| { + format!("Failed to parse PR number from `gh pr create` output: {stdout}") + }); + } + + // GitHub's PR listing lags for a moment after the branch is pushed, so + // `find_existing_pr` can miss a PR that actually exists and we land here on + // a re-run. `gh pr create` then fails with "a pull request ... already + // exists: ". Recover by editing that PR instead of bailing: letting + // `gh pr create` run a second time is what re-applied every release label + // and re-requested reviews, showing up as duplicated PR timeline entries. + let stderr = String::from_utf8_lossy(&out.stderr); + if stderr.contains("already exists") + && let Some(num) = parse_pr_number(&stderr) + { + log::info!("Release PR #{num} already exists - updating it instead of opening a duplicate"); + edit_release_pr(num, &title, body)?; + return Ok(num); } + + bail!("`gh pr create` failed: {stderr}"); } fn print_manual_instructions(branch: &Branch, plan: &Plan, body: &str) -> Result<()> { @@ -669,6 +713,29 @@ branch 'foo' set up to track 'origin/foo'. assert_eq!(url, "https://github.com/bugadani/esp-hal/pull/new/foo"); } + #[test] + fn parse_pr_number_from_create_and_already_exists() { + // `gh pr create` success: bare PR URL on stdout. + assert_eq!( + parse_pr_number("https://github.com/esp-rs/esp-hal/pull/1234\n"), + Some(1234) + ); + + // `gh pr create` failure when the PR already exists: the number must be + // recovered from the message so we can edit instead of duplicating. + let stderr = "a pull request for branch \"release-branch-t9telr\" into branch \"main\" \ + already exists:\nhttps://github.com/esp-rs/esp-hal/pull/6190\n"; + assert_eq!(parse_pr_number(stderr), Some(6190)); + + // A `/pull/new/` URL (from `git push`) carries no PR number. + assert_eq!( + parse_pr_number("https://github.com/esp-rs/esp-hal/pull/new/release-branch-x"), + None + ); + + assert_eq!(parse_pr_number("no pull url here"), None); + } + #[test] fn create_comparison_url() { let cases = [