From b27f2ce4364d23248c75e93f349a8596b20ee275 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Kongstad?= Date: Fri, 2 Jun 2023 11:43:21 +0200 Subject: [PATCH 1/5] aconfig: give commands ownership of all arguments Pass the Cache argument to command::create__lib functions by value instead of by reference, to align with other commands. The intended ownership flow is as follows: - main creates objects based on command line arguments - main hands commands ownership of the objects - command processes the objects - command gives main ownership of any generated output - main writes the output to file Rationale: commands.rs is a unit testable version of main, and to the rest of aconfig, acts as the top level entry point; main.rs exists only to parse command line arguments and perform I/O. Bug: 283910447 Test: atest aconfig.test Change-Id: I1e1dea7da8ecc2bb6e2f7ee4a6df64562c148959 --- tools/aconfig/src/commands.rs | 12 ++++++------ tools/aconfig/src/main.rs | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index cce1d7f43c..df1a17a9f6 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -93,16 +93,16 @@ pub fn create_cache( Ok(builder.build()) } -pub fn create_java_lib(cache: &Cache) -> Result { - generate_java_code(cache) +pub fn create_java_lib(cache: Cache) -> Result { + generate_java_code(&cache) } -pub fn create_cpp_lib(cache: &Cache) -> Result { - generate_cpp_code(cache) +pub fn create_cpp_lib(cache: Cache) -> Result { + generate_cpp_code(&cache) } -pub fn create_rust_lib(cache: &Cache) -> Result { - generate_rust_code(cache) +pub fn create_rust_lib(cache: Cache) -> Result { + generate_rust_code(&cache) } #[derive(Copy, Clone, Debug, PartialEq, Eq, ValueEnum)] diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs index 1d2ec95fec..c07bcf3793 100644 --- a/tools/aconfig/src/main.rs +++ b/tools/aconfig/src/main.rs @@ -125,7 +125,7 @@ fn main() -> Result<()> { let file = fs::File::open(path)?; let cache = Cache::read_from_reader(file)?; let dir = PathBuf::from(get_required_arg::(sub_matches, "out")?); - let generated_file = commands::create_java_lib(&cache)?; + let generated_file = commands::create_java_lib(cache)?; write_output_file_realtive_to_dir(&dir, &generated_file)?; } Some(("create-cpp-lib", sub_matches)) => { @@ -133,7 +133,7 @@ fn main() -> Result<()> { let file = fs::File::open(path)?; let cache = Cache::read_from_reader(file)?; let dir = PathBuf::from(get_required_arg::(sub_matches, "out")?); - let generated_file = commands::create_cpp_lib(&cache)?; + let generated_file = commands::create_cpp_lib(cache)?; write_output_file_realtive_to_dir(&dir, &generated_file)?; } Some(("create-rust-lib", sub_matches)) => { @@ -141,7 +141,7 @@ fn main() -> Result<()> { let file = fs::File::open(path)?; let cache = Cache::read_from_reader(file)?; let dir = PathBuf::from(get_required_arg::(sub_matches, "out")?); - let generated_file = commands::create_rust_lib(&cache)?; + let generated_file = commands::create_rust_lib(cache)?; write_output_file_realtive_to_dir(&dir, &generated_file)?; } Some(("dump", sub_matches)) => { From 1cd166cd313bc521a9849e2a76ca76d30e714ea1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Kongstad?= Date: Fri, 2 Jun 2023 11:27:30 +0200 Subject: [PATCH 2/5] aconfig: cache.rs: remove unnecessary use statements Remove unnecessary use from the cache::test module: they already covered by `use super:*;`. Bug: 283910447 Test: atest aconfig.test Change-Id: I9e03385629f38180c0f92080c7f097a8e0d9ef69 --- tools/aconfig/src/cache.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/aconfig/src/cache.rs b/tools/aconfig/src/cache.rs index 44ad3dd005..972ba41d5d 100644 --- a/tools/aconfig/src/cache.rs +++ b/tools/aconfig/src/cache.rs @@ -179,7 +179,6 @@ impl CacheBuilder { #[cfg(test)] mod tests { use super::*; - use crate::aconfig::{FlagState, Permission}; #[test] fn test_add_flag_declaration() { From 83a8760bbc62940f6b5913e0e4168ced2fca5869 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Kongstad?= Date: Fri, 2 Jun 2023 11:20:15 +0200 Subject: [PATCH 3/5] aconfig: add test utilities Create a test utility function to create a Cache from the files in testdata/*. A follow-up CL will update the unit tests to use this instead of creating their own caches. Bug: 283910447 Test: atest aconfig.test Change-Id: Ice5064eadb0babde5eb38d292330d213ab136d96 --- tools/aconfig/src/codegen_rust.rs | 20 +------------- tools/aconfig/src/main.rs | 3 +++ tools/aconfig/src/test.rs | 45 +++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 19 deletions(-) create mode 100644 tools/aconfig/src/test.rs diff --git a/tools/aconfig/src/codegen_rust.rs b/tools/aconfig/src/codegen_rust.rs index b3a6f53b73..6d60323953 100644 --- a/tools/aconfig/src/codegen_rust.rs +++ b/tools/aconfig/src/codegen_rust.rs @@ -79,28 +79,10 @@ fn create_template_parsed_flag(namespace: &str, item: &Item) -> TemplateParsedFl #[cfg(test)] mod tests { use super::*; - use crate::commands::{create_cache, Input, Source}; #[test] fn test_generate_rust_code() { - let cache = create_cache( - "test", - vec![Input { - source: Source::File("testdata/test.aconfig".to_string()), - reader: Box::new(include_bytes!("../testdata/test.aconfig").as_slice()), - }], - vec![ - Input { - source: Source::File("testdata/first.values".to_string()), - reader: Box::new(include_bytes!("../testdata/first.values").as_slice()), - }, - Input { - source: Source::File("testdata/test.aconfig".to_string()), - reader: Box::new(include_bytes!("../testdata/second.values").as_slice()), - }, - ], - ) - .unwrap(); + let cache = crate::test::create_cache(); let generated = generate_rust_code(&cache).unwrap(); assert_eq!("src/lib.rs", format!("{}", generated.path.display())); let expected = r#" diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs index c07bcf3793..b3b6ac408f 100644 --- a/tools/aconfig/src/main.rs +++ b/tools/aconfig/src/main.rs @@ -33,6 +33,9 @@ mod codegen_rust; mod commands; mod protos; +#[cfg(test)] +mod test; + use crate::cache::Cache; use commands::{DumpFormat, Input, OutputFile, Source}; diff --git a/tools/aconfig/src/test.rs b/tools/aconfig/src/test.rs new file mode 100644 index 0000000000..621381afb6 --- /dev/null +++ b/tools/aconfig/src/test.rs @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#[cfg(test)] +pub mod test_utils { + use crate::cache::Cache; + use crate::commands::{Input, Source}; + + pub fn create_cache() -> Cache { + crate::commands::create_cache( + "test", + vec![Input { + source: Source::File("testdata/test.aconfig".to_string()), + reader: Box::new(include_bytes!("../testdata/test.aconfig").as_slice()), + }], + vec![ + Input { + source: Source::File("testdata/first.values".to_string()), + reader: Box::new(include_bytes!("../testdata/first.values").as_slice()), + }, + Input { + source: Source::File("testdata/test.aconfig".to_string()), + reader: Box::new(include_bytes!("../testdata/second.values").as_slice()), + }, + ], + ) + .unwrap() + } +} + +#[cfg(test)] +pub use test_utils::*; From f02734e91586e59048189de3288a1df79b96c2a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Kongstad?= Date: Fri, 2 Jun 2023 11:34:24 +0200 Subject: [PATCH 4/5] aconfig: add create-device-config-defaults command DeviceConfig is the backend for READ_WRITE flags. Add a new command "create-device-config-defaults" to create a file that DeviceConfig will read to pre-populate its data store on first init. This will be used to quickly switch flag values during CI tests: rebuilding and reflashing a device would have the same effect, but would be costlier. This feature is not expected to be used outside CI tests. Note: because DeviceConfig only works with READ_WRITE flags, the generated file excludes READ_ONLY flags. Bug: 285468565 Test: atest aconfig.test Change-Id: I4caff1a10647b8da0ce4e3615678993a957a92dd --- tools/aconfig/src/commands.rs | 76 +++++++++++++++++++++++++---------- tools/aconfig/src/main.rs | 32 ++++++++++++--- 2 files changed, 80 insertions(+), 28 deletions(-) diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index df1a17a9f6..d5e47daa1b 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -22,8 +22,8 @@ use std::fmt; use std::io::Read; use std::path::PathBuf; -use crate::aconfig::{FlagDeclarations, FlagValue}; -use crate::cache::{Cache, CacheBuilder}; +use crate::aconfig::{FlagDeclarations, FlagState, FlagValue, Permission}; +use crate::cache::{Cache, CacheBuilder, Item}; use crate::codegen_cpp::generate_cpp_code; use crate::codegen_java::generate_java_code; use crate::codegen_rust::generate_rust_code; @@ -105,6 +105,24 @@ pub fn create_rust_lib(cache: Cache) -> Result { generate_rust_code(&cache) } +pub fn create_device_config_defaults(caches: Vec) -> Result> { + let mut output = Vec::new(); + for item in sort_and_iter_items(caches).filter(|item| item.permission == Permission::ReadWrite) + { + let line = format!( + "{}/{}:{}\n", + item.namespace, + item.name, + match item.state { + FlagState::Enabled => "enabled", + FlagState::Disabled => "disabled", + } + ); + output.extend_from_slice(line.as_bytes()); + } + Ok(output) +} + #[derive(Copy, Clone, Debug, PartialEq, Eq, ValueEnum)] pub enum DumpFormat { Text, @@ -112,29 +130,26 @@ pub enum DumpFormat { Protobuf, } -pub fn dump_cache(mut caches: Vec, format: DumpFormat) -> Result> { +pub fn dump_cache(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()); + match format { + DumpFormat::Text => { + for item in sort_and_iter_items(caches) { + let line = format!( + "{}/{}: {:?} {:?}\n", + item.namespace, item.name, item.state, item.permission + ); + output.extend_from_slice(line.as_bytes()); } - DumpFormat::Debug => { - let mut lines = vec![]; - for item in cache.iter() { - lines.push(format!("{:#?}\n", item)); - } - output.append(&mut lines.concat().into()); + } + DumpFormat::Debug => { + for item in sort_and_iter_items(caches) { + let line = format!("{:#?}\n", item); + output.extend_from_slice(line.as_bytes()); } - DumpFormat::Protobuf => { + } + DumpFormat::Protobuf => { + for cache in sort_and_iter_caches(caches) { let parsed_flags: ProtoParsedFlags = cache.into(); parsed_flags.write_to_vec(&mut output)?; } @@ -143,6 +158,15 @@ pub fn dump_cache(mut caches: Vec, format: DumpFormat) -> Result> Ok(output) } +fn sort_and_iter_items(caches: Vec) -> impl Iterator { + sort_and_iter_caches(caches).flat_map(|cache| cache.into_iter()) +} + +fn sort_and_iter_caches(mut caches: Vec) -> impl Iterator { + caches.sort_by_cached_key(|cache| cache.namespace().to_string()); + caches.into_iter() +} + #[cfg(test)] mod tests { use super::*; @@ -202,6 +226,14 @@ mod tests { assert_eq!(Permission::ReadOnly, item.permission); } + #[test] + fn test_create_device_config_defaults() { + let caches = vec![crate::test::create_cache()]; + let bytes = create_device_config_defaults(caches).unwrap(); + let text = std::str::from_utf8(&bytes).unwrap(); + assert_eq!("test/disabled_rw:disabled\ntest/enabled_rw:enabled\n", text); + } + #[test] fn test_dump_text_format() { let caches = vec![create_test_cache_ns1()]; diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs index b3b6ac408f..f632ce7efc 100644 --- a/tools/aconfig/src/main.rs +++ b/tools/aconfig/src/main.rs @@ -64,6 +64,11 @@ fn cli() -> Command { .arg(Arg::new("cache").long("cache").required(true)) .arg(Arg::new("out").long("out").required(true)), ) + .subcommand( + Command::new("create-device-config-defaults") + .arg(Arg::new("cache").long("cache").action(ArgAction::Append).required(true)) + .arg(Arg::new("out").long("out").default_value("-")), + ) .subcommand( Command::new("dump") .arg(Arg::new("cache").long("cache").action(ArgAction::Append).required(true)) @@ -111,6 +116,15 @@ fn write_output_file_realtive_to_dir(root: &Path, output_file: &OutputFile) -> R Ok(()) } +fn write_output_to_file_or_stdout(path: &str, data: &[u8]) -> Result<()> { + if path == "-" { + io::stdout().write_all(data)?; + } else { + fs::File::create(path)?.write_all(data)?; + } + Ok(()) +} + fn main() -> Result<()> { let matches = cli().get_matches(); match matches.subcommand() { @@ -147,6 +161,17 @@ fn main() -> Result<()> { let generated_file = commands::create_rust_lib(cache)?; write_output_file_realtive_to_dir(&dir, &generated_file)?; } + Some(("create-device-config-defaults", sub_matches)) => { + 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 output = commands::create_device_config_defaults(caches)?; + let path = get_required_arg::(sub_matches, "out")?; + write_output_to_file_or_stdout(path, &output)?; + } Some(("dump", sub_matches)) => { let mut caches = Vec::new(); for path in sub_matches.get_many::("cache").unwrap_or_default() { @@ -157,12 +182,7 @@ fn main() -> Result<()> { let format = get_required_arg::(sub_matches, "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()) - } else { - Box::new(fs::File::create(path)?) - }; - file.write_all(&output)?; + write_output_to_file_or_stdout(path, &output)?; } _ => unreachable!(), } From c31a6ff6534dfa558f1dda484fffd192d55eb17c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Kongstad?= Date: Fri, 2 Jun 2023 11:54:36 +0200 Subject: [PATCH 5/5] aconfig: add create-device-config-sysprops command Add a new "create-device-config-sysprops" command that works like "create-device-config-defaults" but for system properties. DeviceConfig is a Java service, and will mirror (some of) its values by setting system properties in the persist.device_config namespace. Native code will access DeviceConfig (actually, the system properties) via the server_configurable_flags library. The new command writes a file that can be appended to /system/build.prop to pre-populate persist.device_config before DeviceConfig has started. Like create-device-config-defaults, the new command skips READ_ONLY flags. Bug: 285468565 Test: atest aconfig.test Change-Id: I311c7c5e0b03dc897b09204137d43cc182324717 --- tools/aconfig/src/commands.rs | 26 ++++++++++++++++++++++++++ tools/aconfig/src/main.rs | 16 ++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index d5e47daa1b..3ae72c67fa 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -123,6 +123,24 @@ pub fn create_device_config_defaults(caches: Vec) -> Result> { Ok(output) } +pub fn create_device_config_sysprops(caches: Vec) -> Result> { + let mut output = Vec::new(); + for item in sort_and_iter_items(caches).filter(|item| item.permission == Permission::ReadWrite) + { + let line = format!( + "persist.device_config.{}.{}={}\n", + item.namespace, + item.name, + match item.state { + FlagState::Enabled => "true", + FlagState::Disabled => "false", + } + ); + output.extend_from_slice(line.as_bytes()); + } + Ok(output) +} + #[derive(Copy, Clone, Debug, PartialEq, Eq, ValueEnum)] pub enum DumpFormat { Text, @@ -234,6 +252,14 @@ mod tests { assert_eq!("test/disabled_rw:disabled\ntest/enabled_rw:enabled\n", text); } + #[test] + fn test_create_device_config_sysprops() { + let caches = vec![crate::test::create_cache()]; + let bytes = create_device_config_sysprops(caches).unwrap(); + let text = std::str::from_utf8(&bytes).unwrap(); + assert_eq!("persist.device_config.test.disabled_rw=false\npersist.device_config.test.enabled_rw=true\n", text); + } + #[test] fn test_dump_text_format() { let caches = vec![create_test_cache_ns1()]; diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs index f632ce7efc..dab01918f0 100644 --- a/tools/aconfig/src/main.rs +++ b/tools/aconfig/src/main.rs @@ -69,6 +69,11 @@ fn cli() -> Command { .arg(Arg::new("cache").long("cache").action(ArgAction::Append).required(true)) .arg(Arg::new("out").long("out").default_value("-")), ) + .subcommand( + Command::new("create-device-config-sysprops") + .arg(Arg::new("cache").long("cache").action(ArgAction::Append).required(true)) + .arg(Arg::new("out").long("out").default_value("-")), + ) .subcommand( Command::new("dump") .arg(Arg::new("cache").long("cache").action(ArgAction::Append).required(true)) @@ -172,6 +177,17 @@ fn main() -> Result<()> { let path = get_required_arg::(sub_matches, "out")?; write_output_to_file_or_stdout(path, &output)?; } + Some(("create-device-config-sysprops", sub_matches)) => { + 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 output = commands::create_device_config_sysprops(caches)?; + let path = get_required_arg::(sub_matches, "out")?; + write_output_to_file_or_stdout(path, &output)?; + } Some(("dump", sub_matches)) => { let mut caches = Vec::new(); for path in sub_matches.get_many::("cache").unwrap_or_default() {