From 6683e0e6b2152e8e92e7da9c4008913518e1fbd8 Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Tue, 28 Dec 2021 22:22:49 +0000 Subject: patch_sync: Filter patches by platform Because some patch collections may contain patches which don't apply to our given platform, we don't want to attempt to lookup their hashes. If we do, we may encounter a file-not-found error, as those patches haven't been ported to this platform. Also fixes a bug where no_commit was inverted. BUG=b:209493133 TEST=N/A Change-Id: I8b86133dfc6361919174f9f9b392d756b4304d2b Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3364792 Reviewed-by: George Burgess Tested-by: Jordan Abrahams-Whitehead Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/main.rs | 36 ++++++++++++++++++++++-------- llvm_tools/patch_sync/src/patch_parsing.rs | 8 +++++++ 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index 081ce01a..f2d70edb 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -82,7 +82,8 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { // Chromium OS Patches ---------------------------------------------------- let mut cur_cros_collection = patch_parsing::PatchCollection::parse_from_file(&cros_patches_path) - .context("parsing cros PATCHES.json")?; + .context("parsing cros PATCHES.json")? + .filter_patches(|p| p.platforms.contains("chromiumos")); let new_cros_patches: patch_parsing::PatchCollection = { let cros_old_patches_json = ctx.old_cros_patch_contents(&args.old_cros_ref)?; let old_cros_collection = patch_parsing::PatchCollection::parse_from_str( @@ -95,7 +96,8 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { // Android Patches ------------------------------------------------------- let mut cur_android_collection = patch_parsing::PatchCollection::parse_from_file(&android_patches_path) - .context("parsing android PATCHES.json")?; + .context("parsing android PATCHES.json")? + .filter_patches(|p| p.platforms.contains("android")); let new_android_patches: patch_parsing::PatchCollection = { let android_old_patches_json = ctx.old_android_patch_contents(&args.old_android_ref)?; let old_android_collection = patch_parsing::PatchCollection::parse_from_str( @@ -105,18 +107,34 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { cur_android_collection.subtract(&old_android_collection)? }; + if args.dry_run { + println!("--dry-run specified; skipping modifications"); + return Ok(()); + } + // Transpose Patches ----------------------------------------------------- - new_cros_patches.transpose_write(&mut cur_cros_collection)?; - new_android_patches.transpose_write(&mut cur_android_collection)?; + if !new_cros_patches.is_empty() { + 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 !args.no_commit { + if args.no_commit { + println!("--no-commit specified; not committing or uploading"); return Ok(()); } // Commit and upload for review ------------------------------------------ - ctx.cros_repo_upload() - .context("uploading chromiumos changes")?; - ctx.android_repo_upload() - .context("uploading android changes")?; + // 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() + .context("uploading chromiumos changes")?; + } + if !new_cros_patches.is_empty() { + ctx.android_repo_upload() + .context("uploading android changes")?; + } Ok(()) } diff --git a/llvm_tools/patch_sync/src/patch_parsing.rs b/llvm_tools/patch_sync/src/patch_parsing.rs index 733451ae..befa6ccb 100644 --- a/llvm_tools/patch_sync/src/patch_parsing.rs +++ b/llvm_tools/patch_sync/src/patch_parsing.rs @@ -44,6 +44,14 @@ impl PatchCollection { }) } + /// Copy this collection with patches filtered by given criterion. + pub fn filter_patches(&self, f: impl FnMut(&PatchDictSchema) -> bool) -> Self { + Self { + patches: self.patches.iter().cloned().filter(f).collect(), + workdir: self.workdir.clone(), + } + } + #[allow(dead_code)] /// Return true if the collection is tracking any patches. pub fn is_empty(&self) -> bool { -- cgit v1.2.3 From c5b1bd6c4bb4f3e753c25872b58f1b9a53f3165b Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Wed, 12 Jan 2022 00:31:47 +0000 Subject: patch_sync: Add better command debugging This commit allows the git commands to print out what the command was that they failed at. This is pretty helpful in debugging what went wrong. We don't capture the repo output ever, so that can remained piped to the terminal. But we need to do some trickery for the git cmd. BUG=b:209493133 TEST=None Change-Id: I389257fef1e3bf394fb4013588df6c78e83b733a Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3379478 Reviewed-by: George Burgess Reviewed-by: Michael Benfield Tested-by: Jordan Abrahams-Whitehead Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/version_control.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/llvm_tools/patch_sync/src/version_control.rs b/llvm_tools/patch_sync/src/version_control.rs index 3dc5aae9..3621a909 100644 --- a/llvm_tools/patch_sync/src/version_control.rs +++ b/llvm_tools/patch_sync/src/version_control.rs @@ -179,9 +179,16 @@ where I: IntoIterator, S: AsRef, { - let output = Command::new("git").current_dir(&pwd).args(args).output()?; + let mut command = Command::new("git"); + command.current_dir(&pwd).args(args); + let output = command.output()?; if !output.status.success() { - bail!("git command failed") + bail!( + "git command failed:\n {:?}\nstdout --\n{}\nstderr --\n{}", + command, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); } Ok(output) } @@ -191,9 +198,11 @@ where I: IntoIterator, S: AsRef, { - let status = Command::new("repo").current_dir(&pwd).args(args).status()?; + let mut command = Command::new("repo"); + command.current_dir(&pwd).args(args); + let status = command.status()?; if !status.success() { - bail!("repo command failed") + bail!("repo command failed:\n {:?} \n", command) } Ok(()) } -- cgit v1.2.3 From 500f764595891c07c8de9e6bfaa2b16d8628e561 Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Wed, 12 Jan 2022 00:29:50 +0000 Subject: patch_sync: Commit features This adds several important version control features: * Changes branches on set up. * Cleans up any changes made, even if upload fails. * Sends commits for review (enabled via CLI). * Enables CQ for CrOS. BUG=b:209493133 TEST=cargo test Change-Id: I8ab2650aae301c08fd80358162a285e46d44e3e1 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3379479 Reviewed-by: George Burgess Commit-Queue: Jordan Abrahams-Whitehead Tested-by: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/main.rs | 29 +++++-- llvm_tools/patch_sync/src/version_control.rs | 113 +++++++++++++++++++++------ 2 files changed, 114 insertions(+), 28 deletions(-) diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index f2d70edb..cec523bb 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -2,6 +2,7 @@ mod patch_parsing; mod version_control; use anyhow::{Context, Result}; +use std::borrow::ToOwned; use std::path::PathBuf; use structopt::StructOpt; @@ -14,8 +15,10 @@ fn main() -> Result<()> { } => show_subcmd(cros_checkout_path, android_checkout_path, sync), Opt::Transpose { cros_checkout_path, + cros_reviewers, old_cros_ref, android_checkout_path, + android_reviewers, old_android_ref, sync, verbose, @@ -23,8 +26,13 @@ fn main() -> Result<()> { no_commit, } => transpose_subcmd(TransposeOpt { cros_checkout_path, + cros_reviewers: cros_reviewers.split(',').map(ToOwned::to_owned).collect(), old_cros_ref, android_checkout_path, + android_reviewers: android_reviewers + .split(',') + .map(ToOwned::to_owned) + .collect(), old_android_ref, sync, verbose, @@ -67,6 +75,8 @@ struct TransposeOpt { verbose: bool, dry_run: bool, no_commit: bool, + cros_reviewers: Vec, + android_reviewers: Vec, } fn transpose_subcmd(args: TransposeOpt) -> Result<()> { @@ -128,11 +138,11 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { // 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() + ctx.cros_repo_upload(&args.cros_reviewers) .context("uploading chromiumos changes")?; } if !new_cros_patches.is_empty() { - ctx.android_repo_upload() + ctx.android_repo_upload(&args.android_reviewers) .context("uploading android changes")?; } Ok(()) @@ -158,6 +168,11 @@ enum Opt { #[structopt(long = "cros-checkout", parse(from_os_str))] cros_checkout_path: PathBuf, + /// Emails to send review requests to during Chromium OS upload. + /// Comma separated. + #[structopt(long = "cros-rev")] + cros_reviewers: String, + /// Git ref (e.g. hash) for the ChromiumOS overlay to use as the base. #[structopt(long = "overlay-base-ref")] old_cros_ref: String, @@ -166,6 +181,11 @@ enum Opt { #[structopt(long = "aosp-checkout", parse(from_os_str))] android_checkout_path: PathBuf, + /// Emails to send review requests to during Android upload. + /// Comma separated. + #[structopt(long = "aosp-rev")] + android_reviewers: String, + /// Git ref (e.g. hash) for the llvm_android repo to use as the base. #[structopt(long = "aosp-base-ref")] old_android_ref: String, @@ -179,12 +199,11 @@ enum Opt { verbose: bool, /// Do not change any files. Useful in combination with `--verbose` - /// Implies `--no-commit` and `--no-upload`. + /// Implies `--no-commit`. #[structopt(long)] dry_run: bool, - /// Do not commit any changes made. - /// Implies `--no-upload`. + /// Do not commit or upload any changes made. #[structopt(long)] no_commit: bool, }, diff --git a/llvm_tools/patch_sync/src/version_control.rs b/llvm_tools/patch_sync/src/version_control.rs index 3621a909..a19c16e5 100644 --- a/llvm_tools/patch_sync/src/version_control.rs +++ b/llvm_tools/patch_sync/src/version_control.rs @@ -8,6 +8,9 @@ use std::process::{Command, Output}; const CHROMIUMOS_OVERLAY_REL_PATH: &str = "src/third_party/chromiumos-overlay"; const ANDROID_LLVM_REL_PATH: &str = "toolchain/llvm_android"; +const CROS_MAIN_BRANCH: &str = "main"; +const ANDROID_MAIN_BRANCH: &str = "master"; // nocheck + /// Context struct to keep track of both Chromium OS and Android checkouts. #[derive(Debug)] pub struct RepoSetupContext { @@ -20,13 +23,23 @@ pub struct RepoSetupContext { impl RepoSetupContext { pub fn setup(&self) -> Result<()> { if self.sync_before { + { + let crpp = self.cros_patches_path(); + let cros_git = crpp.parent().unwrap(); + git_cd_cmd(cros_git, ["checkout", CROS_MAIN_BRANCH])?; + } + { + let anpp = self.android_patches_path(); + let android_git = anpp.parent().unwrap(); + git_cd_cmd(android_git, ["checkout", ANDROID_MAIN_BRANCH])?; + } repo_cd_cmd(&self.cros_checkout, &["sync", CHROMIUMOS_OVERLAY_REL_PATH])?; repo_cd_cmd(&self.android_checkout, &["sync", ANDROID_LLVM_REL_PATH])?; } Ok(()) } - pub fn cros_repo_upload(&self) -> Result<()> { + pub fn cros_repo_upload>(&self, reviewers: &[S]) -> Result<()> { let llvm_dir = self .cros_checkout .join(&CHROMIUMOS_OVERLAY_REL_PATH) @@ -37,36 +50,48 @@ impl RepoSetupContext { llvm_dir.display() ); Self::rev_bump_llvm(&llvm_dir)?; + let mut extra_args = vec!["--label=Commit-Queue+1"]; + for reviewer in reviewers { + extra_args.push("--re"); + extra_args.push(reviewer.as_ref()); + } Self::repo_upload( &self.cros_checkout, + CROS_MAIN_BRANCH, CHROMIUMOS_OVERLAY_REL_PATH, - &Self::build_commit_msg("android", "chromiumos", "BUG=None\nTEST=CQ"), + &Self::build_commit_msg( + "llvm: Synchronize patches from android", + "android", + "chromiumos", + "BUG=None\nTEST=CQ", + ), + extra_args, ) } - pub fn android_repo_upload(&self) -> Result<()> { + pub fn android_repo_upload>(&self, reviewers: &[S]) -> Result<()> { + let mut extra_args = Vec::new(); + // TODO(ajordanr): Presubmit ready can only be enabled if we + // have the permissions. + // extra_args.push("--label=Presubmit-Ready+1"); + for reviewer in reviewers { + extra_args.push("--re"); + extra_args.push(reviewer.as_ref()); + } Self::repo_upload( &self.android_checkout, + ANDROID_MAIN_BRANCH, ANDROID_LLVM_REL_PATH, - &Self::build_commit_msg("chromiumos", "android", "Test: N/A"), + &Self::build_commit_msg( + "Synchronize patches from chromiumos", + "chromiumos", + "android", + "Test: N/A", + ), + extra_args, ) } - fn repo_upload(path: &Path, git_wd: &str, commit_msg: &str) -> Result<()> { - // TODO(ajordanr): Need to clean up if there's any failures during upload. - let git_path = &path.join(&git_wd); - ensure!( - git_path.is_dir(), - "git_path {} is not a directory", - git_path.display() - ); - repo_cd_cmd(path, &["start", "patch_sync_branch", git_wd])?; - git_cd_cmd(git_path, &["add", "."])?; - git_cd_cmd(git_path, &["commit", "-m", commit_msg])?; - repo_cd_cmd(path, &["upload", "-y", "--verify", git_wd])?; - Ok(()) - } - pub fn android_patches_path(&self) -> PathBuf { self.android_checkout .join(&ANDROID_LLVM_REL_PATH) @@ -79,6 +104,46 @@ impl RepoSetupContext { .join("sys-devel/llvm/files/PATCHES.json") } + fn repo_upload<'a, I: IntoIterator>( + path: &Path, + base_branch: &str, + git_wd: &'a str, + commit_msg: &str, + extra_flags: I, + ) -> Result<()> { + let git_path = &path.join(&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 + } + + /// Clean up the git repo after we're done with it. + fn cleanup_branch(git_path: &Path, base_branch: &str, rm_branch: &str) -> Result<()> { + 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])?; + Ok(()) + } + /// Increment LLVM's revision number fn rev_bump_llvm(llvm_dir: &Path) -> Result { let ebuild = find_ebuild(llvm_dir) @@ -146,11 +211,13 @@ impl RepoSetupContext { } /// Create the commit message - fn build_commit_msg(from: &str, to: &str, footer: &str) -> String { + fn build_commit_msg(subj: &str, from: &str, to: &str, footer: &str) -> String { format!( - "[patch_sync] Synchronize patches from {}\n\n\ - Copies new PATCHES.json changes from {} to {}\n\n{}", - from, from, to, footer + "[patch_sync] {}\n\n\ +Copies new PATCHES.json changes from {} to {}.\n +For questions about this job, contact chromeos-toolchain@google.com\n\n +{}", + subj, from, to, footer ) } } -- cgit v1.2.3 From 384bee7f9cdad4c183e7339985a26f3b9e9e31ee Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Wed, 12 Jan 2022 23:37:47 +0000 Subject: patch_sync: Fix PatchDictSchema fields These were out of alphabetical order, and serde_json would write them in the incorrect order to the PATCHES.json file. Additionally, the `platforms` field may end up being null after some discussion with the Android team. This converts platforms to an Optional accordingly. BUG=b:209493133 TEST=patch_sync transpose <...> Change-Id: I7c0b2e984d713698a0ed53e7f36b38bef1a49d1c Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3379480 Reviewed-by: George Burgess Tested-by: Jordan Abrahams-Whitehead Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/patch_parsing.rs | 38 ++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/llvm_tools/patch_sync/src/patch_parsing.rs b/llvm_tools/patch_sync/src/patch_parsing.rs index befa6ccb..c89d4eb2 100644 --- a/llvm_tools/patch_sync/src/patch_parsing.rs +++ b/llvm_tools/patch_sync/src/patch_parsing.rs @@ -10,11 +10,12 @@ use sha2::{Digest, Sha256}; /// JSON serde struct. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct PatchDictSchema { - pub rel_patch_path: String, - pub start_version: Option, pub end_version: Option, - pub platforms: BTreeSet, pub metadata: Option>, + #[serde(default, skip_serializing_if = "BTreeSet::is_empty")] + pub platforms: BTreeSet, + pub rel_patch_path: String, + pub start_version: Option, } /// Struct to keep track of patches and their relative paths. @@ -59,7 +60,7 @@ impl PatchCollection { } /// Compute the set-set subtraction, returning a new `PatchCollection` which - /// keeps the minuend's wordir. + /// keeps the minuend's workdir. pub fn subtract(&self, subtrahend: &Self) -> Result { let mut new_patches = Vec::new(); // This is O(n^2) when it could be much faster, but n is always going to be less @@ -248,6 +249,7 @@ fn copy_create_parents(from: &Path, to: &Path) -> Result<()> { #[cfg(test)] mod test { + use super::*; /// Test we can extract the hash from patch files. @@ -318,4 +320,32 @@ mod test { vec!["x", "y"] ); } + + #[test] + fn test_union_empties() { + let patch1 = PatchDictSchema { + start_version: Some(0), + end_version: Some(1), + rel_patch_path: "a".into(), + metadata: None, + platforms: Default::default(), + }; + let collection1 = PatchCollection { + workdir: PathBuf::new(), + patches: vec![patch1.clone()], + }; + let collection2 = PatchCollection { + workdir: PathBuf::new(), + patches: vec![patch1], + }; + let union = collection1 + .union_helper( + &collection2, + |p| Ok(p.rel_patch_path.to_string()), + |p| Ok(p.rel_patch_path.to_string()), + ) + .expect("could not create union"); + assert_eq!(union.patches.len(), 1); + assert_eq!(union.patches[0].platforms.len(), 0); + } } -- cgit v1.2.3 From b0d6ea52f9ebad2924b04f6a87da28b1256480b1 Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Wed, 12 Jan 2022 23:38:30 +0000 Subject: patch_sync: Refactor shared main code This commit attempts to remove duplicated code between Android and CrOS, as the complexity between this duplicated code exceeded the scope of the main file. Also adds display_patches code. This is to pretty print what patches are getting applied and to where. Only enabled if --verbose is on. Also does some minor function cleanup in version_control.rs. BUG=b:209493133 TEST=cargo test TEST=patch_sync transpose --dry-run --verbose <...> Change-Id: I63410160ff5159f4c079ac1cc189674fe3fc02a0 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3379481 Reviewed-by: George Burgess Tested-by: Jordan Abrahams-Whitehead Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/main.rs | 62 ++++++++++++------------- llvm_tools/patch_sync/src/patch_parsing.rs | 67 +++++++++++++++++++++++++++- llvm_tools/patch_sync/src/version_control.rs | 41 +++++++++-------- 3 files changed, 116 insertions(+), 54 deletions(-) diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index cec523bb..bfcba584 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -2,6 +2,7 @@ mod patch_parsing; mod version_control; use anyhow::{Context, Result}; +use patch_parsing::PatchCollection; use std::borrow::ToOwned; use std::path::PathBuf; use structopt::StructOpt; @@ -55,11 +56,10 @@ fn show_subcmd( ctx.setup()?; let cros_patches_path = ctx.cros_patches_path(); let android_patches_path = ctx.android_patches_path(); - let cur_cros_collection = patch_parsing::PatchCollection::parse_from_file(&cros_patches_path) + let cur_cros_collection = PatchCollection::parse_from_file(&cros_patches_path) .context("could not parse cros PATCHES.json")?; - let cur_android_collection = - patch_parsing::PatchCollection::parse_from_file(&android_patches_path) - .context("could not parse android PATCHES.json")?; + let cur_android_collection = PatchCollection::parse_from_file(&android_patches_path) + .context("could not parse android PATCHES.json")?; let merged = cur_cros_collection.union(&cur_android_collection)?; println!("{}", merged.serialize_patches()?); Ok(()) @@ -89,33 +89,24 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { let cros_patches_path = ctx.cros_patches_path(); let android_patches_path = ctx.android_patches_path(); - // Chromium OS Patches ---------------------------------------------------- - let mut cur_cros_collection = - patch_parsing::PatchCollection::parse_from_file(&cros_patches_path) - .context("parsing cros PATCHES.json")? - .filter_patches(|p| p.platforms.contains("chromiumos")); - let new_cros_patches: patch_parsing::PatchCollection = { - let cros_old_patches_json = ctx.old_cros_patch_contents(&args.old_cros_ref)?; - let old_cros_collection = patch_parsing::PatchCollection::parse_from_str( - cros_patches_path.parent().unwrap().to_path_buf(), - &cros_old_patches_json, - )?; - cur_cros_collection.subtract(&old_cros_collection)? - }; - - // Android Patches ------------------------------------------------------- - let mut cur_android_collection = - patch_parsing::PatchCollection::parse_from_file(&android_patches_path) - .context("parsing android PATCHES.json")? - .filter_patches(|p| p.platforms.contains("android")); - let new_android_patches: patch_parsing::PatchCollection = { - let android_old_patches_json = ctx.old_android_patch_contents(&args.old_android_ref)?; - let old_android_collection = patch_parsing::PatchCollection::parse_from_str( - android_patches_path.parent().unwrap().to_path_buf(), - &android_old_patches_json, - )?; - cur_android_collection.subtract(&old_android_collection)? - }; + // Get new Patches ------------------------------------------------------- + let (mut 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( + &android_patches_path, + &ctx.old_android_patch_contents(&args.old_android_ref)?, + "android", + ) + .context("finding new patches for android")?; + + if args.verbose { + display_patches("New patches from Chromium OS", &new_cros_patches); + display_patches("New patches from Android", &new_android_patches); + } if args.dry_run { println!("--dry-run specified; skipping modifications"); @@ -148,6 +139,15 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { Ok(()) } +fn display_patches(prelude: &str, collection: &PatchCollection) { + println!("{}", prelude); + if collection.patches.is_empty() { + println!(" [No Patches]"); + return; + } + println!("{}", collection); +} + #[derive(Debug, structopt::StructOpt)] #[structopt(name = "patch_sync", about = "A pipeline for syncing the patch code")] enum Opt { diff --git a/llvm_tools/patch_sync/src/patch_parsing.rs b/llvm_tools/patch_sync/src/patch_parsing.rs index c89d4eb2..1da2c2a6 100644 --- a/llvm_tools/patch_sync/src/patch_parsing.rs +++ b/llvm_tools/patch_sync/src/patch_parsing.rs @@ -53,7 +53,14 @@ impl PatchCollection { } } - #[allow(dead_code)] + /// Map over the patches. + pub fn map_patches(&self, f: impl FnMut(&PatchDictSchema) -> PatchDictSchema) -> Self { + Self { + patches: self.patches.iter().map(f).collect(), + workdir: self.workdir.clone(), + } + } + /// Return true if the collection is tracking any patches. pub fn is_empty(&self) -> bool { self.patches.is_empty() @@ -196,11 +203,67 @@ impl PatchCollection { Ok(std::str::from_utf8(&serialization_buffer)?.to_string()) } + /// Return whether a given patch actually exists on the file system. + pub fn patch_exists(&self, patch: &PatchDictSchema) -> bool { + self.workdir.join(&patch.rel_patch_path).exists() + } + fn hash_from_rel_patch(&self, patch: &PatchDictSchema) -> Result { hash_from_patch_path(&self.workdir.join(&patch.rel_patch_path)) } } +impl std::fmt::Display for PatchCollection { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + for (i, p) in self.patches.iter().enumerate() { + let title = p + .metadata + .as_ref() + .and_then(|x| x.get("title")) + .and_then(serde_json::Value::as_str) + .unwrap_or("[No Title]"); + let path = self.workdir.join(&p.rel_patch_path); + writeln!(f, "* {}", title)?; + if i == self.patches.len() - 1 { + write!(f, " {}", path.display())?; + } else { + writeln!(f, " {}", path.display())?; + } + } + Ok(()) + } +} + +/// Generate a PatchCollection incorporating only the diff between current patches and old patch +/// contents. +pub fn new_patches( + patches_path: &Path, + old_patch_contents: &str, + platform: &str, +) -> Result<(PatchCollection, PatchCollection)> { + let cur_collection = PatchCollection::parse_from_file(patches_path) + .with_context(|| format!("parsing {} PATCHES.json", platform))? + .filter_patches(|p| p.platforms.contains(platform)); + let cur_collection = cur_collection.filter_patches(|p| cur_collection.patch_exists(p)); + let new_patches: PatchCollection = { + let old_collection = PatchCollection::parse_from_str( + patches_path.parent().unwrap().to_path_buf(), + old_patch_contents, + )?; + let old_collection = old_collection.filter_patches(|p| old_collection.patch_exists(p)); + cur_collection.subtract(&old_collection)? + }; + let new_patches = new_patches.map_patches(|p| PatchDictSchema { + platforms: BTreeSet::from(["android".to_string(), "chromiumos".to_string()]) + .union(&p.platforms) + .cloned() + .collect(), + + ..p.to_owned() + }); + Ok((cur_collection, new_patches)) +} + /// Get the hash from the patch file contents. /// /// Not every patch file actually contains its own hash, @@ -228,7 +291,7 @@ fn hash_from_patch(patch_contents: impl Read) -> Result { } fn hash_from_patch_path(patch: &Path) -> Result { - let f = File::open(patch)?; + let f = File::open(patch).with_context(|| format!("opening patch file {}", patch.display()))?; hash_from_patch(f) } diff --git a/llvm_tools/patch_sync/src/version_control.rs b/llvm_tools/patch_sync/src/version_control.rs index a19c16e5..e6d32a58 100644 --- a/llvm_tools/patch_sync/src/version_control.rs +++ b/llvm_tools/patch_sync/src/version_control.rs @@ -92,18 +92,38 @@ impl RepoSetupContext { ) } + /// Get the Android path to the PATCHES.json file pub fn android_patches_path(&self) -> PathBuf { self.android_checkout .join(&ANDROID_LLVM_REL_PATH) .join("patches/PATCHES.json") } + /// Get the Chromium OS path to the PATCHES.json file pub fn cros_patches_path(&self) -> PathBuf { self.cros_checkout .join(&CHROMIUMOS_OVERLAY_REL_PATH) .join("sys-devel/llvm/files/PATCHES.json") } + /// Return the contents of the old PATCHES.json from Chromium OS + pub fn old_cros_patch_contents(&self, hash: &str) -> Result { + Self::old_file_contents( + hash, + &self.cros_checkout.join(CHROMIUMOS_OVERLAY_REL_PATH), + Path::new("sys-devel/llvm/files/PATCHES.json"), + ) + } + + /// Return the contents of the old PATCHES.json from android + pub fn old_android_patch_contents(&self, hash: &str) -> Result { + Self::old_file_contents( + hash, + &self.android_checkout.join(ANDROID_LLVM_REL_PATH), + Path::new("patches/PATCHES.json"), + ) + } + fn repo_upload<'a, I: IntoIterator>( path: &Path, base_branch: &str, @@ -173,28 +193,7 @@ impl RepoSetupContext { Ok(new_path) } - /// Return the contents of the old PATCHES.json from Chromium OS - #[allow(dead_code)] - pub fn old_cros_patch_contents(&self, hash: &str) -> Result { - Self::old_file_contents( - hash, - &self.cros_checkout.join(CHROMIUMOS_OVERLAY_REL_PATH), - Path::new("sys-devel/llvm/files/PATCHES.json"), - ) - } - - /// Return the contents of the old PATCHES.json from android - #[allow(dead_code)] - pub fn old_android_patch_contents(&self, hash: &str) -> Result { - Self::old_file_contents( - hash, - &self.android_checkout.join(ANDROID_LLVM_REL_PATH), - Path::new("patches/PATCHES.json"), - ) - } - /// Return the contents of an old file in git - #[allow(dead_code)] fn old_file_contents(hash: &str, pwd: &Path, file: &Path) -> Result { let git_ref = format!( "{}:{}", -- cgit v1.2.3 From d3fe9cbd37edaa1f6cda95e1866fa3bcf86c7eb2 Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Wed, 12 Jan 2022 00:51:24 +0000 Subject: patch_sync: Bugfix for ebuild finding Previously, the find_ebuild function could return the original ebuild instead of the llvm symlink. This would lead to incorrect uprevs, depending on how the file system returned the order of the files. Adds a test case to prevent this from happening in the future. BUG=b:209493133 TEST=cargo test Change-Id: Ia47ecee2a9c5b6ce0559e316c5227f70dcf87833 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3379482 Reviewed-by: George Burgess Tested-by: Jordan Abrahams-Whitehead Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/version_control.rs | 80 ++++++++++++++++++++++------ 1 file changed, 65 insertions(+), 15 deletions(-) diff --git a/llvm_tools/patch_sync/src/version_control.rs b/llvm_tools/patch_sync/src/version_control.rs index e6d32a58..b45177f7 100644 --- a/llvm_tools/patch_sync/src/version_control.rs +++ b/llvm_tools/patch_sync/src/version_control.rs @@ -223,20 +223,44 @@ For questions about this job, contact chromeos-toolchain@google.com\n\n /// Return the path of an ebuild located within the given directory. fn find_ebuild(dir: &Path) -> Result { - // TODO(ajordanr): Maybe use OnceCell for this regex? - let ebuild_matcher = Regex::new(r"(-r[0-9]+)?\.ebuild").unwrap(); - for entry in fs::read_dir(dir)? { - let path = entry?.path(); - if let Some(name) = path.file_name() { - if ebuild_matcher.is_match( - name.to_str() - .ok_or_else(|| anyhow!("converting filepath to UTF-8"))?, - ) { - return Ok(path); - } + // The logic here is that we create an iterator over all file paths to ebuilds + // with _pre in the name. Then we sort those ebuilds based on their revision numbers. + // Then we return the highest revisioned one. + + let ebuild_rev_matcher = Regex::new(r"-r([0-9]+)\.ebuild").unwrap(); + // For LLVM ebuilds, we only want to check for ebuilds that have this in their file name. + let per_heuristic = "_pre"; + // Get an iterator over all ebuilds with a _per in the file name. + let ebuild_candidates = fs::read_dir(dir)?.filter_map(|entry| { + let entry = entry.ok()?; + let path = entry.path(); + if path.extension()? != "ebuild" { + // Not an ebuild, ignore. + return None; } - } - bail!("could not find ebuild") + let stem = path.file_stem()?.to_str()?; + if stem.contains(per_heuristic) { + return Some(path); + } + None + }); + let try_parse_ebuild_rev = |path: PathBuf| -> Option<(u64, PathBuf)> { + let name = path.file_name()?; + if let Some(rev_match) = ebuild_rev_matcher.captures(name.to_str()?) { + let rev_str = rev_match.get(1)?; + let rev_num = rev_str.as_str().parse::().ok()?; + return Some((rev_num, path)); + } + // If it doesn't have a revision, then it's revision 0. + Some((0, path)) + }; + let mut sorted_candidates: Vec<_> = + ebuild_candidates.filter_map(try_parse_ebuild_rev).collect(); + sorted_candidates.sort_unstable_by_key(|x| x.0); + let highest_rev_ebuild = sorted_candidates + .pop() + .ok_or_else(|| anyhow!("could not find ebuild"))?; + Ok(highest_rev_ebuild.1) } /// Run a given git command from inside a specified git dir. @@ -294,7 +318,11 @@ mod test { File::create(&ebuild_path).expect("creating test ebuild file"); let new_ebuild_path = RepoSetupContext::rev_bump_llvm(&llvm_dir).expect("rev bumping the ebuild"); - assert!(new_ebuild_path.ends_with("llvm-13.0_pre433403_p20211019-r11.ebuild")); + assert!( + new_ebuild_path.ends_with("llvm-13.0_pre433403_p20211019-r11.ebuild"), + "{}", + new_ebuild_path.display() + ); fs::remove_file(new_ebuild_path).expect("removing renamed ebuild file"); } { @@ -304,9 +332,31 @@ mod test { File::create(&ebuild_path).expect("creating test ebuild file"); let new_ebuild_path = RepoSetupContext::rev_bump_llvm(&llvm_dir).expect("rev bumping the ebuild"); - assert!(new_ebuild_path.ends_with("llvm-13.0_pre433403_p20211019-r1.ebuild")); + assert!( + new_ebuild_path.ends_with("llvm-13.0_pre433403_p20211019-r1.ebuild"), + "{}", + new_ebuild_path.display() + ); fs::remove_file(new_ebuild_path).expect("removing renamed ebuild file"); } + { + // With both + let ebuild_name = "llvm-13.0_pre433403_p20211019.ebuild"; + let ebuild_path = llvm_dir.join(ebuild_name); + File::create(&ebuild_path).expect("creating test ebuild file"); + let ebuild_link_name = "llvm-13.0_pre433403_p20211019-r2.ebuild"; + let ebuild_link_path = llvm_dir.join(ebuild_link_name); + File::create(&ebuild_link_path).expect("creating test ebuild link file"); + let new_ebuild_path = + RepoSetupContext::rev_bump_llvm(&llvm_dir).expect("rev bumping the ebuild"); + assert!( + new_ebuild_path.ends_with("llvm-13.0_pre433403_p20211019-r3.ebuild"), + "{}", + new_ebuild_path.display() + ); + fs::remove_file(new_ebuild_path).expect("removing renamed ebuild link file"); + fs::remove_file(ebuild_path).expect("removing renamed ebuild file"); + } fs::remove_dir(&llvm_dir).expect("removing temp test dir"); } -- cgit v1.2.3 From 86739250cf453a73ff88b064c51a09b99c93b99f Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Wed, 12 Jan 2022 00:57:09 +0000 Subject: patch_sync: Filter patches better This commit introduces better patch filtering. Specifically, * it prevents patches from being reintroduced to repos which already have those patches in their own PATCHES.json. * it only applies android patches which are within the desired version range. BUG=b:209493133 TEST=patch_sync transpose --no-commit -s <...> Change-Id: I667a095395a36edf290e5e652ae40efaa2df7d57 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3382194 Reviewed-by: George Burgess Tested-by: Jordan Abrahams-Whitehead Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/android_utils.rs | 33 ++++++++++++++++++++++++++++++ llvm_tools/patch_sync/src/main.rs | 26 +++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 llvm_tools/patch_sync/src/android_utils.rs diff --git a/llvm_tools/patch_sync/src/android_utils.rs b/llvm_tools/patch_sync/src/android_utils.rs new file mode 100644 index 00000000..c6c1cd50 --- /dev/null +++ b/llvm_tools/patch_sync/src/android_utils.rs @@ -0,0 +1,33 @@ +use std::path::Path; +use std::process::Command; + +use anyhow::{bail, ensure, Result}; + +/// Return the Android checkout's current llvm version. +/// +/// This uses android_version.get_svn_revision_number, a python function +/// that can't be executed directly. We spawn a Python3 program +/// to run it and get the result from that. +pub fn get_android_llvm_version(android_checkout: &Path) -> Result { + let mut command = Command::new("python3"); + let llvm_android_dir = android_checkout.join("toolchain/llvm_android"); + ensure!( + llvm_android_dir.is_dir(), + "can't get android llvm version; {} is not a directory", + llvm_android_dir.display() + ); + command.current_dir(llvm_android_dir); + command.args([ + "-c", + "import android_version; print(android_version.get_svn_revision_number(), end='')", + ]); + let output = command.output()?; + if !output.status.success() { + bail!( + "could not get android llvm version: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + let out_string = String::from_utf8(output.stdout)?.trim().to_string(); + Ok(out_string) +} diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index bfcba584..9616716c 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -1,3 +1,4 @@ +mod android_utils; mod patch_parsing; mod version_control; @@ -103,6 +104,31 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { ) .context("finding new patches for android")?; + // Have to ignore patches that are already at the destination, even if + // the patches are new. + let new_cros_patches = new_cros_patches.subtract(&cur_android_collection)?; + let new_android_patches = new_android_patches.subtract(&cur_cros_collection)?; + + // Need to do an extra filtering step for Android, as AOSP doesn't + // want patches outside of the start/end bounds. + let android_llvm_version: u64 = { + let android_llvm_version_str = + android_utils::get_android_llvm_version(&ctx.android_checkout)?; + android_llvm_version_str.parse::().with_context(|| { + format!( + "converting llvm version to u64: '{}'", + android_llvm_version_str + ) + })? + }; + let new_android_patches = + new_android_patches.filter_patches(|p| match (p.start_version, p.end_version) { + (Some(start), Some(end)) => start <= android_llvm_version && android_llvm_version < end, + (Some(start), None) => start <= android_llvm_version, + (None, Some(end)) => android_llvm_version < end, + (None, None) => true, + }); + if args.verbose { display_patches("New patches from Chromium OS", &new_cros_patches); display_patches("New patches from Android", &new_android_patches); -- cgit v1.2.3 From 8337ced7d44c9d0d37ef87bbfb77913ed4f19c88 Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Thu, 13 Jan 2022 21:48:15 +0000 Subject: patch_sync: Rework show cmd to display merged At present, patch_sync show merges two PATCHES.json files by merging their platform contents. However, because of timing, review latency, or just patches being denied, it's possible that one repo recommends a patch being applied to a certain platform, and the other denies that same patch. To resolve this conflict, this commit by default only considers patches that exist in the PATCHES.json file at the present time. The original behaviour can be enabled by turning on --keep-unmerged. BUG=b:209493133 TEST=patch_sync show <...cros> <...android> TEST=patch_sync transpose --dry-run -s <...> Change-Id: I3bdf6c36b4dbfe26d4221191b5c22363a7f0dfe0 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3388390 Reviewed-by: George Burgess Tested-by: Jordan Abrahams-Whitehead Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/main.rs | 59 +++++++++++++++++++++++------- llvm_tools/patch_sync/src/patch_parsing.rs | 14 ++++++- 2 files changed, 58 insertions(+), 15 deletions(-) diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index 9616716c..8c1eff1c 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -2,19 +2,28 @@ mod android_utils; mod patch_parsing; mod version_control; -use anyhow::{Context, Result}; -use patch_parsing::PatchCollection; use std::borrow::ToOwned; -use std::path::PathBuf; +use std::collections::BTreeSet; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; use structopt::StructOpt; +use patch_parsing::{filter_patches_by_platform, PatchCollection, PatchDictSchema}; + fn main() -> Result<()> { match Opt::from_args() { Opt::Show { cros_checkout_path, android_checkout_path, sync, - } => show_subcmd(cros_checkout_path, android_checkout_path, sync), + keep_unmerged, + } => show_subcmd(ShowOpt { + cros_checkout_path, + android_checkout_path, + sync, + keep_unmerged, + }), Opt::Transpose { cros_checkout_path, cros_reviewers, @@ -44,29 +53,47 @@ fn main() -> Result<()> { } } -fn show_subcmd( +struct ShowOpt { cros_checkout_path: PathBuf, android_checkout_path: PathBuf, + keep_unmerged: bool, sync: bool, -) -> Result<()> { +} + +fn show_subcmd(args: ShowOpt) -> Result<()> { + let ShowOpt { + cros_checkout_path, + android_checkout_path, + keep_unmerged, + sync, + } = args; let ctx = version_control::RepoSetupContext { cros_checkout: cros_checkout_path, android_checkout: android_checkout_path, sync_before: sync, }; ctx.setup()?; - let cros_patches_path = ctx.cros_patches_path(); - let android_patches_path = ctx.android_patches_path(); - let cur_cros_collection = PatchCollection::parse_from_file(&cros_patches_path) - .context("could not parse cros PATCHES.json")?; - let cur_android_collection = PatchCollection::parse_from_file(&android_patches_path) - .context("could not parse android PATCHES.json")?; + let make_collection = |platform: &str, patches_fp: &Path| -> Result { + let parsed_collection = PatchCollection::parse_from_file(patches_fp) + .with_context(|| format!("could not parse {} PATCHES.json", platform))?; + Ok(if keep_unmerged { + parsed_collection + } else { + filter_patches_by_platform(&parsed_collection, platform).map_patches(|p| { + PatchDictSchema { + platforms: BTreeSet::from([platform.to_string()]), + ..p.clone() + } + }) + }) + }; + let cur_cros_collection = make_collection("chromiumos", &ctx.cros_patches_path())?; + let cur_android_collection = make_collection("android", &ctx.android_patches_path())?; let merged = cur_cros_collection.union(&cur_android_collection)?; println!("{}", merged.serialize_patches()?); Ok(()) } -#[allow(dead_code)] struct TransposeOpt { cros_checkout_path: PathBuf, old_cros_ref: String, @@ -184,6 +211,12 @@ enum Opt { cros_checkout_path: PathBuf, #[structopt(parse(from_os_str))] android_checkout_path: PathBuf, + + /// Keep a patch's platform field even if it's not merged at that platform. + #[structopt(long)] + keep_unmerged: bool, + + /// Run repo sync before transposing. #[structopt(short, long)] sync: bool, }, diff --git a/llvm_tools/patch_sync/src/patch_parsing.rs b/llvm_tools/patch_sync/src/patch_parsing.rs index 1da2c2a6..2f0fbc87 100644 --- a/llvm_tools/patch_sync/src/patch_parsing.rs +++ b/llvm_tools/patch_sync/src/patch_parsing.rs @@ -242,8 +242,8 @@ pub fn new_patches( platform: &str, ) -> Result<(PatchCollection, PatchCollection)> { let cur_collection = PatchCollection::parse_from_file(patches_path) - .with_context(|| format!("parsing {} PATCHES.json", platform))? - .filter_patches(|p| p.platforms.contains(platform)); + .with_context(|| format!("parsing {} PATCHES.json", platform))?; + let cur_collection = filter_patches_by_platform(&cur_collection, platform); let cur_collection = cur_collection.filter_patches(|p| cur_collection.patch_exists(p)); let new_patches: PatchCollection = { let old_collection = PatchCollection::parse_from_str( @@ -264,6 +264,16 @@ pub fn new_patches( Ok((cur_collection, new_patches)) } +/// Create a new collection with only the patches that apply to the +/// given platform. +/// +/// If there's no platform listed, the patch should still apply if the patch file exists. +pub fn filter_patches_by_platform(collection: &PatchCollection, platform: &str) -> PatchCollection { + collection.filter_patches(|p| { + p.platforms.contains(platform) || (p.platforms.is_empty() && collection.patch_exists(p)) + }) +} + /// Get the hash from the patch file contents. /// /// Not every patch file actually contains its own hash, -- cgit v1.2.3 From 96968ca5d35291869c6801b7e96f89b3c7d49249 Mon Sep 17 00:00:00 2001 From: Manoj Gupta Date: Sat, 15 Jan 2022 16:40:27 -0800 Subject: afdo_metadata: Publish the new kernel profiles Update chromeos-kernel-4_4 Update chromeos-kernel-4_14 Update chromeos-kernel-4_19 Update chromeos-kernel-5_4 BUG=None TEST=Verified in kernel-release-afdo-verify-orchestrator Change-Id: Ic1f1bd9979a247f9c20487365595db99f5fd9747 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3392443 Tested-by: Manoj Gupta Auto-Submit: Manoj Gupta Reviewed-by: Denis Nikitin Commit-Queue: Denis Nikitin --- afdo_metadata/kernel_afdo.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/afdo_metadata/kernel_afdo.json b/afdo_metadata/kernel_afdo.json index a3275004..a016651f 100644 --- a/afdo_metadata/kernel_afdo.json +++ b/afdo_metadata/kernel_afdo.json @@ -1,14 +1,14 @@ { "chromeos-kernel-4_4": { - "name": "R99-14388.8-1640601492" + "name": "R99-14440.0-1641810997" }, "chromeos-kernel-4_14": { - "name": "R99-14407.0-1640601365" + "name": "R99-14440.0-1641811258" }, "chromeos-kernel-4_19": { - "name": "R99-14388.8-1640601108" + "name": "R99-14440.0-1641811049" }, "chromeos-kernel-5_4": { - "name": "R99-14397.0-1640601195" + "name": "R99-14440.0-1641811130" } } -- cgit v1.2.3 From 72d98e4ca9ac0147b1025836214c88696919c532 Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Tue, 18 Jan 2022 13:22:09 -0800 Subject: rust_watch: use better titles for bugs These titles are more consistent with bugs filed for Rust upgrades in the past. BUG=b:215209865 TEST=Unittests Change-Id: I1673bda6a033f70a276f6cfe8af17709db985feb Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3399867 Tested-by: George Burgess Auto-Submit: George Burgess Reviewed-by: Michael Benfield Commit-Queue: George Burgess --- rust_tools/rust_watch.py | 2 +- rust_tools/rust_watch_test.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/rust_tools/rust_watch.py b/rust_tools/rust_watch.py index db6ae71b..c347d2c6 100755 --- a/rust_tools/rust_watch.py +++ b/rust_tools/rust_watch.py @@ -241,7 +241,7 @@ def maybe_compose_bug( if newest_release == old_state.last_seen_release: return None - title = f'New rustc release detected: v{newest_release}' + title = f'[Rust] Update to {newest_release}' body = ('A new release has been detected; we should probably roll to it. ' "Please see go/crostc-rust-rotation for who's turn it is.") return title, body diff --git a/rust_tools/rust_watch_test.py b/rust_tools/rust_watch_test.py index 30bacbb9..583a9125 100755 --- a/rust_tools/rust_watch_test.py +++ b/rust_tools/rust_watch_test.py @@ -129,7 +129,7 @@ class Test(unittest.TestCase): ), newest_release=rust_watch.RustReleaseVersion(1, 0, 1), ) - self.assertEqual(title, 'New rustc release detected: v1.0.1') + self.assertEqual(title, '[Rust] Update to 1.0.1') self.assertTrue(body.startswith('A new release has been detected;')) title, body = rust_watch.maybe_compose_bug( @@ -139,7 +139,7 @@ class Test(unittest.TestCase): ), newest_release=rust_watch.RustReleaseVersion(1, 1, 0), ) - self.assertEqual(title, 'New rustc release detected: v1.1.0') + self.assertEqual(title, '[Rust] Update to 1.1.0') self.assertTrue(body.startswith('A new release has been detected;')) title, body = rust_watch.maybe_compose_bug( @@ -149,7 +149,7 @@ class Test(unittest.TestCase): ), newest_release=rust_watch.RustReleaseVersion(2, 0, 0), ) - self.assertEqual(title, 'New rustc release detected: v2.0.0') + self.assertEqual(title, '[Rust] Update to 2.0.0') self.assertTrue(body.startswith('A new release has been detected;')) def test_compose_bug_does_nothing_when_no_new_updates_exist(self): -- cgit v1.2.3 From f96fffcd3c3bcfa3af9fed129aed3466ee9adc1b Mon Sep 17 00:00:00 2001 From: Denis Nikitin Date: Tue, 18 Jan 2022 10:44:16 -0800 Subject: afdo_metadata: Publish the new kernel profiles Update chromeos-kernel-4_4 Update chromeos-kernel-4_14 Update chromeos-kernel-4_19 Update chromeos-kernel-5_4 BUG=None TEST=Verified in kernel-release-afdo-verify-orchestrator Change-Id: Id770cd48f9c286a26d18fd24654eeea076fd886b Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3399468 Tested-by: Denis Nikitin Reviewed-by: Ryan Beltran Commit-Queue: Denis Nikitin --- afdo_metadata/kernel_afdo.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/afdo_metadata/kernel_afdo.json b/afdo_metadata/kernel_afdo.json index a016651f..82f8201f 100644 --- a/afdo_metadata/kernel_afdo.json +++ b/afdo_metadata/kernel_afdo.json @@ -1,14 +1,14 @@ { "chromeos-kernel-4_4": { - "name": "R99-14440.0-1641810997" + "name": "R99-14447.0-1642415921" }, "chromeos-kernel-4_14": { - "name": "R99-14440.0-1641811258" + "name": "R99-14453.0-1642415754" }, "chromeos-kernel-4_19": { - "name": "R99-14440.0-1641811049" + "name": "R99-14447.0-1642415503" }, "chromeos-kernel-5_4": { - "name": "R99-14440.0-1641811130" + "name": "R99-14447.0-1642415491" } } -- cgit v1.2.3 From 86d317a4fdb5f603df849b7056815db5ebe30804 Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Wed, 19 Jan 2022 23:51:36 +0000 Subject: patch_sync: Fix for Rust 1.55 compat, cli patch On chrotomation, we still use Rust 1.55. Rust 1.55 does not have the "from" implementation for arrays, so we must build the BTrees by hand. Additionally, this removes the requirement for having the review strings be set. BUG=b:209493133 TEST=cargo check Change-Id: I6bc16e96cd56775c8c80667395f3dc3fb4857356 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3403387 Tested-by: Jordan Abrahams-Whitehead Reviewed-by: George Burgess Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/main.rs | 18 +++++++++++------- llvm_tools/patch_sync/src/patch_parsing.rs | 14 +++++++------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index 8c1eff1c..033691e4 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -37,13 +37,14 @@ fn main() -> Result<()> { no_commit, } => transpose_subcmd(TransposeOpt { cros_checkout_path, - cros_reviewers: cros_reviewers.split(',').map(ToOwned::to_owned).collect(), + cros_reviewers: cros_reviewers + .map(|r| r.split(',').map(ToOwned::to_owned).collect()) + .unwrap_or_default(), old_cros_ref, android_checkout_path, android_reviewers: android_reviewers - .split(',') - .map(ToOwned::to_owned) - .collect(), + .map(|r| r.split(',').map(ToOwned::to_owned).collect()) + .unwrap_or_default(), old_android_ref, sync, verbose, @@ -80,8 +81,11 @@ fn show_subcmd(args: ShowOpt) -> Result<()> { parsed_collection } else { filter_patches_by_platform(&parsed_collection, platform).map_patches(|p| { + // Need to do this platforms creation as Rust 1.55 cannot use "from". + let mut platforms = BTreeSet::new(); + platforms.insert(platform.to_string()); PatchDictSchema { - platforms: BTreeSet::from([platform.to_string()]), + platforms, ..p.clone() } }) @@ -230,7 +234,7 @@ enum Opt { /// Emails to send review requests to during Chromium OS upload. /// Comma separated. #[structopt(long = "cros-rev")] - cros_reviewers: String, + cros_reviewers: Option, /// Git ref (e.g. hash) for the ChromiumOS overlay to use as the base. #[structopt(long = "overlay-base-ref")] @@ -243,7 +247,7 @@ enum Opt { /// Emails to send review requests to during Android upload. /// Comma separated. #[structopt(long = "aosp-rev")] - android_reviewers: String, + android_reviewers: Option, /// Git ref (e.g. hash) for the llvm_android repo to use as the base. #[structopt(long = "aosp-base-ref")] diff --git a/llvm_tools/patch_sync/src/patch_parsing.rs b/llvm_tools/patch_sync/src/patch_parsing.rs index 2f0fbc87..581b1899 100644 --- a/llvm_tools/patch_sync/src/patch_parsing.rs +++ b/llvm_tools/patch_sync/src/patch_parsing.rs @@ -253,13 +253,13 @@ pub fn new_patches( let old_collection = old_collection.filter_patches(|p| old_collection.patch_exists(p)); cur_collection.subtract(&old_collection)? }; - let new_patches = new_patches.map_patches(|p| PatchDictSchema { - platforms: BTreeSet::from(["android".to_string(), "chromiumos".to_string()]) - .union(&p.platforms) - .cloned() - .collect(), - - ..p.to_owned() + let new_patches = new_patches.map_patches(|p| { + let mut platforms = BTreeSet::new(); + platforms.extend(["android".to_string(), "chromiumos".to_string()]); + PatchDictSchema { + platforms: platforms.union(&p.platforms).cloned().collect(), + ..p.to_owned() + } }); Ok((cur_collection, new_patches)) } -- cgit v1.2.3 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(-) 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 From 6c2fb76f73585aca8ef4e1e78f4396d9d4ddf103 Mon Sep 17 00:00:00 2001 From: Denis Nikitin Date: Fri, 28 Jan 2022 13:56:46 -0800 Subject: afdo_metadata: Publish the new kernel profiles Update chromeos-kernel-4_4 Update chromeos-kernel-4_14 Update chromeos-kernel-4_19 Update chromeos-kernel-5_4 BUG=None TEST=Verified in kernel-release-afdo-verify-orchestrator Change-Id: I9227de3d695a364ec8dfc85cf79c823af3dee9c3 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3425677 Reviewed-by: Bob Haarman Tested-by: Denis Nikitin Commit-Queue: Denis Nikitin --- afdo_metadata/kernel_afdo.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/afdo_metadata/kernel_afdo.json b/afdo_metadata/kernel_afdo.json index 82f8201f..456b1a97 100644 --- a/afdo_metadata/kernel_afdo.json +++ b/afdo_metadata/kernel_afdo.json @@ -1,14 +1,14 @@ { "chromeos-kernel-4_4": { - "name": "R99-14447.0-1642415921" + "name": "R99-14455.0-1643020830" }, "chromeos-kernel-4_14": { - "name": "R99-14453.0-1642415754" + "name": "R99-14455.0-1643020500" }, "chromeos-kernel-4_19": { - "name": "R99-14447.0-1642415503" + "name": "R99-14455.0-1643020735" }, "chromeos-kernel-5_4": { - "name": "R99-14447.0-1642415491" + "name": "R99-14455.0-1643020569" } } -- cgit v1.2.3 From 3dc6ca12ad631a8826fed1558d877982ce3a0bb3 Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Mon, 24 Jan 2022 00:08:35 -0800 Subject: compiler_wrapper: disable ccache during src_configure This seems to speed things up in cmake by 10% or so, which is consistent with expectations (using `ccache` adds 15-30ms in experimental testing; cmake checks are often faster than that). BUG=b:215747936 TEST=CQ Change-Id: I1a12e50277b37af7bb0b6a58fea2de10006983c8 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3411542 Reviewed-by: Manoj Gupta Commit-Queue: George Burgess Tested-by: George Burgess --- compiler_wrapper/ccache_flag.go | 7 +++++++ compiler_wrapper/ccache_flag_test.go | 13 +++++++++++++ 2 files changed, 20 insertions(+) diff --git a/compiler_wrapper/ccache_flag.go b/compiler_wrapper/ccache_flag.go index 265b8fc2..02fb43ac 100644 --- a/compiler_wrapper/ccache_flag.go +++ b/compiler_wrapper/ccache_flag.go @@ -19,6 +19,13 @@ func processCCacheFlag(builder *commandBuilder) { return arg.value }) + // Disable ccache during portage's src_configure phase. Using ccache here is generally a + // waste of time, since these files are very small. Experimentally, this speeds up + // configuring by ~13%. + if val, present := builder.env.getenv("EBUILD_PHASE"); present && val == "configure" { + useCCache = false + } + if builder.cfg.useCCache && useCCache { // Note: we used to also set CCACHE_BASEDIR but don't do it // anymore for reasons outlined in crrev.com/c/2103170. diff --git a/compiler_wrapper/ccache_flag_test.go b/compiler_wrapper/ccache_flag_test.go index 50205312..d6eeb926 100644 --- a/compiler_wrapper/ccache_flag_test.go +++ b/compiler_wrapper/ccache_flag_test.go @@ -174,3 +174,16 @@ func TestRusagePreventsCCache(t *testing.T) { } }) } + +func TestCcacheIsDisabledInSrcConfigure(t *testing.T) { + withCCacheEnabledTestContext(t, func(ctx *testContext) { + ctx.NoteTestWritesToUmask() + + ctx.env = append(ctx.env, "EBUILD_PHASE=configure") + cmd := ctx.must(callCompiler(ctx, ctx.cfg, + ctx.newCommand(gccX86_64, mainCc))) + if err := verifyPath(cmd, gccX86_64+".real"); err != nil { + t.Error(err) + } + }) +} -- cgit v1.2.3 From 872c973050d1b105c054e1724d1201797d2f8bae Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Tue, 1 Feb 2022 10:18:09 -0800 Subject: nightly_revert_checker: fix breakage We were missing `platforms` here, leading to crashes. Seems best to default to all platforms getting each revert. BUG=b:216107005 TEST=Ran the revert checker; no more crashes. Change-Id: I44658bb01e1c45977ed70c77572cac8a1fd61586 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3429680 Reviewed-by: Jordan Abrahams-Whitehead Commit-Queue: George Burgess Tested-by: George Burgess --- llvm_tools/nightly_revert_checker.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/llvm_tools/nightly_revert_checker.py b/llvm_tools/nightly_revert_checker.py index 5e878816..8e3fcf57 100755 --- a/llvm_tools/nightly_revert_checker.py +++ b/llvm_tools/nightly_revert_checker.py @@ -32,8 +32,8 @@ import get_upstream_patch State = t.Any -def _find_interesting_android_shas(android_llvm_toolchain_dir: str - ) -> t.List[t.Tuple[str, str]]: +def _find_interesting_android_shas( + android_llvm_toolchain_dir: str) -> t.List[t.Tuple[str, str]]: llvm_project = os.path.join(android_llvm_toolchain_dir, 'toolchain/llvm-project') @@ -54,8 +54,8 @@ def _find_interesting_android_shas(android_llvm_toolchain_dir: str return result -def _parse_llvm_ebuild_for_shas(ebuild_file: io.TextIOWrapper - ) -> t.List[t.Tuple[str, str]]: +def _parse_llvm_ebuild_for_shas( + ebuild_file: io.TextIOWrapper) -> t.List[t.Tuple[str, str]]: def parse_ebuild_assignment(line: str) -> str: no_comments = line.split('#')[0] no_assign = no_comments.split('=', 1)[1].strip() @@ -82,8 +82,8 @@ def _parse_llvm_ebuild_for_shas(ebuild_file: io.TextIOWrapper return results -def _find_interesting_chromeos_shas(chromeos_base: str - ) -> t.List[t.Tuple[str, str]]: +def _find_interesting_chromeos_shas( + chromeos_base: str) -> t.List[t.Tuple[str, str]]: llvm_dir = os.path.join(chromeos_base, 'src/third_party/chromiumos-overlay/sys-devel/llvm') candidate_ebuilds = [ @@ -230,12 +230,15 @@ def do_cherrypick(chroot_path: str, llvm_dir: str, seen.add(friendly_name) for sha, reverted_sha in reverts: try: + # We upload reverts for all platforms by default, since there's no + # real reason for them to be CrOS-specific. get_upstream_patch.get_from_upstream(chroot_path=chroot_path, create_cl=True, start_sha=reverted_sha, patches=[sha], reviewers=reviewers, - cc=cc) + cc=cc, + platforms=()) except get_upstream_patch.CherrypickError as e: logging.info('%s, skipping...', str(e)) return new_state @@ -324,8 +327,9 @@ def parse_args(argv: t.List[str]) -> t.Any: return parser.parse_args(argv) -def find_chroot(opts: t.Any, reviewers: t.List[str], cc: t.List[str] - ) -> t.Tuple[str, t.List[t.Tuple[str, str]], _EmailRecipients]: +def find_chroot( + opts: t.Any, reviewers: t.List[str], cc: t.List[str] +) -> t.Tuple[str, t.List[t.Tuple[str, str]], _EmailRecipients]: recipients = reviewers + cc if opts.repository == 'chromeos': chroot_path = opts.chromeos_dir -- cgit v1.2.3 From 82ea0615fe9f6438df4669f17e4f31808788f019 Mon Sep 17 00:00:00 2001 From: Luis Lozano Date: Tue, 1 Feb 2022 21:46:26 -0800 Subject: afdo_metadata: Publish the new kernel profiles Update chromeos-kernel-4_4 Update chromeos-kernel-4_14 Update chromeos-kernel-4_19 Update chromeos-kernel-5_4 BUG=None TEST=Verified in kernel-release-afdo-verify-orchestrator Change-Id: I42377d5f11a392fc13dcb76622eab67d25495421 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3430587 Tested-by: Luis Lozano Auto-Submit: Luis Lozano Reviewed-by: Denis Nikitin Commit-Queue: Denis Nikitin --- afdo_metadata/kernel_afdo.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/afdo_metadata/kernel_afdo.json b/afdo_metadata/kernel_afdo.json index 456b1a97..84e62e55 100644 --- a/afdo_metadata/kernel_afdo.json +++ b/afdo_metadata/kernel_afdo.json @@ -1,14 +1,14 @@ { "chromeos-kernel-4_4": { - "name": "R99-14455.0-1643020830" + "name": "R99-14455.0-1643625613" }, "chromeos-kernel-4_14": { - "name": "R99-14455.0-1643020500" + "name": "R99-14455.0-1643625453" }, "chromeos-kernel-4_19": { - "name": "R99-14455.0-1643020735" + "name": "R99-14455.0-1643625497" }, "chromeos-kernel-5_4": { - "name": "R99-14455.0-1643020569" + "name": "R99-14455.0-1643625229" } } -- cgit v1.2.3 From 3527566c2b13b4977e10281a1cf62b07f1242be0 Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Fri, 4 Feb 2022 01:29:03 +0000 Subject: patch_sync: Sort android patches Android requests that patches are sorted. They use their own __lt__ implementation in cherrypick_cl.py, which we should leverage to keep the sorting stable and robust to implementation details. BUG=b:217767120 TEST=patch_sync transpose <...> Change-Id: I3013b66c4552fd47052e15009df252cdcdc245ad Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3440375 Reviewed-by: George Burgess Reviewed-by: Pirama Arumuga Nainar Commit-Queue: Jordan Abrahams-Whitehead Tested-by: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/android_utils.rs | 45 ++++++++++++++++++++++++------ llvm_tools/patch_sync/src/main.rs | 6 ++++ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/llvm_tools/patch_sync/src/android_utils.rs b/llvm_tools/patch_sync/src/android_utils.rs index c6c1cd50..77cb4b8a 100644 --- a/llvm_tools/patch_sync/src/android_utils.rs +++ b/llvm_tools/patch_sync/src/android_utils.rs @@ -3,20 +3,15 @@ use std::process::Command; use anyhow::{bail, ensure, Result}; +const LLVM_ANDROID_REL_PATH: &str = "toolchain/llvm_android"; + /// Return the Android checkout's current llvm version. /// /// This uses android_version.get_svn_revision_number, a python function /// that can't be executed directly. We spawn a Python3 program /// to run it and get the result from that. pub fn get_android_llvm_version(android_checkout: &Path) -> Result { - let mut command = Command::new("python3"); - let llvm_android_dir = android_checkout.join("toolchain/llvm_android"); - ensure!( - llvm_android_dir.is_dir(), - "can't get android llvm version; {} is not a directory", - llvm_android_dir.display() - ); - command.current_dir(llvm_android_dir); + let mut command = new_android_cmd(android_checkout, "python3")?; command.args([ "-c", "import android_version; print(android_version.get_svn_revision_number(), end='')", @@ -31,3 +26,37 @@ pub fn get_android_llvm_version(android_checkout: &Path) -> Result { let out_string = String::from_utf8(output.stdout)?.trim().to_string(); Ok(out_string) } + +/// Sort the Android patches using the cherrypick_cl.py Android utility. +/// +/// This assumes that: +/// 1. There exists a python script called cherrypick_cl.py +/// 2. That calling it with the given arguments sorts the PATCHES.json file. +/// 3. Calling it does nothing besides sorting the PATCHES.json file. +/// +/// We aren't doing our own sorting because we shouldn't have to update patch_sync along +/// with cherrypick_cl.py any time they change the __lt__ implementation. +pub fn sort_android_patches(android_checkout: &Path) -> Result<()> { + let mut command = new_android_cmd(android_checkout, "python3")?; + command.args(["cherrypick_cl.py", "--reason", "patch_sync sorting"]); + let output = command.output()?; + if !output.status.success() { + bail!( + "could not sort: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + Ok(()) +} + +fn new_android_cmd(android_checkout: &Path, cmd: &str) -> Result { + let mut command = Command::new(cmd); + let llvm_android_dir = android_checkout.join(LLVM_ANDROID_REL_PATH); + ensure!( + llvm_android_dir.is_dir(), + "can't make android command; {} is not a directory", + llvm_android_dir.display() + ); + command.current_dir(llvm_android_dir); + Ok(command) +} diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index a8a957f9..58276151 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -228,6 +228,12 @@ fn modify_repos(ctx: &RepoSetupContext, no_commit: bool, opt: ModifyOpt) -> Resu .context("uploading chromiumos changes")?; } if !opt.new_cros_patches.is_empty() { + if let Err(e) = android_utils::sort_android_patches(&ctx.android_checkout) { + eprintln!( + "Couldn't sort Android patches; continuing. Caused by: {}", + e + ); + } ctx.android_repo_upload(&opt.android_reviewers) .context("uploading android changes")?; } -- cgit v1.2.3 From 76d717633c0aa3371a3f9c28fdbf7092af81d9a0 Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Wed, 9 Feb 2022 20:41:54 +0000 Subject: patch_sync: Toggle CQ for CrOS, Android Currently, patch sync will always run CQ for CrOS, but never for Android. This was because the chrotomation bot didn't have the proper permissions to run the needed Presubmit-Ready checks. Now the bot does in fact have permission to enable the Presubmit check on Android, but some users may not have this permission when running locally. This commit introduces the ability to toggle running the presubmit/CQ checks early through the --disable-cq flags (by default it's assumed that running these checks is desirable). BUG=None TEST=patch_sync transpose --disable-cq <...> TEST=patch_sync transpose <...> Change-Id: I6185b98aa4394ba22f8433541885a450855cfbb4 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3451037 Reviewed-by: George Burgess Commit-Queue: Jordan Abrahams-Whitehead Tested-by: Jordan Abrahams-Whitehead --- llvm_tools/patch_sync/src/main.rs | 11 ++++++++++- llvm_tools/patch_sync/src/version_control.rs | 12 ++++++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index 58276151..b7967c31 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -37,6 +37,7 @@ fn main() -> Result<()> { dry_run, no_commit, wip, + disable_cq, } => transpose_subcmd(TransposeOpt { cros_checkout_path, cros_reviewers: cros_reviewers @@ -53,6 +54,7 @@ fn main() -> Result<()> { dry_run, no_commit, wip, + disable_cq, }), } } @@ -75,7 +77,8 @@ fn show_subcmd(args: ShowOpt) -> Result<()> { cros_checkout: cros_checkout_path, android_checkout: android_checkout_path, sync_before: sync, - wip_mode: true, + wip_mode: true, // Has no effect, as we're not making changes + enable_cq: false, // Has no effect, as we're not uploading anything }; ctx.setup()?; let make_collection = |platform: &str, patches_fp: &Path| -> Result { @@ -114,6 +117,7 @@ struct TransposeOpt { cros_reviewers: Vec, android_reviewers: Vec, wip: bool, + disable_cq: bool, } fn transpose_subcmd(args: TransposeOpt) -> Result<()> { @@ -122,6 +126,7 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { android_checkout: args.android_checkout_path, sync_before: args.sync, wip_mode: args.wip, + enable_cq: !args.disable_cq, }; ctx.setup()?; let cros_patches_path = ctx.cros_patches_path(); @@ -318,5 +323,9 @@ enum Opt { /// emails. #[structopt(long)] wip: bool, + + /// Don't run CQ if set. Only has an effect if uploading. + #[structopt(long)] + disable_cq: bool, }, } diff --git a/llvm_tools/patch_sync/src/version_control.rs b/llvm_tools/patch_sync/src/version_control.rs index dc43523e..e07d39d6 100644 --- a/llvm_tools/patch_sync/src/version_control.rs +++ b/llvm_tools/patch_sync/src/version_control.rs @@ -20,6 +20,7 @@ pub struct RepoSetupContext { /// Run `repo sync` before doing any comparisons. pub sync_before: bool, pub wip_mode: bool, + pub enable_cq: bool, } impl RepoSetupContext { @@ -52,7 +53,7 @@ impl RepoSetupContext { llvm_dir.display() ); Self::rev_bump_llvm(&llvm_dir)?; - let mut extra_args = vec!["--label=Commit-Queue+1"]; + let mut extra_args = Vec::new(); for reviewer in reviewers { extra_args.push("--re"); extra_args.push(reviewer.as_ref()); @@ -61,6 +62,9 @@ impl RepoSetupContext { extra_args.push("--wip"); extra_args.push("--no-emails"); } + if self.enable_cq { + extra_args.push("--label=Commit-Queue+1"); + } Self::repo_upload( &self.cros_checkout, CHROMIUMOS_OVERLAY_REL_PATH, @@ -76,9 +80,6 @@ impl RepoSetupContext { pub fn android_repo_upload>(&self, reviewers: &[S]) -> Result<()> { let mut extra_args = Vec::new(); - // TODO(ajordanr): Presubmit ready can only be enabled if we - // have the permissions. - // extra_args.push("--label=Presubmit-Ready+1"); for reviewer in reviewers { extra_args.push("--re"); extra_args.push(reviewer.as_ref()); @@ -87,6 +88,9 @@ impl RepoSetupContext { extra_args.push("--wip"); extra_args.push("--no-emails"); } + if self.enable_cq { + extra_args.push("--label=Presubmit-Ready+1"); + } Self::repo_upload( &self.android_checkout, ANDROID_LLVM_REL_PATH, -- cgit v1.2.3 From b968618aeec7a7c1bc1e4a6e045226e84f97152d Mon Sep 17 00:00:00 2001 From: Denis Nikitin Date: Fri, 11 Feb 2022 09:56:28 -0800 Subject: afdo_metadata: Publish the new kernel profiles Update chromeos-kernel-4_4 Update chromeos-kernel-4_14 Update chromeos-kernel-4_19 Update chromeos-kernel-5_4 BUG=None TEST=Verified in kernel-release-afdo-verify-orchestrator Change-Id: I388e4710f2c4a9d5d6c9a9a6bf191c3ce3b0c0f1 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3457222 Tested-by: Denis Nikitin Reviewed-by: Manoj Gupta Commit-Queue: Manoj Gupta --- afdo_metadata/kernel_afdo.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/afdo_metadata/kernel_afdo.json b/afdo_metadata/kernel_afdo.json index 84e62e55..b21cd598 100644 --- a/afdo_metadata/kernel_afdo.json +++ b/afdo_metadata/kernel_afdo.json @@ -1,14 +1,14 @@ { "chromeos-kernel-4_4": { - "name": "R99-14455.0-1643625613" + "name": "R100-14469.8-1644230249" }, "chromeos-kernel-4_14": { - "name": "R99-14455.0-1643625453" + "name": "R100-14469.8-1644230170" }, "chromeos-kernel-4_19": { - "name": "R99-14455.0-1643625497" + "name": "R100-14469.8-1644229984" }, "chromeos-kernel-5_4": { - "name": "R99-14455.0-1643625229" + "name": "R100-14496.0-1644229921" } } -- cgit v1.2.3 From f07561c3e22d1199f44519d614eb2241efb0be3c Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Thu, 24 Feb 2022 15:34:58 -0800 Subject: command_executer: reformat `yapf` makes a lot of changes here. Split that out into another (nop) CL. BUG=b:221302420 TEST=Unittests Change-Id: Ieb1f839d6619d7d162ae6a37bff0be77bc0b91a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3489848 Tested-by: George Burgess Auto-Submit: George Burgess Reviewed-by: Manoj Gupta Commit-Queue: Manoj Gupta --- cros_utils/command_executer.py | 136 ++++++++++++++++++++--------------------- 1 file changed, 67 insertions(+), 69 deletions(-) diff --git a/cros_utils/command_executer.py b/cros_utils/command_executer.py index aeedf3ea..0bf89d4d 100755 --- a/cros_utils/command_executer.py +++ b/cros_utils/command_executer.py @@ -103,14 +103,13 @@ class CommandExecuter(object): p = None try: # pylint: disable=bad-option-value, subprocess-popen-preexec-fn - p = subprocess.Popen( - cmd, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - shell=True, - preexec_fn=os.setsid, - executable='/bin/bash', - env=env) + p = subprocess.Popen(cmd, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + shell=True, + preexec_fn=os.setsid, + executable='/bin/bash', + env=env) full_stdout = '' full_stderr = '' @@ -159,16 +158,17 @@ class CommandExecuter(object): if p.poll() is not None: if terminated_time is None: terminated_time = time.time() - elif (terminated_timeout is not None and - time.time() - terminated_time > terminated_timeout): + elif (terminated_timeout is not None + and time.time() - terminated_time > terminated_timeout): if self.logger: self.logger.LogWarning( 'Timeout of %s seconds reached since ' - 'process termination.' % terminated_timeout, print_to_console) + 'process termination.' % terminated_timeout, + print_to_console) break - if (command_timeout is not None and - time.time() - started_time > command_timeout): + if (command_timeout is not None + and time.time() - started_time > command_timeout): os.killpg(os.getpgid(p.pid), signal.SIGTERM) if self.logger: self.logger.LogWarning( @@ -242,9 +242,11 @@ class CommandExecuter(object): return command def WriteToTempShFile(self, contents): - with tempfile.NamedTemporaryFile( - 'w', encoding='utf-8', delete=False, prefix=os.uname()[1], - suffix='.sh') as f: + with tempfile.NamedTemporaryFile('w', + encoding='utf-8', + delete=False, + prefix=os.uname()[1], + suffix='.sh') as f: f.write('#!/bin/bash\n') f.write(contents) f.flush() @@ -292,16 +294,15 @@ class CommandExecuter(object): machine, port = machine.split(':') # Write all commands to a file. command_file = self.WriteToTempShFile(cmd) - retval = self.CopyFiles( - command_file, - command_file, - dest_machine=machine, - dest_port=port, - command_terminator=command_terminator, - chromeos_root=chromeos_root, - dest_cros=True, - recursive=False, - print_to_console=print_to_console) + retval = self.CopyFiles(command_file, + command_file, + dest_machine=machine, + dest_port=port, + command_terminator=command_terminator, + chromeos_root=chromeos_root, + dest_cros=True, + recursive=False, + print_to_console=print_to_console) if retval: if self.logger: self.logger.LogError('Could not run remote command on machine.' @@ -311,13 +312,12 @@ class CommandExecuter(object): command = self.RemoteAccessInitCommand(chromeos_root, machine, port) command += '\nremote_sh bash %s' % command_file command += '\nl_retval=$?; echo "$REMOTE_OUT"; exit $l_retval' - retval = self.RunCommandGeneric( - command, - return_output, - command_terminator=command_terminator, - command_timeout=command_timeout, - terminated_timeout=terminated_timeout, - print_to_console=print_to_console) + retval = self.RunCommandGeneric(command, + return_output, + command_terminator=command_terminator, + command_timeout=command_timeout, + terminated_timeout=terminated_timeout, + print_to_console=print_to_console) if return_output: connect_signature = ('Initiating first contact with remote host\n' + 'Connection OK\n') @@ -372,13 +372,13 @@ class CommandExecuter(object): if self.logger: self.logger.LogCmd(command, print_to_console=print_to_console) - with tempfile.NamedTemporaryFile( - 'w', - encoding='utf-8', - delete=False, - dir=os.path.join(chromeos_root, 'src/scripts'), - suffix='.sh', - prefix='in_chroot_cmd') as f: + with tempfile.NamedTemporaryFile('w', + encoding='utf-8', + delete=False, + dir=os.path.join(chromeos_root, + 'src/scripts'), + suffix='.sh', + prefix='in_chroot_cmd') as f: f.write('#!/bin/bash\n') f.write(command) f.write('\n') @@ -402,14 +402,13 @@ class CommandExecuter(object): command = ("cd %s; cros_sdk %s -- bash -c '%s/%s'" % (chromeos_root, cros_sdk_options, CHROMEOS_SCRIPTS_DIR, os.path.basename(command_file))) - ret = self.RunCommandGeneric( - command, - return_output, - command_terminator=command_terminator, - command_timeout=command_timeout, - terminated_timeout=terminated_timeout, - print_to_console=print_to_console, - env=env) + ret = self.RunCommandGeneric(command, + return_output, + command_terminator=command_terminator, + command_timeout=command_timeout, + terminated_timeout=terminated_timeout, + print_to_console=print_to_console, + env=env) os.remove(command_file) return ret @@ -445,11 +444,10 @@ class CommandExecuter(object): username=None, command_terminator=None): cmd = ' ;\n'.join(cmdlist) - return self.RunCommand( - cmd, - machine=machine, - username=username, - command_terminator=command_terminator) + return self.RunCommand(cmd, + machine=machine, + username=username, + command_terminator=command_terminator) def CopyFiles(self, src, @@ -505,12 +503,11 @@ class CommandExecuter(object): else: command += rsync_prefix + 'root@%s:%s %s' % (cros_machine, src, dest) - return self.RunCommand( - command, - machine=host_machine, - username=host_user, - command_terminator=command_terminator, - print_to_console=print_to_console) + return self.RunCommand(command, + machine=host_machine, + username=host_user, + command_terminator=command_terminator, + print_to_console=print_to_console) if dest_machine == src_machine: command = 'rsync -a %s %s' % (src, dest) @@ -519,12 +516,11 @@ class CommandExecuter(object): src_machine = os.uname()[1] src_user = getpass.getuser() command = 'rsync -a %s@%s:%s %s' % (src_user, src_machine, src, dest) - return self.RunCommand( - command, - machine=dest_machine, - username=dest_user, - command_terminator=command_terminator, - print_to_console=print_to_console) + return self.RunCommand(command, + machine=dest_machine, + username=dest_user, + command_terminator=command_terminator, + print_to_console=print_to_console) def RunCommand2(self, cmd, @@ -593,8 +589,9 @@ class CommandExecuter(object): def notify_line(self): p = self._buf.find('\n') while p >= 0: - self._line_consumer( - line=self._buf[:p + 1], output=self._name, pobject=self._pobject) + self._line_consumer(line=self._buf[:p + 1], + output=self._name, + pobject=self._pobject) if p < len(self._buf) - 1: self._buf = self._buf[p + 1:] p = self._buf.find('\n') @@ -606,8 +603,9 @@ class CommandExecuter(object): def notify_eos(self): # Notify end of stream. The last line may not end with a '\n'. if self._buf != '': - self._line_consumer( - line=self._buf, output=self._name, pobject=self._pobject) + self._line_consumer(line=self._buf, + output=self._name, + pobject=self._pobject) self._buf = '' if self.log_level == 'verbose': -- cgit v1.2.3 From 56465d5c8a1745abcf6d5da395e8958b5df18b75 Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Thu, 24 Feb 2022 15:32:24 -0800 Subject: command_executer: extend timeout for first-time SDK entry As noted in the comment, this can sometimes be an operation that takes quite a while. This is WAI. BUG=b:221302420 TEST=Unittests Change-Id: I21a18713d050b21c593caf2fde69b91e2ffd7c8e Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3489849 Tested-by: George Burgess Auto-Submit: George Burgess Reviewed-by: Manoj Gupta Commit-Queue: Manoj Gupta --- cros_utils/command_executer.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cros_utils/command_executer.py b/cros_utils/command_executer.py index 0bf89d4d..cc0f3372 100755 --- a/cros_utils/command_executer.py +++ b/cros_utils/command_executer.py @@ -393,7 +393,11 @@ class CommandExecuter(object): if return_output: ret = self.RunCommand( 'cd %s; cros_sdk %s -- true' % (chromeos_root, cros_sdk_options), - env=env) + env=env, + # Give this command a long time to execute; it might involve setting + # the chroot up, or running fstrim on its image file. Both of these + # operations can take well over the timeout default of 10 seconds. + terminated_timeout=5 * 60) if ret: return (ret, '', '') -- cgit v1.2.3 From cf276c12f5fbbaf0167c5242b317c712b7162f9d Mon Sep 17 00:00:00 2001 From: Manoj Gupta Date: Sat, 26 Feb 2022 11:14:44 -0800 Subject: afdo_metadata: Publish the new kernel profiles Update chromeos-kernel-4_4 Update chromeos-kernel-4_14 Update chromeos-kernel-4_19 Update chromeos-kernel-5_4 BUG=None TEST=Verified in kernel-release-afdo-verify-orchestrator Change-Id: I98bc1f19694d32c02d1c6cf6e046f6f27f720ba3 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3492484 Tested-by: Manoj Gupta Auto-Submit: Manoj Gupta Reviewed-by: Denis Nikitin Commit-Queue: Denis Nikitin --- afdo_metadata/kernel_afdo.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/afdo_metadata/kernel_afdo.json b/afdo_metadata/kernel_afdo.json index b21cd598..49e19277 100644 --- a/afdo_metadata/kernel_afdo.json +++ b/afdo_metadata/kernel_afdo.json @@ -1,14 +1,14 @@ { "chromeos-kernel-4_4": { - "name": "R100-14469.8-1644230249" + "name": "R100-14516.0-1645439511" }, "chromeos-kernel-4_14": { - "name": "R100-14469.8-1644230170" + "name": "R100-14516.0-1645439661" }, "chromeos-kernel-4_19": { - "name": "R100-14469.8-1644229984" + "name": "R100-14516.0-1645439606" }, "chromeos-kernel-5_4": { - "name": "R100-14496.0-1644229921" + "name": "R100-14516.0-1645439482" } } -- cgit v1.2.3 From 79644ecd692df1a59cc50e9d31514948edb74852 Mon Sep 17 00:00:00 2001 From: Jordan R Abrahams Date: Fri, 25 Feb 2022 22:40:36 +0000 Subject: llvm_tools: Allow version_range for start/end At present, the start_version and end_version information are split as far apart as possible due to alphabetical sorting. This leads to bugs when developers want to modify those bug ranges by hand. This CL is the first step to grouping these version ranges together under the `version_range` property. BUG=b:221489531 TEST=python3 patch_manager_unittest.py TEST=cd patch_sync; cargo test TEST=edit PATCHES.json w/ version_range; patch_sync show <...> Change-Id: I9d0fead07c61c0bd0edb745ed623990f0686b8b2 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3490757 Reviewed-by: George Burgess Tested-by: Jordan Abrahams-Whitehead Commit-Queue: Jordan Abrahams-Whitehead --- llvm_tools/get_upstream_patch.py | 11 +-- llvm_tools/patch_manager.py | 9 ++- llvm_tools/patch_manager_unittest.py | 112 ++++++++++++++++++++--------- llvm_tools/patch_sync/Cargo.lock | 2 +- llvm_tools/patch_sync/Cargo.toml | 2 +- llvm_tools/patch_sync/src/main.rs | 7 +- llvm_tools/patch_sync/src/patch_parsing.rs | 38 ++++++++++ 7 files changed, 136 insertions(+), 45 deletions(-) diff --git a/llvm_tools/get_upstream_patch.py b/llvm_tools/get_upstream_patch.py index 7a4be3eb..5669b023 100755 --- a/llvm_tools/get_upstream_patch.py +++ b/llvm_tools/get_upstream_patch.py @@ -96,15 +96,18 @@ def add_patch(patches_json_path: str, patches_dir: str, cwd=llvm_dir, encoding='utf-8') + end_vers = rev.number if isinstance(rev, git_llvm_rev.Rev) else None patch_props = { 'rel_patch_path': rel_patch_path, - 'start_version': start_version.number, 'metadata': { 'title': commit_subject.strip(), 'info': [], }, 'platforms': sorted(platforms), - 'end_version': rev.number if isinstance(rev, git_llvm_rev.Rev) else None, + 'version_range': { + 'from': start_version.number, + 'until': end_vers, + }, } patches_json.append(patch_props) @@ -346,8 +349,8 @@ def _convert_patch(llvm_config: git_llvm_rev.LLVMConfig, is_differential=is_differential) -def _get_duplicate_shas( - patches: t.List[ParsedPatch]) -> t.List[t.Tuple[ParsedPatch, ParsedPatch]]: +def _get_duplicate_shas(patches: t.List[ParsedPatch] + ) -> t.List[t.Tuple[ParsedPatch, ParsedPatch]]: """Return a list of Patches which have duplicate SHA's""" return [(left, right) for i, left in enumerate(patches) for right in patches[i + 1:] if left.sha == right.sha] diff --git a/llvm_tools/patch_manager.py b/llvm_tools/patch_manager.py index 3c83fa96..f2d6b322 100755 --- a/llvm_tools/patch_manager.py +++ b/llvm_tools/patch_manager.py @@ -212,8 +212,13 @@ def GetPatchMetadata(patch_dict): """ # Get the metadata values of a patch if possible. - start_version = patch_dict.get('start_version', 0) - end_version = patch_dict.get('end_version', None) + # FIXME(b/221489531): Remove start_version & end_version + if 'version_range' in patch_dict: + start_version = patch_dict['version_range'].get('from', 0) + end_version = patch_dict['version_range'].get('until', None) + else: + start_version = patch_dict.get('start_version', 0) + end_version = patch_dict.get('end_version', None) is_critical = patch_dict.get('is_critical', False) return start_version, end_version, is_critical diff --git a/llvm_tools/patch_manager_unittest.py b/llvm_tools/patch_manager_unittest.py index 62947ed1..69bb683e 100755 --- a/llvm_tools/patch_manager_unittest.py +++ b/llvm_tools/patch_manager_unittest.py @@ -182,7 +182,9 @@ class PatchManagerTest(unittest.TestCase): test_patch = { 'comment': 'Redirects output to stdout', 'rel_patch_path': 'cherry/fixes_stdout.patch', - 'end_version': 1000 + 'version_range': { + 'until': 1000, + } } self.assertEqual( @@ -275,7 +277,9 @@ class PatchManagerTest(unittest.TestCase): patch = [{ 'comment': 'Redirects output to stdout', 'rel_patch_path': 'cherry/fixes_output.patch', - 'start_version': 10 + 'version_range': { + 'from': 10, + }, }] abs_patch_path = '/abs/path/to/filesdir/PATCHES' @@ -293,13 +297,17 @@ class PatchManagerTest(unittest.TestCase): test_updated_patch_metadata = [{ 'comment': 'Redirects output to stdout', 'rel_patch_path': 'cherry/fixes_output.patch', - 'start_version': 10 + 'version_range': { + 'from': 10, + } }] expected_patch_metadata = { 'comment': 'Redirects output to stdout', 'rel_patch_path': 'cherry/fixes_output.patch', - 'start_version': 10 + 'version_range': { + 'from': 10, + } } with CreateTemporaryJsonFile() as json_test_file: @@ -335,7 +343,9 @@ class PatchManagerTest(unittest.TestCase): test_patch_metadata = [{ 'comment': 'Redirects output to stdout', 'rel_patch_path': rel_patch_path, - 'start_version': 10 + 'version_range': { + 'from': 10, + } }] with CreateTemporaryJsonFile() as json_test_file: @@ -379,7 +389,9 @@ class PatchManagerTest(unittest.TestCase): test_patch_metadata = [{ 'comment': 'Redirects output to stdout', 'rel_patch_path': rel_patch_path, - 'start_version': 1000 + 'version_range': { + 'from': 1000, + }, }] with CreateTemporaryJsonFile() as json_test_file: @@ -415,28 +427,36 @@ class PatchManagerTest(unittest.TestCase): test_patch_1 = { 'comment': 'Redirects output to stdout', 'rel_patch_path': 'cherry/fixes_output.patch', - 'start_version': 1000, - 'end_version': 1250 + 'version_range': { + 'from': 1000, + 'until': 1250 + } } test_patch_2 = { 'comment': 'Fixes input', 'rel_patch_path': 'cherry/fixes_input.patch', - 'start_version': 1000 + 'version_range': { + 'from': 1000 + } } test_patch_3 = { 'comment': 'Adds a warning', 'rel_patch_path': 'add_warning.patch', - 'start_version': 750, - 'end_version': 1500 + 'version_range': { + 'from': 750, + 'until': 1500 + } } test_patch_4 = { 'comment': 'Adds a helper function', 'rel_patch_path': 'add_helper.patch', - 'start_version': 20, - 'end_version': 900 + 'version_range': { + 'from': 20, + 'until': 900 + } } test_patch_metadata = [ @@ -520,28 +540,36 @@ class PatchManagerTest(unittest.TestCase): test_patch_1 = { 'comment': 'Redirects output to stdout', 'rel_patch_path': 'cherry/fixes_output.patch', - 'start_version': 1000, - 'end_version': 1190 + 'version_range': { + 'from': 1000, + 'until': 1190 + } } test_patch_2 = { 'comment': 'Fixes input', 'rel_patch_path': 'cherry/fixes_input.patch', - 'start_version': 1000 + 'version_range': { + 'from': 1000 + } } test_patch_3 = { 'comment': 'Adds a warning', 'rel_patch_path': 'add_warning.patch', - 'start_version': 750, - 'end_version': 1500 + 'version_range': { + 'from': 750, + 'until': 1500 + } } test_patch_4 = { 'comment': 'Adds a helper function', 'rel_patch_path': 'add_helper.patch', - 'start_version': 20, - 'end_version': 2000 + 'version_range': { + 'from': 20, + 'until': 2000 + } } test_patch_metadata = [ @@ -654,8 +682,10 @@ class PatchManagerTest(unittest.TestCase): test_patch_1 = { 'comment': 'Redirects output to stdout', 'rel_patch_path': 'cherry/fixes_output.patch', - 'start_version': 1000, - 'end_version': 1190 + 'version_range': { + 'from': 1000, + 'until': 1190 + } } # For the 'remove_patches' mode, this patch is expected to be in the @@ -665,7 +695,9 @@ class PatchManagerTest(unittest.TestCase): test_patch_2 = { 'comment': 'Fixes input', 'rel_patch_path': 'cherry/fixes_input.patch', - 'start_version': 1000 + 'version_range': { + 'from': 1000 + } } # For the 'remove_patches' mode, this patch is expected to be in the @@ -674,8 +706,10 @@ class PatchManagerTest(unittest.TestCase): test_patch_3 = { 'comment': 'Adds a warning', 'rel_patch_path': 'add_warning.patch', - 'start_version': 750, - 'end_version': 1500 + 'version_range': { + 'from': 750, + 'until': 1500 + } } # For the 'remove_patches' mode, this patch is expected to be in the @@ -684,8 +718,10 @@ class PatchManagerTest(unittest.TestCase): test_patch_4 = { 'comment': 'Adds a helper function', 'rel_patch_path': 'add_helper.patch', - 'start_version': 20, - 'end_version': 1400 + 'version_range': { + 'from': 20, + 'until': 1400 + } } test_patch_metadata = [ @@ -786,8 +822,10 @@ class PatchManagerTest(unittest.TestCase): test_patch_1 = { 'comment': 'Redirects output to stdout', 'rel_patch_path': 'cherry/fixes_output.patch', - 'start_version': 1000, - 'end_version': 1190 + 'version_range': { + 'from': 1000, + 'until': 1190 + } } # For the 'remove_patches' mode, this patch is expected to be in the @@ -797,7 +835,9 @@ class PatchManagerTest(unittest.TestCase): test_patch_2 = { 'comment': 'Fixes input', 'rel_patch_path': 'cherry/fixes_input.patch', - 'start_version': 1000 + 'version_range': { + 'from': 1000, + } } # For the 'remove_patches' mode, this patch is expected to be in the @@ -806,8 +846,10 @@ class PatchManagerTest(unittest.TestCase): test_patch_3 = { 'comment': 'Adds a warning', 'rel_patch_path': 'add_warning.patch', - 'start_version': 750, - 'end_version': 1500 + 'version_range': { + 'from': 750, + 'until': 1500 + } } # For the 'remove_patches' mode, this patch is expected to be in the @@ -816,8 +858,10 @@ class PatchManagerTest(unittest.TestCase): test_patch_4 = { 'comment': 'Adds a helper function', 'rel_patch_path': 'add_helper.patch', - 'start_version': 1600, - 'end_version': 2000 + 'version_range': { + 'from': 1600, + 'until': 2000 + } } test_patch_metadata = [ diff --git a/llvm_tools/patch_sync/Cargo.lock b/llvm_tools/patch_sync/Cargo.lock index 62db2220..1ad74a77 100644 --- a/llvm_tools/patch_sync/Cargo.lock +++ b/llvm_tools/patch_sync/Cargo.lock @@ -162,7 +162,7 @@ checksum = "624a8340c38c1b80fd549087862da4ba43e08858af025b236e509b6649fc13d5" [[package]] name = "patch_sync" -version = "0.1.0" +version = "1.1.0" dependencies = [ "anyhow", "rand", diff --git a/llvm_tools/patch_sync/Cargo.toml b/llvm_tools/patch_sync/Cargo.toml index ba8817d5..ed33d5ca 100644 --- a/llvm_tools/patch_sync/Cargo.toml +++ b/llvm_tools/patch_sync/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "patch_sync" -version = "0.1.0" +version = "1.1.0" authors = ["Jordan R Abrahams-Whitehead "] edition = "2018" diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs index b7967c31..c244f1c0 100644 --- a/llvm_tools/patch_sync/src/main.rs +++ b/llvm_tools/patch_sync/src/main.rs @@ -163,13 +163,14 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> { ) })? }; - let new_android_patches = - new_android_patches.filter_patches(|p| match (p.start_version, p.end_version) { + let new_android_patches = new_android_patches.filter_patches(|p| { + match (p.get_start_version(), p.get_end_version()) { (Some(start), Some(end)) => start <= android_llvm_version && android_llvm_version < end, (Some(start), None) => start <= android_llvm_version, (None, Some(end)) => android_llvm_version < end, (None, None) => true, - }); + } + }); if args.verbose { display_patches("New patches from Chromium OS", &new_cros_patches); diff --git a/llvm_tools/patch_sync/src/patch_parsing.rs b/llvm_tools/patch_sync/src/patch_parsing.rs index 581b1899..124f0d6f 100644 --- a/llvm_tools/patch_sync/src/patch_parsing.rs +++ b/llvm_tools/patch_sync/src/patch_parsing.rs @@ -8,14 +8,43 @@ use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; /// JSON serde struct. +// FIXME(b/221489531): Remove when we clear out start_version and +// end_version. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct PatchDictSchema { + /// [deprecated(since = "1.1", note = "Use version_range")] + #[serde(skip_serializing_if = "Option::is_none")] pub end_version: Option, pub metadata: Option>, #[serde(default, skip_serializing_if = "BTreeSet::is_empty")] pub platforms: BTreeSet, pub rel_patch_path: String, + /// [deprecated(since = "1.1", note = "Use version_range")] + #[serde(skip_serializing_if = "Option::is_none")] pub start_version: Option, + pub version_range: Option, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize)] +pub struct VersionRange { + pub from: Option, + pub until: Option, +} + +// FIXME(b/221489531): Remove when we clear out start_version and +// end_version. +impl PatchDictSchema { + pub fn get_start_version(&self) -> Option { + self.version_range + .map(|x| x.from) + .unwrap_or(self.start_version) + } + + pub fn get_end_version(&self) -> Option { + self.version_range + .map(|x| x.until) + .unwrap_or(self.end_version) + } } /// Struct to keep track of patches and their relative paths. @@ -137,6 +166,7 @@ impl PatchCollection { end_version: p.end_version, platforms: new_platforms, metadata: p.metadata.clone(), + version_range: p.version_range, }); // iii. *merged = true; @@ -358,6 +388,10 @@ mod test { rel_patch_path: "a".into(), metadata: None, platforms: BTreeSet::from(["x".into()]), + version_range: Some(VersionRange { + from: Some(0), + until: Some(1), + }), }; let patch2 = PatchDictSchema { rel_patch_path: "b".into(), @@ -402,6 +436,10 @@ mod test { rel_patch_path: "a".into(), metadata: None, platforms: Default::default(), + version_range: Some(VersionRange { + from: Some(0), + until: Some(1), + }), }; let collection1 = PatchCollection { workdir: PathBuf::new(), -- cgit v1.2.3 From d2dffc5458d2eb6caa25db986aa2c24fdd9ddcd5 Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Wed, 2 Mar 2022 12:11:57 -0800 Subject: nightly_revert_checker: add logging Android is reporting strange behavior for this script; logging the SHAs we observe on chrotomation each night will hopefully help us understand this. BUG=None TEST=Unittests Change-Id: Id3a41d9c5f9eae7021863bc994cedeb5469158ae Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3498905 Reviewed-by: Manoj Gupta Commit-Queue: George Burgess Tested-by: George Burgess --- llvm_tools/nightly_revert_checker.py | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/llvm_tools/nightly_revert_checker.py b/llvm_tools/nightly_revert_checker.py index 8e3fcf57..89485088 100755 --- a/llvm_tools/nightly_revert_checker.py +++ b/llvm_tools/nightly_revert_checker.py @@ -24,25 +24,34 @@ import typing as t import cros_utils.email_sender as email_sender import cros_utils.tiny_render as tiny_render + import get_llvm_hash +import get_upstream_patch import git_llvm_rev import revert_checker -import get_upstream_patch State = t.Any -def _find_interesting_android_shas( - android_llvm_toolchain_dir: str) -> t.List[t.Tuple[str, str]]: +def _find_interesting_android_shas(android_llvm_toolchain_dir: str + ) -> t.List[t.Tuple[str, str]]: llvm_project = os.path.join(android_llvm_toolchain_dir, 'toolchain/llvm-project') def get_llvm_merge_base(branch: str) -> str: - return subprocess.check_output( + head_sha = subprocess.check_output( + ['git', 'rev-parse', branch], + cwd=llvm_project, + encoding='utf-8', + ).strip() + merge_base = subprocess.check_output( ['git', 'merge-base', branch, 'aosp/upstream-main'], cwd=llvm_project, encoding='utf-8', ).strip() + logging.info('Merge-base for %s (HEAD == %s) and upstream-main is %s', + branch, head_sha, merge_base) + return merge_base main_legacy = get_llvm_merge_base('aosp/master-legacy') # nocheck testing_upstream = get_llvm_merge_base('aosp/testing-upstream') @@ -51,11 +60,14 @@ def _find_interesting_android_shas( # If these are the same SHA, there's no point in tracking both. if main_legacy != testing_upstream: result.append(('testing-upstream', testing_upstream)) + else: + logging.info('main-legacy and testing-upstream are identical; ignoring ' + 'the latter.') return result -def _parse_llvm_ebuild_for_shas( - ebuild_file: io.TextIOWrapper) -> t.List[t.Tuple[str, str]]: +def _parse_llvm_ebuild_for_shas(ebuild_file: io.TextIOWrapper + ) -> t.List[t.Tuple[str, str]]: def parse_ebuild_assignment(line: str) -> str: no_comments = line.split('#')[0] no_assign = no_comments.split('=', 1)[1].strip() @@ -82,8 +94,8 @@ def _parse_llvm_ebuild_for_shas( return results -def _find_interesting_chromeos_shas( - chromeos_base: str) -> t.List[t.Tuple[str, str]]: +def _find_interesting_chromeos_shas(chromeos_base: str + ) -> t.List[t.Tuple[str, str]]: llvm_dir = os.path.join(chromeos_base, 'src/third_party/chromiumos-overlay/sys-devel/llvm') candidate_ebuilds = [ @@ -327,9 +339,8 @@ def parse_args(argv: t.List[str]) -> t.Any: return parser.parse_args(argv) -def find_chroot( - opts: t.Any, reviewers: t.List[str], cc: t.List[str] -) -> t.Tuple[str, t.List[t.Tuple[str, str]], _EmailRecipients]: +def find_chroot(opts: t.Any, reviewers: t.List[str], cc: t.List[str] + ) -> t.Tuple[str, t.List[t.Tuple[str, str]], _EmailRecipients]: recipients = reviewers + cc if opts.repository == 'chromeos': chroot_path = opts.chromeos_dir -- cgit v1.2.3