Add container field to flag_declarations
A container is software which is always built in its entirety using the
same build environment. In particular, all of its parts are built using
the same build-time default flag values. In addition, containers are
always installed as a single unit.
Bug: 312696545
Test: atest aconfig.test && m all_aconfig_declarations
Change-Id: I2ef3db836c4456f4f4fb5c066edf9094e38f89cc
diff --git a/tools/aconfig/protos/aconfig.proto b/tools/aconfig/protos/aconfig.proto
index 9e193ec..1a80b04 100644
--- a/tools/aconfig/protos/aconfig.proto
+++ b/tools/aconfig/protos/aconfig.proto
@@ -46,6 +46,7 @@
message flag_declarations {
optional string package = 1;
repeated flag_declaration flag = 2;
+ optional string container = 3;
};
message flag_value {
@@ -79,7 +80,7 @@
repeated tracepoint trace = 8;
optional bool is_fixed_read_only = 9;
optional bool is_exported = 10;
-
+ optional string container = 11;
}
message parsed_flags {
diff --git a/tools/aconfig/src/codegen.rs b/tools/aconfig/src/codegen.rs
index b7fb08f..fef7a3f 100644
--- a/tools/aconfig/src/codegen.rs
+++ b/tools/aconfig/src/codegen.rs
@@ -38,6 +38,10 @@
s.split('.').all(is_valid_name_ident)
}
+pub fn is_valid_container_ident(s: &str) -> bool {
+ is_valid_name_ident(s) || s.split('.').all(is_valid_name_ident)
+}
+
pub fn create_device_config_ident(package: &str, flag_name: &str) -> Result<String> {
ensure!(is_valid_package_ident(package), "bad package");
ensure!(is_valid_name_ident(flag_name), "bad flag name");
@@ -85,6 +89,29 @@
}
#[test]
+ fn test_is_valid_container_ident() {
+ assert!(is_valid_container_ident("foo.bar"));
+ assert!(is_valid_container_ident("foo.bar_baz"));
+ assert!(is_valid_container_ident("foo.bar.a123"));
+ assert!(is_valid_container_ident("foo"));
+ assert!(is_valid_container_ident("foo_bar_123"));
+
+ assert!(!is_valid_container_ident(""));
+ assert!(!is_valid_container_ident("foo._bar"));
+ assert!(!is_valid_container_ident("_foo"));
+ assert!(!is_valid_container_ident("123_foo"));
+ assert!(!is_valid_container_ident("foo-bar"));
+ assert!(!is_valid_container_ident("foo-b\u{00e5}r"));
+ assert!(!is_valid_container_ident("foo.bar.123"));
+ assert!(!is_valid_container_ident(".foo.bar"));
+ assert!(!is_valid_container_ident("foo.bar."));
+ assert!(!is_valid_container_ident("."));
+ assert!(!is_valid_container_ident(".."));
+ assert!(!is_valid_container_ident("foo..bar"));
+ assert!(!is_valid_container_ident("foo.__bar"));
+ }
+
+ #[test]
fn test_create_device_config_ident() {
assert_eq!(
"com.foo.bar.some_flag",
diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs
index 47e90ac..d04c0cf 100644
--- a/tools/aconfig/src/commands.rs
+++ b/tools/aconfig/src/commands.rs
@@ -57,6 +57,7 @@
pub fn parse_flags(
package: &str,
+ container: Option<&str>,
declarations: Vec<Input>,
values: Vec<Input>,
default_permission: ProtoFlagPermission,
@@ -79,12 +80,24 @@
package,
flag_declarations.package()
);
+ if let Some(c) = container {
+ ensure!(
+ c == flag_declarations.container(),
+ "failed to parse {}: expected container {}, got {}",
+ input.source,
+ c,
+ flag_declarations.container()
+ );
+ }
for mut flag_declaration in flag_declarations.flag.into_iter() {
crate::protos::flag_declaration::verify_fields(&flag_declaration)
.with_context(|| input.error_context())?;
// create ParsedFlag using FlagDeclaration and default values
let mut parsed_flag = ProtoParsedFlag::new();
+ if let Some(c) = container {
+ parsed_flag.set_container(c.to_string());
+ }
parsed_flag.set_package(package.to_string());
parsed_flag.set_name(flag_declaration.take_name());
parsed_flag.set_namespace(flag_declaration.take_namespace());
@@ -262,9 +275,10 @@
DumpFormat::Text => {
for parsed_flag in parsed_flags.parsed_flag.into_iter() {
let line = format!(
- "{}.{}: {:?} + {:?}\n",
+ "{}.{} [{}]: {:?} + {:?}\n",
parsed_flag.package(),
parsed_flag.name(),
+ parsed_flag.container(),
parsed_flag.permission(),
parsed_flag.state()
);
@@ -276,9 +290,10 @@
let sources: Vec<_> =
parsed_flag.trace.iter().map(|tracepoint| tracepoint.source()).collect();
let line = format!(
- "{}.{}: {:?} + {:?} ({})\n",
+ "{}.{} [{}]: {:?} + {:?} ({})\n",
parsed_flag.package(),
parsed_flag.name(),
+ parsed_flag.container(),
parsed_flag.permission(),
parsed_flag.state(),
sources.join(", ")
@@ -377,6 +392,7 @@
let flags_bytes = crate::commands::parse_flags(
"com.first",
+ None,
declaration,
value,
ProtoFlagPermission::READ_ONLY,
@@ -391,9 +407,72 @@
}
#[test]
+ fn test_parse_flags_package_mismatch_between_declaration_and_command_line() {
+ let first_flag = r#"
+ package: "com.declaration.package"
+ container: "first.container"
+ flag {
+ name: "first"
+ namespace: "first_ns"
+ description: "This is the description of the first flag."
+ bug: "123"
+ }
+ "#;
+ let declaration =
+ vec![Input { source: "memory".to_string(), reader: Box::new(first_flag.as_bytes()) }];
+
+ let value: Vec<Input> = vec![];
+
+ let error = crate::commands::parse_flags(
+ "com.argument.package",
+ Some("first.container"),
+ declaration,
+ value,
+ ProtoFlagPermission::READ_WRITE,
+ )
+ .unwrap_err();
+ assert_eq!(
+ format!("{:?}", error),
+ "failed to parse memory: expected package com.argument.package, got com.declaration.package"
+ );
+ }
+
+ #[test]
+ fn test_parse_flags_container_mismatch_between_declaration_and_command_line() {
+ let first_flag = r#"
+ package: "com.first"
+ container: "declaration.container"
+ flag {
+ name: "first"
+ namespace: "first_ns"
+ description: "This is the description of the first flag."
+ bug: "123"
+ }
+ "#;
+ let declaration =
+ vec![Input { source: "memory".to_string(), reader: Box::new(first_flag.as_bytes()) }];
+
+ let value: Vec<Input> = vec![];
+
+ let error = crate::commands::parse_flags(
+ "com.first",
+ Some("argument.container"),
+ declaration,
+ value,
+ ProtoFlagPermission::READ_WRITE,
+ )
+ .unwrap_err();
+ assert_eq!(
+ format!("{:?}", error),
+ "failed to parse memory: expected container argument.container, got declaration.container"
+ );
+ }
+
+ #[test]
fn test_parse_flags_override_fixed_read_only() {
let first_flag = r#"
package: "com.first"
+ container: "com.first.container"
flag {
name: "first"
namespace: "first_ns"
@@ -419,6 +498,7 @@
}];
let error = crate::commands::parse_flags(
"com.first",
+ Some("com.first.container"),
declaration,
value,
ProtoFlagPermission::READ_WRITE,
@@ -451,7 +531,9 @@
let input = parse_test_flags_as_input();
let bytes = dump_parsed_flags(vec![input], DumpFormat::Text).unwrap();
let text = std::str::from_utf8(&bytes).unwrap();
- assert!(text.contains("com.android.aconfig.test.disabled_ro: READ_ONLY + DISABLED"));
+ assert!(
+ text.contains("com.android.aconfig.test.disabled_ro [system]: READ_ONLY + DISABLED")
+ );
}
#[test]
diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs
index 7e44baf..af0368a 100644
--- a/tools/aconfig/src/main.rs
+++ b/tools/aconfig/src/main.rs
@@ -42,6 +42,8 @@
.subcommand(
Command::new("create-cache")
.arg(Arg::new("package").long("package").required(true))
+ // TODO(b/312769710): Make this argument required.
+ .arg(Arg::new("container").long("container"))
.arg(Arg::new("declarations").long("declarations").action(ArgAction::Append))
.arg(Arg::new("values").long("values").action(ArgAction::Append))
.arg(
@@ -119,6 +121,13 @@
.ok_or(anyhow!("internal error: required argument '{}' not found", arg_name))
}
+fn get_optional_arg<'a, T>(matches: &'a ArgMatches, arg_name: &str) -> Option<&'a T>
+where
+ T: Any + Clone + Send + Sync + 'static,
+{
+ matches.get_one::<T>(arg_name)
+}
+
fn open_zero_or_more_files(matches: &ArgMatches, arg_name: &str) -> Result<Vec<Input>> {
let mut opened_files = vec![];
for path in matches.get_many::<String>(arg_name).unwrap_or_default() {
@@ -167,12 +176,20 @@
match matches.subcommand() {
Some(("create-cache", sub_matches)) => {
let package = get_required_arg::<String>(sub_matches, "package")?;
+ let container =
+ get_optional_arg::<String>(sub_matches, "container").map(|c| c.as_str());
let declarations = open_zero_or_more_files(sub_matches, "declarations")?;
let values = open_zero_or_more_files(sub_matches, "values")?;
let default_permission =
get_required_arg::<protos::ProtoFlagPermission>(sub_matches, "default-permission")?;
- let output = commands::parse_flags(package, declarations, values, *default_permission)
- .context("failed to create cache")?;
+ let output = commands::parse_flags(
+ package,
+ container,
+ declarations,
+ values,
+ *default_permission,
+ )
+ .context("failed to create cache")?;
let path = get_required_arg::<String>(sub_matches, "cache")?;
write_output_to_file_or_stdout(path, &output)?;
}
diff --git a/tools/aconfig/src/protos.rs b/tools/aconfig/src/protos.rs
index a5a5342..c11596b 100644
--- a/tools/aconfig/src/protos.rs
+++ b/tools/aconfig/src/protos.rs
@@ -111,11 +111,16 @@
pub fn verify_fields(pdf: &ProtoFlagDeclarations) -> Result<()> {
ensure_required_fields!("flag declarations", pdf, "package");
+ // TODO(b/312769710): Make the container field required.
ensure!(
codegen::is_valid_package_ident(pdf.package()),
"bad flag declarations: bad package"
);
+ ensure!(
+ !pdf.has_container() || codegen::is_valid_container_ident(pdf.container()),
+ "bad flag declarations: bad container"
+ );
for flag_declaration in pdf.flag.iter() {
super::flag_declaration::verify_fields(flag_declaration)?;
}
@@ -207,6 +212,10 @@
);
ensure!(codegen::is_valid_package_ident(pf.package()), "bad parsed flag: bad package");
+ ensure!(
+ !pf.has_container() || codegen::is_valid_container_ident(pf.container()),
+ "bad parsed flag: bad container"
+ );
ensure!(codegen::is_valid_name_ident(pf.name()), "bad parsed flag: bad name");
ensure!(codegen::is_valid_name_ident(pf.namespace()), "bad parsed flag: bad namespace");
ensure!(!pf.description().is_empty(), "bad parsed flag: empty description");
@@ -303,6 +312,7 @@
let flag_declarations = flag_declarations::try_from_text_proto(
r#"
package: "com.foo.bar"
+container: "system"
flag {
name: "first"
namespace: "first_ns"
@@ -321,6 +331,7 @@
)
.unwrap();
assert_eq!(flag_declarations.package(), "com.foo.bar");
+ assert_eq!(flag_declarations.container(), "system");
let first = flag_declarations.flag.iter().find(|pf| pf.name() == "first").unwrap();
assert_eq!(first.name(), "first");
assert_eq!(first.namespace(), "first_ns");
@@ -336,9 +347,26 @@
assert!(second.is_fixed_read_only());
assert!(!second.is_exported());
+ // valid input: missing container in flag declarations is supported
+ let flag_declarations = flag_declarations::try_from_text_proto(
+ r#"
+package: "com.foo.bar"
+flag {
+ name: "first"
+ namespace: "first_ns"
+ description: "This is the description of the first flag."
+ bug: "123"
+}
+"#,
+ )
+ .unwrap();
+ assert_eq!(flag_declarations.container(), "");
+ assert!(!flag_declarations.has_container());
+
// bad input: missing package in flag declarations
let error = flag_declarations::try_from_text_proto(
r#"
+container: "system"
flag {
name: "first"
namespace: "first_ns"
@@ -358,6 +386,7 @@
let error = flag_declarations::try_from_text_proto(
r#"
package: "com.foo.bar"
+container: "system"
flag {
name: "first"
description: "This is the description of the first flag."
@@ -376,6 +405,7 @@
let error = flag_declarations::try_from_text_proto(
r#"
package: "_com.FOO__BAR"
+container: "system"
flag {
name: "first"
namespace: "first_ns"
@@ -395,6 +425,7 @@
let error = flag_declarations::try_from_text_proto(
r#"
package: "com.foo.bar"
+container: "system"
flag {
name: "FIRST"
namespace: "first_ns"
@@ -414,6 +445,7 @@
let error = flag_declarations::try_from_text_proto(
r#"
package: "com.foo.bar"
+container: "system"
flag {
name: "first"
namespace: "first_ns"
@@ -428,6 +460,7 @@
let error = flag_declarations::try_from_text_proto(
r#"
package: "com.foo.bar"
+container: "system"
flag {
name: "first"
namespace: "first_ns"
@@ -439,6 +472,25 @@
)
.unwrap_err();
assert!(format!("{:?}", error).contains("bad flag declaration: exactly one bug required"));
+
+ // bad input: invalid container name in flag declaration
+ let error = flag_declarations::try_from_text_proto(
+ r#"
+package: "com.foo.bar"
+container: "__bad_bad_container.com"
+flag {
+ name: "first"
+ namespace: "first_ns"
+ description: "This is the description of the first flag."
+ bug: "123"
+ bug: "abc"
+}
+"#,
+ )
+ .unwrap_err();
+ assert!(format!("{:?}", error).contains("bad flag declarations: bad container"));
+
+ // TODO(b/312769710): Verify error when container is missing.
}
#[test]
@@ -553,6 +605,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
parsed_flag {
package: "com.second"
@@ -573,6 +626,7 @@
permission: READ_ONLY
}
is_fixed_read_only: true
+ container: "system"
}
"#;
let parsed_flags = try_from_binary_proto_from_text_proto(text_proto).unwrap();
@@ -607,6 +661,7 @@
description: "This is the description of the first flag."
state: DISABLED
permission: READ_ONLY
+ container: "system"
}
"#;
let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err();
@@ -625,6 +680,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
"#;
let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err();
@@ -645,6 +701,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
parsed_flag {
package: "aaa.aaa"
@@ -659,6 +716,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
"#;
let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err();
@@ -682,6 +740,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
parsed_flag {
package: "com.foo"
@@ -696,6 +755,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
"#;
let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err();
@@ -719,6 +779,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
parsed_flag {
package: "com.foo"
@@ -733,6 +794,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
"#;
let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err();
@@ -760,6 +822,7 @@
state: ENABLED
permission: READ_ONLY
}
+ container: "system"
}
"#;
let parsed_flags = try_from_binary_proto_from_text_proto(text_proto).unwrap();
@@ -786,6 +849,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
parsed_flag {
package: "com.second"
@@ -800,6 +864,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
"#;
let expected = try_from_binary_proto_from_text_proto(text_proto).unwrap();
@@ -818,6 +883,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
"#;
let first = try_from_binary_proto_from_text_proto(text_proto).unwrap();
@@ -836,6 +902,7 @@
state: DISABLED
permission: READ_ONLY
}
+ container: "system"
}
"#;
let second = try_from_binary_proto_from_text_proto(text_proto).unwrap();
diff --git a/tools/aconfig/src/test.rs b/tools/aconfig/src/test.rs
index 9f598d0..8df4493 100644
--- a/tools/aconfig/src/test.rs
+++ b/tools/aconfig/src/test.rs
@@ -43,6 +43,7 @@
}
is_fixed_read_only: false
is_exported: false
+ container: "system"
}
parsed_flag {
package: "com.android.aconfig.test"
@@ -59,6 +60,7 @@
}
is_fixed_read_only: false
is_exported: true
+ container: "system"
}
parsed_flag {
package: "com.android.aconfig.test"
@@ -80,6 +82,7 @@
}
is_fixed_read_only: false
is_exported: true
+ container: "system"
}
parsed_flag {
package: "com.android.aconfig.test"
@@ -101,6 +104,7 @@
}
is_fixed_read_only: false
is_exported: false
+ container: "system"
}
parsed_flag {
package: "com.android.aconfig.test"
@@ -122,6 +126,7 @@
}
is_fixed_read_only: true
is_exported: false
+ container: "system"
}
parsed_flag {
package: "com.android.aconfig.test"
@@ -148,6 +153,7 @@
}
is_fixed_read_only: false
is_exported: false
+ container: "system"
}
parsed_flag {
package: "com.android.aconfig.test"
@@ -169,12 +175,14 @@
}
is_fixed_read_only: false
is_exported: false
+ container: "system"
}
"#;
pub fn parse_test_flags() -> ProtoParsedFlags {
let bytes = crate::commands::parse_flags(
"com.android.aconfig.test",
+ Some("system"),
vec![Input {
source: "tests/test.aconfig".to_string(),
reader: Box::new(include_bytes!("../tests/test.aconfig").as_slice()),
diff --git a/tools/aconfig/tests/test.aconfig b/tools/aconfig/tests/test.aconfig
index 8a1a913..e44d154 100644
--- a/tools/aconfig/tests/test.aconfig
+++ b/tools/aconfig/tests/test.aconfig
@@ -1,4 +1,5 @@
package: "com.android.aconfig.test"
+container: "system"
# This flag's final value is calculated from:
# - test.aconfig: DISABLED + READ_WRITE (default)