diff --git a/tools/aconfig/src/cache.rs b/tools/aconfig/src/cache.rs index c546f7b84e..30810fa344 100644 --- a/tools/aconfig/src/cache.rs +++ b/tools/aconfig/src/cache.rs @@ -51,72 +51,42 @@ pub struct Cache { items: Vec, } -impl Cache { - pub fn new(namespace: String) -> Result { - ensure!(!namespace.is_empty(), "empty namespace"); - Ok(Cache { namespace, items: vec![] }) +// TODO: replace this function with Iterator.is_sorted_by_key(...)) when that API becomes stable +fn iter_is_sorted_by_key<'a, T: 'a, F, K>(iter: impl Iterator, f: F) -> bool +where + F: FnMut(&'a T) -> K, + K: PartialOrd, +{ + let mut last: Option = None; + for current in iter.map(f) { + if let Some(l) = last { + if l > current { + return false; + } + } + last = Some(current); } + true +} +impl Cache { pub fn read_from_reader(reader: impl Read) -> Result { - serde_json::from_reader(reader).map_err(|e| e.into()) + let cache: Cache = serde_json::from_reader(reader)?; + ensure!( + iter_is_sorted_by_key(cache.iter(), |item| &item.name), + "internal error: flags in cache file not sorted" + ); + Ok(cache) } pub fn write_to_writer(&self, writer: impl Write) -> Result<()> { + ensure!( + iter_is_sorted_by_key(self.iter(), |item| &item.name), + "internal error: flags in cache file not sorted" + ); serde_json::to_writer(writer, self).map_err(|e| e.into()) } - pub fn add_flag_declaration( - &mut self, - source: Source, - declaration: FlagDeclaration, - ) -> Result<()> { - ensure!(!declaration.name.is_empty(), "empty flag name"); - ensure!(!declaration.description.is_empty(), "empty flag description"); - ensure!( - self.items.iter().all(|item| item.name != declaration.name), - "failed to declare flag {} from {}: flag already declared", - declaration.name, - source - ); - self.items.push(Item { - namespace: self.namespace.clone(), - name: declaration.name.clone(), - description: declaration.description, - state: DEFAULT_FLAG_STATE, - permission: DEFAULT_FLAG_PERMISSION, - trace: vec![Tracepoint { - source, - state: DEFAULT_FLAG_STATE, - permission: DEFAULT_FLAG_PERMISSION, - }], - }); - Ok(()) - } - - pub fn add_flag_value(&mut self, source: Source, value: FlagValue) -> Result<()> { - ensure!(!value.namespace.is_empty(), "empty flag namespace"); - ensure!(!value.name.is_empty(), "empty flag name"); - ensure!( - value.namespace == self.namespace, - "failed to set values for flag {}/{} from {}: expected namespace {}", - value.namespace, - value.name, - source, - self.namespace - ); - let Some(existing_item) = self.items.iter_mut().find(|item| item.name == value.name) else { - bail!("failed to set values for flag {}/{} from {}: flag not declared", value.namespace, value.name, source); - }; - existing_item.state = value.state; - existing_item.permission = value.permission; - existing_item.trace.push(Tracepoint { - source, - state: value.state, - permission: value.permission, - }); - Ok(()) - } - pub fn iter(&self) -> impl Iterator { self.items.iter() } @@ -131,6 +101,80 @@ impl Cache { } } +#[derive(Debug)] +pub struct CacheBuilder { + cache: Cache, +} + +impl CacheBuilder { + pub fn new(namespace: String) -> Result { + ensure!(!namespace.is_empty(), "empty namespace"); + let cache = Cache { namespace, items: vec![] }; + Ok(CacheBuilder { cache }) + } + + pub fn add_flag_declaration( + &mut self, + source: Source, + declaration: FlagDeclaration, + ) -> Result<&mut CacheBuilder> { + ensure!(!declaration.name.is_empty(), "empty flag name"); + ensure!(!declaration.description.is_empty(), "empty flag description"); + ensure!( + self.cache.items.iter().all(|item| item.name != declaration.name), + "failed to declare flag {} from {}: flag already declared", + declaration.name, + source + ); + self.cache.items.push(Item { + namespace: self.cache.namespace.clone(), + name: declaration.name.clone(), + description: declaration.description, + state: DEFAULT_FLAG_STATE, + permission: DEFAULT_FLAG_PERMISSION, + trace: vec![Tracepoint { + source, + state: DEFAULT_FLAG_STATE, + permission: DEFAULT_FLAG_PERMISSION, + }], + }); + Ok(self) + } + + pub fn add_flag_value( + &mut self, + source: Source, + value: FlagValue, + ) -> Result<&mut CacheBuilder> { + ensure!(!value.namespace.is_empty(), "empty flag namespace"); + ensure!(!value.name.is_empty(), "empty flag name"); + ensure!( + value.namespace == self.cache.namespace, + "failed to set values for flag {}/{} from {}: expected namespace {}", + value.namespace, + value.name, + source, + self.cache.namespace + ); + let Some(existing_item) = self.cache.items.iter_mut().find(|item| item.name == value.name) else { + bail!("failed to set values for flag {}/{} from {}: flag not declared", value.namespace, value.name, source); + }; + existing_item.state = value.state; + existing_item.permission = value.permission; + existing_item.trace.push(Tracepoint { + source, + state: value.state, + permission: value.permission, + }); + Ok(self) + } + + pub fn build(mut self) -> Cache { + self.cache.items.sort_by_cached_key(|item| item.name.clone()); + self.cache + } +} + #[cfg(test)] mod tests { use super::*; @@ -138,14 +182,14 @@ mod tests { #[test] fn test_add_flag_declaration() { - let mut cache = Cache::new("ns".to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new("ns".to_string()).unwrap(); + builder .add_flag_declaration( Source::File("first.txt".to_string()), FlagDeclaration { name: "foo".to_string(), description: "desc".to_string() }, ) .unwrap(); - let error = cache + let error = builder .add_flag_declaration( Source::File("second.txt".to_string()), FlagDeclaration { name: "foo".to_string(), description: "desc".to_string() }, @@ -155,17 +199,26 @@ mod tests { &format!("{:?}", error), "failed to declare flag foo from second.txt: flag already declared" ); + builder + .add_flag_declaration( + Source::File("first.txt".to_string()), + FlagDeclaration { name: "bar".to_string(), description: "desc".to_string() }, + ) + .unwrap(); + + let cache = builder.build(); + + // check flags are sorted by name + assert_eq!( + cache.into_iter().map(|item| item.name).collect::>(), + vec!["bar".to_string(), "foo".to_string()] + ); } #[test] fn test_add_flag_value() { - fn check(cache: &Cache, name: &str, expected: (FlagState, Permission)) -> bool { - let item = cache.iter().find(|&item| item.name == name).unwrap(); - item.state == expected.0 && item.permission == expected.1 - } - - let mut cache = Cache::new("ns".to_string()).unwrap(); - let error = cache + let mut builder = CacheBuilder::new("ns".to_string()).unwrap(); + let error = builder .add_flag_value( Source::Memory, FlagValue { @@ -181,15 +234,14 @@ mod tests { "failed to set values for flag ns/foo from : flag not declared" ); - cache + builder .add_flag_declaration( Source::File("first.txt".to_string()), FlagDeclaration { name: "foo".to_string(), description: "desc".to_string() }, ) .unwrap(); - assert!(check(&cache, "foo", (DEFAULT_FLAG_STATE, DEFAULT_FLAG_PERMISSION))); - cache + builder .add_flag_value( Source::Memory, FlagValue { @@ -200,9 +252,8 @@ mod tests { }, ) .unwrap(); - assert!(check(&cache, "foo", (FlagState::Disabled, Permission::ReadOnly))); - cache + builder .add_flag_value( Source::Memory, FlagValue { @@ -213,10 +264,9 @@ mod tests { }, ) .unwrap(); - assert!(check(&cache, "foo", (FlagState::Enabled, Permission::ReadWrite))); // different namespace -> no-op - let error = cache + let error = builder .add_flag_value( Source::Memory, FlagValue { @@ -228,19 +278,23 @@ mod tests { ) .unwrap_err(); assert_eq!(&format!("{:?}", error), "failed to set values for flag some-other-namespace/foo from : expected namespace ns"); - assert!(check(&cache, "foo", (FlagState::Enabled, Permission::ReadWrite))); + + let cache = builder.build(); + let item = cache.iter().find(|&item| item.name == "foo").unwrap(); + assert_eq!(FlagState::Enabled, item.state); + assert_eq!(Permission::ReadWrite, item.permission); } #[test] fn test_reject_empty_cache_namespace() { - Cache::new("".to_string()).unwrap_err(); + CacheBuilder::new("".to_string()).unwrap_err(); } #[test] fn test_reject_empty_flag_declaration_fields() { - let mut cache = Cache::new("ns".to_string()).unwrap(); + let mut builder = CacheBuilder::new("ns".to_string()).unwrap(); - let error = cache + let error = builder .add_flag_declaration( Source::Memory, FlagDeclaration { name: "".to_string(), description: "Description".to_string() }, @@ -248,7 +302,7 @@ mod tests { .unwrap_err(); assert_eq!(&format!("{:?}", error), "empty flag name"); - let error = cache + let error = builder .add_flag_declaration( Source::Memory, FlagDeclaration { name: "foo".to_string(), description: "".to_string() }, @@ -259,15 +313,15 @@ mod tests { #[test] fn test_reject_empty_flag_value_files() { - let mut cache = Cache::new("ns".to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new("ns".to_string()).unwrap(); + builder .add_flag_declaration( Source::Memory, FlagDeclaration { name: "foo".to_string(), description: "desc".to_string() }, ) .unwrap(); - let error = cache + let error = builder .add_flag_value( Source::Memory, FlagValue { @@ -280,7 +334,7 @@ mod tests { .unwrap_err(); assert_eq!(&format!("{:?}", error), "empty flag namespace"); - let error = cache + let error = builder .add_flag_value( Source::Memory, FlagValue { @@ -293,4 +347,11 @@ mod tests { .unwrap_err(); assert_eq!(&format!("{:?}", error), "empty flag name"); } + + #[test] + fn test_iter_is_sorted_by_key() { + assert!(iter_is_sorted_by_key(["a", "b", "c"].iter(), |s| s)); + assert!(iter_is_sorted_by_key(Vec::<&str>::new().iter(), |s| s)); + assert!(!iter_is_sorted_by_key(["a", "c", "b"].iter(), |s| s)); + } } diff --git a/tools/aconfig/src/codegen_cpp.rs b/tools/aconfig/src/codegen_cpp.rs index cb266f1657..2aeea6a7c2 100644 --- a/tools/aconfig/src/codegen_cpp.rs +++ b/tools/aconfig/src/codegen_cpp.rs @@ -64,13 +64,14 @@ fn create_class_element(item: &Item) -> ClassElement { mod tests { use super::*; use crate::aconfig::{FlagDeclaration, FlagState, FlagValue, Permission}; + use crate::cache::CacheBuilder; use crate::commands::Source; #[test] fn test_cpp_codegen_build_time_flag_only() { let namespace = "my_namespace"; - let mut cache = Cache::new(namespace.to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new(namespace.to_string()).unwrap(); + builder .add_flag_declaration( Source::File("aconfig_one.txt".to_string()), FlagDeclaration { @@ -78,8 +79,7 @@ mod tests { description: "buildtime disable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_value( Source::Memory, FlagValue { @@ -89,8 +89,7 @@ mod tests { permission: Permission::ReadOnly, }, ) - .unwrap(); - cache + .unwrap() .add_flag_declaration( Source::File("aconfig_two.txt".to_string()), FlagDeclaration { @@ -98,8 +97,7 @@ mod tests { description: "buildtime enable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_value( Source::Memory, FlagValue { @@ -110,6 +108,7 @@ mod tests { }, ) .unwrap(); + let cache = builder.build(); let expect_content = r#"#ifndef my_namespace_HEADER_H #define my_namespace_HEADER_H #include "my_namespace.h" @@ -144,8 +143,8 @@ mod tests { #[test] fn test_cpp_codegen_runtime_flag() { let namespace = "my_namespace"; - let mut cache = Cache::new(namespace.to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new(namespace.to_string()).unwrap(); + builder .add_flag_declaration( Source::File("aconfig_one.txt".to_string()), FlagDeclaration { @@ -153,8 +152,7 @@ mod tests { description: "buildtime disable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_declaration( Source::File("aconfig_two.txt".to_string()), FlagDeclaration { @@ -162,8 +160,7 @@ mod tests { description: "runtime enable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_value( Source::Memory, FlagValue { @@ -174,6 +171,7 @@ mod tests { }, ) .unwrap(); + let cache = builder.build(); let expect_content = r#"#ifndef my_namespace_HEADER_H #define my_namespace_HEADER_H #include "my_namespace.h" diff --git a/tools/aconfig/src/codegen_java.rs b/tools/aconfig/src/codegen_java.rs index 476a89ddcc..733b1c5d1f 100644 --- a/tools/aconfig/src/codegen_java.rs +++ b/tools/aconfig/src/codegen_java.rs @@ -71,13 +71,14 @@ fn create_class_element(item: &Item) -> ClassElement { mod tests { use super::*; use crate::aconfig::{FlagDeclaration, FlagValue}; + use crate::cache::CacheBuilder; use crate::commands::Source; #[test] fn test_generate_java_code() { let namespace = "com.example"; - let mut cache = Cache::new(namespace.to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new(namespace.to_string()).unwrap(); + builder .add_flag_declaration( Source::File("test.txt".to_string()), FlagDeclaration { @@ -85,8 +86,7 @@ mod tests { description: "buildtime enable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_declaration( Source::File("test2.txt".to_string()), FlagDeclaration { @@ -94,8 +94,7 @@ mod tests { description: "runtime disable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_value( Source::Memory, FlagValue { @@ -106,6 +105,7 @@ mod tests { }, ) .unwrap(); + let cache = builder.build(); let expect_content = r#"package com.example; import android.provider.DeviceConfig; diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index 0bdb0b5f57..22de33175a 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -23,7 +23,7 @@ use std::io::Read; use std::path::PathBuf; use crate::aconfig::{FlagDeclarations, FlagValue}; -use crate::cache::Cache; +use crate::cache::{Cache, CacheBuilder}; use crate::codegen_cpp::generate_cpp_code; use crate::codegen_java::generate_java_code; use crate::protos::ProtoParsedFlags; @@ -59,7 +59,7 @@ pub fn create_cache( declarations: Vec, values: Vec, ) -> Result { - let mut cache = Cache::new(namespace.to_owned())?; + let mut builder = CacheBuilder::new(namespace.to_owned())?; for mut input in declarations { let mut contents = String::new(); @@ -74,7 +74,7 @@ pub fn create_cache( dec_list.namespace ); for d in dec_list.flags.into_iter() { - cache.add_flag_declaration(input.source.clone(), d)?; + builder.add_flag_declaration(input.source.clone(), d)?; } } @@ -85,11 +85,11 @@ pub fn create_cache( .with_context(|| format!("Failed to parse {}", input.source))?; for v in values_list { // TODO: warn about flag values that do not take effect? - let _ = cache.add_flag_value(input.source.clone(), v); + let _ = builder.add_flag_value(input.source.clone(), v); } } - Ok(cache) + Ok(builder.build()) } pub fn create_java_lib(cache: &Cache) -> Result { @@ -107,29 +107,35 @@ pub enum DumpFormat { Protobuf, } -pub fn dump_cache(cache: Cache, format: DumpFormat) -> Result> { - match format { - DumpFormat::Text => { - let mut lines = vec![]; - for item in cache.iter() { - lines.push(format!("{}: {:?}\n", item.name, item.state)); +pub fn dump_cache(mut caches: Vec, format: DumpFormat) -> Result> { + let mut output = Vec::new(); + caches.sort_by_cached_key(|cache| cache.namespace().to_string()); + for cache in caches.into_iter() { + match format { + DumpFormat::Text => { + let mut lines = vec![]; + for item in cache.iter() { + lines.push(format!( + "{}/{}: {:?} {:?}\n", + item.namespace, item.name, item.state, item.permission + )); + } + output.append(&mut lines.concat().into()); } - Ok(lines.concat().into()) - } - DumpFormat::Debug => { - let mut lines = vec![]; - for item in cache.iter() { - lines.push(format!("{:?}\n", item)); + DumpFormat::Debug => { + let mut lines = vec![]; + for item in cache.iter() { + lines.push(format!("{:?}\n", item)); + } + output.append(&mut lines.concat().into()); + } + DumpFormat::Protobuf => { + let parsed_flags: ProtoParsedFlags = cache.into(); + parsed_flags.write_to_vec(&mut output)?; } - Ok(lines.concat().into()) - } - DumpFormat::Protobuf => { - let parsed_flags: ProtoParsedFlags = cache.into(); - let mut output = vec![]; - parsed_flags.write_to_vec(&mut output)?; - Ok(output) } } + Ok(output) } #[cfg(test)] @@ -137,9 +143,9 @@ mod tests { use super::*; use crate::aconfig::{FlagState, Permission}; - fn create_test_cache() -> Cache { + fn create_test_cache_ns1() -> Cache { let s = r#" - namespace: "ns" + namespace: "ns1" flag { name: "a" description: "Description of a" @@ -152,28 +158,49 @@ mod tests { let declarations = vec![Input { source: Source::Memory, reader: Box::new(s.as_bytes()) }]; let o = r#" flag_value { - namespace: "ns" + namespace: "ns1" name: "a" state: DISABLED permission: READ_ONLY } "#; let values = vec![Input { source: Source::Memory, reader: Box::new(o.as_bytes()) }]; - create_cache("ns", declarations, values).unwrap() + create_cache("ns1", declarations, values).unwrap() + } + + fn create_test_cache_ns2() -> Cache { + let s = r#" + namespace: "ns2" + flag { + name: "c" + description: "Description of c" + } + "#; + let declarations = vec![Input { source: Source::Memory, reader: Box::new(s.as_bytes()) }]; + let o = r#" + flag_value { + namespace: "ns2" + name: "c" + state: DISABLED + permission: READ_ONLY + } + "#; + let values = vec![Input { source: Source::Memory, reader: Box::new(o.as_bytes()) }]; + create_cache("ns2", declarations, values).unwrap() } #[test] fn test_create_cache() { - let cache = create_test_cache(); // calls create_cache - let item = cache.iter().find(|&item| item.name == "a").unwrap(); + let caches = create_test_cache_ns1(); // calls create_cache + let item = caches.iter().find(|&item| item.name == "a").unwrap(); assert_eq!(FlagState::Disabled, item.state); assert_eq!(Permission::ReadOnly, item.permission); } #[test] fn test_dump_text_format() { - let cache = create_test_cache(); - let bytes = dump_cache(cache, DumpFormat::Text).unwrap(); + let caches = vec![create_test_cache_ns1()]; + let bytes = dump_cache(caches, DumpFormat::Text).unwrap(); let text = std::str::from_utf8(&bytes).unwrap(); assert!(text.contains("a: Disabled")); } @@ -183,8 +210,8 @@ mod tests { use crate::protos::{ProtoFlagPermission, ProtoFlagState, ProtoTracepoint}; use protobuf::Message; - let cache = create_test_cache(); - let bytes = dump_cache(cache, DumpFormat::Protobuf).unwrap(); + let caches = vec![create_test_cache_ns1()]; + let bytes = dump_cache(caches, DumpFormat::Protobuf).unwrap(); let actual = ProtoParsedFlags::parse_from_bytes(&bytes).unwrap(); assert_eq!( @@ -194,7 +221,7 @@ mod tests { let item = actual.parsed_flag.iter().find(|item| item.name == Some("b".to_string())).unwrap(); - assert_eq!(item.namespace(), "ns"); + assert_eq!(item.namespace(), "ns1"); assert_eq!(item.name(), "b"); assert_eq!(item.description(), "Description of b"); assert_eq!(item.state(), ProtoFlagState::DISABLED); @@ -205,4 +232,23 @@ mod tests { tp.set_permission(ProtoFlagPermission::READ_WRITE); assert_eq!(item.trace, vec![tp]); } + + #[test] + fn test_dump_multiple_caches() { + let caches = vec![create_test_cache_ns1(), create_test_cache_ns2()]; + let bytes = dump_cache(caches, DumpFormat::Protobuf).unwrap(); + let dump = ProtoParsedFlags::parse_from_bytes(&bytes).unwrap(); + assert_eq!( + dump.parsed_flag + .iter() + .map(|parsed_flag| format!("{}/{}", parsed_flag.namespace(), parsed_flag.name())) + .collect::>(), + vec!["ns1/a".to_string(), "ns1/b".to_string(), "ns2/c".to_string()] + ); + + let caches = vec![create_test_cache_ns2(), create_test_cache_ns1()]; + let bytes = dump_cache(caches, DumpFormat::Protobuf).unwrap(); + let dump_reversed_input = ProtoParsedFlags::parse_from_bytes(&bytes).unwrap(); + assert_eq!(dump, dump_reversed_input); + } } diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs index 6db5948013..d02307d702 100644 --- a/tools/aconfig/src/main.rs +++ b/tools/aconfig/src/main.rs @@ -56,7 +56,7 @@ fn cli() -> Command { ) .subcommand( Command::new("dump") - .arg(Arg::new("cache").long("cache").required(true)) + .arg(Arg::new("cache").long("cache").action(ArgAction::Append).required(true)) .arg( Arg::new("format") .long("format") @@ -130,11 +130,14 @@ fn main() -> Result<()> { write_output_file_realtive_to_dir(&dir, &generated_file)?; } Some(("dump", sub_matches)) => { - let path = get_required_arg::(sub_matches, "cache")?; - let file = fs::File::open(path)?; - let cache = Cache::read_from_reader(file)?; + let mut caches = Vec::new(); + for path in sub_matches.get_many::("cache").unwrap_or_default() { + let file = fs::File::open(path)?; + let cache = Cache::read_from_reader(file)?; + caches.push(cache); + } let format = get_required_arg::(sub_matches, "format")?; - let output = commands::dump_cache(cache, *format)?; + let output = commands::dump_cache(caches, *format)?; let path = get_required_arg::(sub_matches, "out")?; let mut file: Box = if *path == "-" { Box::new(io::stdout())