From 8b206a483bcf832f12854567ff0ac9e174aae75a Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Fri, 21 Jan 2022 19:08:51 +0000 Subject: patch_sync: Conduct unconditional cleanup After any sort of modification, we have to conduct a full cleanup of the workspace. While it's still possible to have a panic cause no cleanup, this is an improvement over the previous work, where cleanup could fail during the transpose stage. Also add a --wip mode to prevent spamming people with emails during testing. BUG=b:209493133 TEST=Running patch_sync transpose locally Change-Id: I01e8a5897ec8eeed8f90c528c567b2ba55613b23 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3407914 Reviewed-by: George Burgess Tested-by: Jordan Abrahams-Whitehead Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/Cargo.lock | 7 +++ llvm_tools/patch_sync/Cargo.toml | 1 + llvm_tools/patch_sync/src/main.rs | 69 +++++++++++++++++++----- llvm_tools/patch_sync/src/version_control.rs | 79 ++++++++++++++++++++-------- 4 files changed, 120 insertions(+), 36 deletions(-) (limited to 'llvm_tools/patch_sync') diff --git a/llvm_tools/patch_sync/Cargo.lock b/llvm_tools/patch_sync/Cargo.lock index 63a9fcf8..62db2220 100644 --- a/llvm_tools/patch_sync/Cargo.lock +++ b/llvm_tools/patch_sync/Cargo.lock @@ -167,6 +167,7 @@ dependencies = [ "anyhow", "rand", "regex", + "scopeguard", "serde", "serde_json", "sha2", @@ -285,6 +286,12 @@ version = "1.0.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "73b4b750c782965c211b42f022f59af1fbceabdd026623714f104152f1ec149f" +[[package]] +name = "scopeguard" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d29ab0c6d3fc0ee92fe66e2d99f700eab17a8d57d1c1d3b748380fb20baa78cd" + [[package]] name = "serde" version = "1.0.132" diff --git a/llvm_tools/patch_sync/Cargo.toml b/llvm_tools/patch_sync/Cargo.toml index 43082627..ba8817d5 100644 --- a/llvm_tools/patch_sync/Cargo.toml +++ b/llvm_tools/patch_sync/Cargo.toml @@ -15,6 +15,7 @@ serde_json = "1.0" sha2 = "0.9" structopt = "0.3" time = "0.3" +scopeguard = "1.1.0" [dev-dependencies] rand = "0.8" diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index 033691e4..a8a957f9 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -10,6 +10,7 @@ use anyhow::{Context, Result}; use structopt::StructOpt; use patch_parsing::{filter_patches_by_platform, PatchCollection, PatchDictSchema}; +use version_control::RepoSetupContext; fn main() -> Result<()> { match Opt::from_args() { @@ -35,6 +36,7 @@ fn main() -> Result<()> { verbose, dry_run, no_commit, + wip, } => transpose_subcmd(TransposeOpt { cros_checkout_path, cros_reviewers: cros_reviewers @@ -50,6 +52,7 @@ fn main() -> Result<()> { verbose, dry_run, no_commit, + wip, }), } } @@ -68,10 +71,11 @@ fn show_subcmd(args: ShowOpt) -> Result<()> { keep_unmerged, sync, } = args; - let ctx = version_control::RepoSetupContext { + let ctx = RepoSetupContext { cros_checkout: cros_checkout_path, android_checkout: android_checkout_path, sync_before: sync, + wip_mode: true, }; ctx.setup()?; let make_collection = |platform: &str, patches_fp: &Path| -> Result { @@ -109,26 +113,28 @@ struct TransposeOpt { no_commit: bool, cros_reviewers: Vec, android_reviewers: Vec, + wip: bool, } fn transpose_subcmd(args: TransposeOpt) -> Result<()> { - let ctx = version_control::RepoSetupContext { + let ctx = RepoSetupContext { cros_checkout: args.cros_checkout_path, android_checkout: args.android_checkout_path, sync_before: args.sync, + wip_mode: args.wip, }; ctx.setup()?; let cros_patches_path = ctx.cros_patches_path(); let android_patches_path = ctx.android_patches_path(); // Get new Patches ------------------------------------------------------- - let (mut cur_cros_collection, new_cros_patches) = patch_parsing::new_patches( + let (cur_cros_collection, new_cros_patches) = patch_parsing::new_patches( &cros_patches_path, &ctx.old_cros_patch_contents(&args.old_cros_ref)?, "chromiumos", ) .context("finding new patches for chromiumos")?; - let (mut cur_android_collection, new_android_patches) = patch_parsing::new_patches( + let (cur_android_collection, new_android_patches) = patch_parsing::new_patches( &android_patches_path, &ctx.old_android_patch_contents(&args.old_android_ref)?, "android", @@ -170,27 +176,59 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { return Ok(()); } + modify_repos( + &ctx, + args.no_commit, + ModifyOpt { + new_cros_patches, + cur_cros_collection, + cros_reviewers: args.cros_reviewers, + new_android_patches, + cur_android_collection, + android_reviewers: args.android_reviewers, + }, + ) +} + +struct ModifyOpt { + new_cros_patches: PatchCollection, + cur_cros_collection: PatchCollection, + cros_reviewers: Vec, + new_android_patches: PatchCollection, + cur_android_collection: PatchCollection, + android_reviewers: Vec, +} + +fn modify_repos(ctx: &RepoSetupContext, no_commit: bool, opt: ModifyOpt) -> Result<()> { + // Cleanup on scope exit. + scopeguard::defer! { + ctx.cleanup(); + } // Transpose Patches ----------------------------------------------------- - if !new_cros_patches.is_empty() { - new_cros_patches.transpose_write(&mut cur_android_collection)?; + let mut cur_android_collection = opt.cur_android_collection; + let mut cur_cros_collection = opt.cur_cros_collection; + if !opt.new_cros_patches.is_empty() { + opt.new_cros_patches + .transpose_write(&mut cur_android_collection)?; } - if !new_android_patches.is_empty() { - new_android_patches.transpose_write(&mut cur_cros_collection)?; + if !opt.new_android_patches.is_empty() { + opt.new_android_patches + .transpose_write(&mut cur_cros_collection)?; } - if args.no_commit { + if no_commit { println!("--no-commit specified; not committing or uploading"); return Ok(()); } // Commit and upload for review ------------------------------------------ // Note we want to check if the android patches are empty for CrOS, and // vice versa. This is a little counterintuitive. - if !new_android_patches.is_empty() { - ctx.cros_repo_upload(&args.cros_reviewers) + if !opt.new_android_patches.is_empty() { + ctx.cros_repo_upload(&opt.cros_reviewers) .context("uploading chromiumos changes")?; } - if !new_cros_patches.is_empty() { - ctx.android_repo_upload(&args.android_reviewers) + if !opt.new_cros_patches.is_empty() { + ctx.android_repo_upload(&opt.android_reviewers) .context("uploading android changes")?; } Ok(()) @@ -269,5 +307,10 @@ enum Opt { /// Do not commit or upload any changes made. #[structopt(long)] no_commit: bool, + + /// Upload and send things for review, but mark as WIP and send no + /// emails. + #[structopt(long)] + wip: bool, }, } diff --git a/llvm_tools/patch_sync/src/version_control.rs b/llvm_tools/patch_sync/src/version_control.rs index b45177f7..dc43523e 100644 --- a/llvm_tools/patch_sync/src/version_control.rs +++ b/llvm_tools/patch_sync/src/version_control.rs @@ -10,6 +10,7 @@ const ANDROID_LLVM_REL_PATH: &str = "toolchain/llvm_android"; const CROS_MAIN_BRANCH: &str = "main"; const ANDROID_MAIN_BRANCH: &str = "master"; // nocheck +const WORK_BRANCH_NAME: &str = "__patch_sync_tmp"; /// Context struct to keep track of both Chromium OS and Android checkouts. #[derive(Debug)] @@ -18,6 +19,7 @@ pub struct RepoSetupContext { pub android_checkout: PathBuf, /// Run `repo sync` before doing any comparisons. pub sync_before: bool, + pub wip_mode: bool, } impl RepoSetupContext { @@ -55,9 +57,12 @@ impl RepoSetupContext { extra_args.push("--re"); extra_args.push(reviewer.as_ref()); } + if self.wip_mode { + extra_args.push("--wip"); + extra_args.push("--no-emails"); + } Self::repo_upload( &self.cros_checkout, - CROS_MAIN_BRANCH, CHROMIUMOS_OVERLAY_REL_PATH, &Self::build_commit_msg( "llvm: Synchronize patches from android", @@ -78,9 +83,12 @@ impl RepoSetupContext { extra_args.push("--re"); extra_args.push(reviewer.as_ref()); } + if self.wip_mode { + extra_args.push("--wip"); + extra_args.push("--no-emails"); + } Self::repo_upload( &self.android_checkout, - ANDROID_MAIN_BRANCH, ANDROID_LLVM_REL_PATH, &Self::build_commit_msg( "Synchronize patches from chromiumos", @@ -92,6 +100,30 @@ impl RepoSetupContext { ) } + fn cros_cleanup(&self) -> Result<()> { + let git_path = self.cros_checkout.join(CHROMIUMOS_OVERLAY_REL_PATH); + Self::cleanup_branch(&git_path, CROS_MAIN_BRANCH, WORK_BRANCH_NAME) + .with_context(|| format!("cleaning up branch {}", WORK_BRANCH_NAME))?; + Ok(()) + } + + fn android_cleanup(&self) -> Result<()> { + let git_path = self.android_checkout.join(ANDROID_LLVM_REL_PATH); + Self::cleanup_branch(&git_path, ANDROID_MAIN_BRANCH, WORK_BRANCH_NAME) + .with_context(|| format!("cleaning up branch {}", WORK_BRANCH_NAME))?; + Ok(()) + } + + /// Wrapper around cleanups to ensure both get run, even if errors appear. + pub fn cleanup(&self) { + if let Err(e) = self.cros_cleanup() { + eprintln!("Failed to clean up chromiumos, continuing: {}", e); + } + if let Err(e) = self.android_cleanup() { + eprintln!("Failed to clean up android, continuing: {}", e); + } + } + /// Get the Android path to the PATCHES.json file pub fn android_patches_path(&self) -> PathBuf { self.android_checkout @@ -125,34 +157,31 @@ impl RepoSetupContext { } fn repo_upload<'a, I: IntoIterator>( - path: &Path, - base_branch: &str, - git_wd: &'a str, + checkout_path: &Path, + subproject_git_wd: &'a str, commit_msg: &str, extra_flags: I, ) -> Result<()> { - let git_path = &path.join(&git_wd); + let git_path = &checkout_path.join(&subproject_git_wd); ensure!( git_path.is_dir(), "git_path {} is not a directory", git_path.display() ); - let branch_name = "__patch_sync_tmp_branch"; - repo_cd_cmd(path, &["start", branch_name, git_wd])?; - let res: Result<()> = { - let base_args = ["upload", "--br", branch_name, "-y", "--verify"]; - let new_args = base_args - .iter() - .copied() - .chain(extra_flags) - .chain(["--", git_wd]); - git_cd_cmd(git_path, &["add", "."]) - .and_then(|_| git_cd_cmd(git_path, &["commit", "-m", commit_msg])) - .and_then(|_| repo_cd_cmd(path, new_args)) - }; - Self::cleanup_branch(git_path, base_branch, branch_name) - .with_context(|| format!("cleaning up branch {}", branch_name))?; - res + repo_cd_cmd( + checkout_path, + &["start", WORK_BRANCH_NAME, subproject_git_wd], + )?; + let base_args = ["upload", "--br", WORK_BRANCH_NAME, "-y", "--verify"]; + let new_args = base_args + .iter() + .copied() + .chain(extra_flags) + .chain(["--", subproject_git_wd]); + git_cd_cmd(git_path, &["add", "."]) + .and_then(|_| git_cd_cmd(git_path, &["commit", "-m", commit_msg])) + .and_then(|_| repo_cd_cmd(checkout_path, new_args))?; + Ok(()) } /// Clean up the git repo after we're done with it. @@ -160,7 +189,11 @@ impl RepoSetupContext { git_cd_cmd(git_path, ["restore", "."])?; git_cd_cmd(git_path, ["clean", "-fd"])?; git_cd_cmd(git_path, ["checkout", base_branch])?; - git_cd_cmd(git_path, ["branch", "-D", rm_branch])?; + // It's acceptable to be able to not delete the branch. This may be + // because the branch does not exist, which is an expected result. + // Since this is a very common case, we won't report any failures related + // to this command failure as it'll pollute the stderr logs. + let _ = git_cd_cmd(git_path, ["branch", "-D", rm_branch]); Ok(()) } -- cgit v1.2.3