From 6befb342fd9d1442ed95100f0d61c3a800cd6710 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Tue, 28 Nov 2023 15:55:07 -0800 Subject: [PATCH] Support aconfig dump --dedup Add a flag to aconfig dump that will allow identical flags to be merged without erroring. This will allow merging the aconfig cache files from dependencies for each module, which requires passing less data to Make from Soong, and thus reduces the percentage of builds require Kati analysis. Bug: 313698230 Test: aconfig.test Change-Id: Id2d9b78225720ce01defad4b13ea9395f6ab1033 --- tools/aconfig/src/commands.rs | 19 ++++++++--- tools/aconfig/src/main.rs | 6 ++-- tools/aconfig/src/protos.rs | 61 +++++++++++++++++++++++++++++------ 3 files changed, 69 insertions(+), 17 deletions(-) diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index ff0df1f56d..ed7489f73c 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -250,11 +250,11 @@ pub enum DumpFormat { Textproto, } -pub fn dump_parsed_flags(mut input: Vec, format: DumpFormat) -> Result> { +pub fn dump_parsed_flags(mut input: Vec, format: DumpFormat, dedup: bool) -> Result> { let individually_parsed_flags: Result> = input.iter_mut().map(|i| i.try_parse_flags()).collect(); let parsed_flags: ProtoParsedFlags = - crate::protos::parsed_flags::merge(individually_parsed_flags?)?; + crate::protos::parsed_flags::merge(individually_parsed_flags?, dedup)?; let mut output = Vec::new(); match format { @@ -448,7 +448,7 @@ mod tests { #[test] fn test_dump_text_format() { let input = parse_test_flags_as_input(); - let bytes = dump_parsed_flags(vec![input], DumpFormat::Text).unwrap(); + let bytes = dump_parsed_flags(vec![input], DumpFormat::Text, false).unwrap(); let text = std::str::from_utf8(&bytes).unwrap(); assert!(text.contains("com.android.aconfig.test.disabled_ro: READ_ONLY + DISABLED")); } @@ -463,7 +463,7 @@ mod tests { .unwrap(); let input = parse_test_flags_as_input(); - let actual = dump_parsed_flags(vec![input], DumpFormat::Protobuf).unwrap(); + let actual = dump_parsed_flags(vec![input], DumpFormat::Protobuf, false).unwrap(); assert_eq!(expected, actual); } @@ -471,7 +471,16 @@ mod tests { #[test] fn test_dump_textproto_format() { let input = parse_test_flags_as_input(); - let bytes = dump_parsed_flags(vec![input], DumpFormat::Textproto).unwrap(); + let bytes = dump_parsed_flags(vec![input], DumpFormat::Textproto, false).unwrap(); + let text = std::str::from_utf8(&bytes).unwrap(); + assert_eq!(crate::test::TEST_FLAGS_TEXTPROTO.trim(), text.trim()); + } + + #[test] + fn test_dump_textproto_format_dedup() { + let input = parse_test_flags_as_input(); + let input2 = parse_test_flags_as_input(); + let bytes = dump_parsed_flags(vec![input, input2], DumpFormat::Textproto, true).unwrap(); let text = std::str::from_utf8(&bytes).unwrap(); assert_eq!(crate::test::TEST_FLAGS_TEXTPROTO.trim(), text.trim()); } diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs index 7e44baf3c9..17c388ddab 100644 --- a/tools/aconfig/src/main.rs +++ b/tools/aconfig/src/main.rs @@ -99,13 +99,14 @@ fn cli() -> Command { ) .subcommand( Command::new("dump") - .arg(Arg::new("cache").long("cache").action(ArgAction::Append).required(true)) + .arg(Arg::new("cache").long("cache").action(ArgAction::Append)) .arg( Arg::new("format") .long("format") .value_parser(EnumValueParser::::new()) .default_value("text"), ) + .arg(Arg::new("dedup").long("dedup").num_args(0).action(ArgAction::SetTrue)) .arg(Arg::new("out").long("out").default_value("-")), ) } @@ -222,7 +223,8 @@ fn main() -> Result<()> { let input = open_zero_or_more_files(sub_matches, "cache")?; let format = get_required_arg::(sub_matches, "format") .context("failed to dump previously parsed flags")?; - let output = commands::dump_parsed_flags(input, *format)?; + let dedup = get_required_arg::(sub_matches, "dedup")?; + let output = commands::dump_parsed_flags(input, *format, *dedup)?; let path = get_required_arg::(sub_matches, "out")?; write_output_to_file_or_stdout(path, &output)?; } diff --git a/tools/aconfig/src/protos.rs b/tools/aconfig/src/protos.rs index a5a53425cf..8e764680ca 100644 --- a/tools/aconfig/src/protos.rs +++ b/tools/aconfig/src/protos.rs @@ -274,12 +274,18 @@ pub mod parsed_flags { Ok(()) } - pub fn merge(parsed_flags: Vec) -> Result { + pub fn merge(parsed_flags: Vec, dedup: bool) -> Result { let mut merged = ProtoParsedFlags::new(); for mut pfs in parsed_flags.into_iter() { merged.parsed_flag.append(&mut pfs.parsed_flag); } merged.parsed_flag.sort_by_cached_key(create_sorting_key); + if dedup { + // Deduplicate identical protobuf messages. Messages with the same sorting key but + // different fields (including the path to the original source file) will not be + // deduplicated and trigger an error in verify_fields. + merged.parsed_flag.dedup(); + } verify_fields(&merged)?; Ok(merged) } @@ -840,14 +846,49 @@ parsed_flag { "#; let second = try_from_binary_proto_from_text_proto(text_proto).unwrap(); - // bad cases - let error = parsed_flags::merge(vec![first.clone(), first.clone()]).unwrap_err(); - assert_eq!(format!("{:?}", error), "bad parsed flags: duplicate flag com.first.first (defined in flags.declarations and flags.declarations)"); - - // valid cases - assert!(parsed_flags::merge(vec![]).unwrap().parsed_flag.is_empty()); - assert_eq!(first, parsed_flags::merge(vec![first.clone()]).unwrap()); - assert_eq!(expected, parsed_flags::merge(vec![first.clone(), second.clone()]).unwrap()); - assert_eq!(expected, parsed_flags::merge(vec![second, first]).unwrap()); + let text_proto = r#" +parsed_flag { + package: "com.second" + name: "second" + namespace: "second_ns" + bug: "b" + description: "This is the description of the second flag." + state: ENABLED + permission: READ_WRITE + trace { + source: "duplicate/flags.declarations" + state: DISABLED + permission: READ_ONLY + } +} +"#; + let second_duplicate = try_from_binary_proto_from_text_proto(text_proto).unwrap(); + + // bad cases + + // two of the same flag with dedup disabled + let error = parsed_flags::merge(vec![first.clone(), first.clone()], false).unwrap_err(); + assert_eq!(format!("{:?}", error), "bad parsed flags: duplicate flag com.first.first (defined in flags.declarations and flags.declarations)"); + + // two conflicting flags with dedup disabled + let error = parsed_flags::merge(vec![second.clone(), second_duplicate.clone()], false).unwrap_err(); + assert_eq!(format!("{:?}", error), "bad parsed flags: duplicate flag com.second.second (defined in flags.declarations and duplicate/flags.declarations)"); + + // two conflicting flags with dedup enabled + let error = parsed_flags::merge(vec![second.clone(), second_duplicate.clone()], true).unwrap_err(); + assert_eq!(format!("{:?}", error), "bad parsed flags: duplicate flag com.second.second (defined in flags.declarations and duplicate/flags.declarations)"); + + // valid cases + assert!(parsed_flags::merge(vec![], false).unwrap().parsed_flag.is_empty()); + assert!(parsed_flags::merge(vec![], true).unwrap().parsed_flag.is_empty()); + assert_eq!(first, parsed_flags::merge(vec![first.clone()], false).unwrap()); + assert_eq!(first, parsed_flags::merge(vec![first.clone()], true).unwrap()); + assert_eq!(expected, parsed_flags::merge(vec![first.clone(), second.clone()], false).unwrap()); + assert_eq!(expected, parsed_flags::merge(vec![first.clone(), second.clone()], true).unwrap()); + assert_eq!(expected, parsed_flags::merge(vec![second.clone(), first.clone()], false).unwrap()); + assert_eq!(expected, parsed_flags::merge(vec![second.clone(), first.clone()], true).unwrap()); + + // two identical flags with dedup enabled + assert_eq!(first, parsed_flags::merge(vec![first.clone(), first.clone()], true).unwrap()); } }