diff --git a/tools/aconfig/protos/aconfig.proto b/tools/aconfig/protos/aconfig.proto index 1a80b0420d..ed4b24c32c 100644 --- a/tools/aconfig/protos/aconfig.proto +++ b/tools/aconfig/protos/aconfig.proto @@ -41,8 +41,23 @@ message flag_declaration { repeated string bug = 4; optional bool is_fixed_read_only = 5; optional bool is_exported = 6; + optional flag_metadata metadata = 7; }; +// Optional metadata about the flag, such as its purpose and its intended form factors. +// Can influence the applied policies and testing strategy. +message flag_metadata { + enum flag_purpose { + PURPOSE_UNSPECIFIED = 0; + PURPOSE_FEATURE = 1; + PURPOSE_BUGFIX = 2; + } + + optional flag_purpose purpose = 1; + + // TODO(b/315025930): Add field to designate intended target device form factor(s), such as phone, watch or other. +} + message flag_declarations { optional string package = 1; repeated flag_declaration flag = 2; @@ -81,6 +96,7 @@ message parsed_flag { optional bool is_fixed_read_only = 9; optional bool is_exported = 10; optional string container = 11; + optional flag_metadata metadata = 12; } message parsed_flags { diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index 37ee79e4da..1e80d267e2 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -24,7 +24,8 @@ use crate::codegen_cpp::generate_cpp_code; use crate::codegen_java::generate_java_code; use crate::codegen_rust::generate_rust_code; use crate::protos::{ - ProtoFlagPermission, ProtoFlagState, ProtoParsedFlag, ProtoParsedFlags, ProtoTracepoint, + ProtoFlagMetadata, ProtoFlagPermission, ProtoFlagState, ProtoParsedFlag, ProtoParsedFlags, + ProtoTracepoint, }; pub struct Input { @@ -118,6 +119,11 @@ pub fn parse_flags( tracepoint.set_permission(flag_permission); parsed_flag.trace.push(tracepoint); + let mut metadata = ProtoFlagMetadata::new(); + let purpose = flag_declaration.metadata.purpose(); + metadata.set_purpose(purpose); + parsed_flag.metadata = Some(metadata).into(); + // verify ParsedFlag looks reasonable crate::protos::parsed_flag::verify_fields(&parsed_flag)?; @@ -264,7 +270,11 @@ pub enum DumpFormat { Textproto, } -pub fn dump_parsed_flags(mut input: Vec, format: DumpFormat, dedup: bool) -> 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 = @@ -325,6 +335,7 @@ fn find_unique_package(parsed_flags: &ProtoParsedFlags) -> Option<&str> { #[cfg(test)] mod tests { use super::*; + use crate::protos::ProtoFlagPurpose; #[test] fn test_parse_flags() { @@ -339,6 +350,7 @@ mod tests { assert_eq!("This flag is ENABLED + READ_ONLY", enabled_ro.description()); assert_eq!(ProtoFlagState::ENABLED, enabled_ro.state()); assert_eq!(ProtoFlagPermission::READ_ONLY, enabled_ro.permission()); + assert_eq!(ProtoFlagPurpose::PURPOSE_BUGFIX, enabled_ro.metadata.purpose()); assert_eq!(3, enabled_ro.trace.len()); assert!(!enabled_ro.is_fixed_read_only()); assert_eq!("tests/test.aconfig", enabled_ro.trace[0].source()); @@ -510,6 +522,41 @@ mod tests { ); } + #[test] + fn test_parse_flags_metadata() { + let metadata_flag = r#" + package: "com.first" + flag { + name: "first" + namespace: "first_ns" + description: "This is the description of this feature flag." + bug: "123" + metadata { + purpose: PURPOSE_FEATURE + } + } + "#; + let declaration = vec![Input { + source: "memory".to_string(), + reader: Box::new(metadata_flag.as_bytes()), + }]; + let value: Vec = vec![]; + + let flags_bytes = crate::commands::parse_flags( + "com.first", + None, + declaration, + value, + ProtoFlagPermission::READ_ONLY, + ) + .unwrap(); + let parsed_flags = + crate::protos::parsed_flags::try_from_binary_proto(&flags_bytes).unwrap(); + assert_eq!(1, parsed_flags.parsed_flag.len()); + let parsed_flag = parsed_flags.parsed_flag.first().unwrap(); + assert_eq!(ProtoFlagPurpose::PURPOSE_FEATURE, parsed_flag.metadata.purpose()); + } + #[test] fn test_create_device_config_defaults() { let input = parse_test_flags_as_input(); diff --git a/tools/aconfig/src/protos.rs b/tools/aconfig/src/protos.rs index 3d9089cb42..37eb67d85a 100644 --- a/tools/aconfig/src/protos.rs +++ b/tools/aconfig/src/protos.rs @@ -29,8 +29,10 @@ // ---- When building with the Android tool-chain ---- #[cfg(not(feature = "cargo"))] mod auto_generated { + pub use aconfig_protos::aconfig::flag_metadata::Flag_purpose as ProtoFlagPurpose; pub use aconfig_protos::aconfig::Flag_declaration as ProtoFlagDeclaration; pub use aconfig_protos::aconfig::Flag_declarations as ProtoFlagDeclarations; + pub use aconfig_protos::aconfig::Flag_metadata as ProtoFlagMetadata; pub use aconfig_protos::aconfig::Flag_permission as ProtoFlagPermission; pub use aconfig_protos::aconfig::Flag_state as ProtoFlagState; pub use aconfig_protos::aconfig::Flag_value as ProtoFlagValue; @@ -47,8 +49,10 @@ mod auto_generated { // because this is only used during local development, and only if using cargo instead of the // Android tool-chain, we allow it include!(concat!(env!("OUT_DIR"), "/aconfig_proto/mod.rs")); + pub use aconfig::flag_metadata::Flag_purpose as ProtoFlagPurpose; pub use aconfig::Flag_declaration as ProtoFlagDeclaration; pub use aconfig::Flag_declarations as ProtoFlagDeclarations; + pub use aconfig::Flag_metadata as ProtoFlagMetadata; pub use aconfig::Flag_permission as ProtoFlagPermission; pub use aconfig::Flag_state as ProtoFlagState; pub use aconfig::Flag_value as ProtoFlagValue; @@ -938,11 +942,13 @@ parsed_flag { 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(); + 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(); + 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 @@ -950,10 +956,22 @@ parsed_flag { 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()); + 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()); diff --git a/tools/aconfig/src/test.rs b/tools/aconfig/src/test.rs index 8df4493250..2c63fefdbc 100644 --- a/tools/aconfig/src/test.rs +++ b/tools/aconfig/src/test.rs @@ -44,6 +44,9 @@ parsed_flag { is_fixed_read_only: false is_exported: false container: "system" + metadata { + purpose: PURPOSE_UNSPECIFIED + } } parsed_flag { package: "com.android.aconfig.test" @@ -61,6 +64,9 @@ parsed_flag { is_fixed_read_only: false is_exported: true container: "system" + metadata { + purpose: PURPOSE_UNSPECIFIED + } } parsed_flag { package: "com.android.aconfig.test" @@ -83,6 +89,9 @@ parsed_flag { is_fixed_read_only: false is_exported: true container: "system" + metadata { + purpose: PURPOSE_UNSPECIFIED + } } parsed_flag { package: "com.android.aconfig.test" @@ -105,6 +114,9 @@ parsed_flag { is_fixed_read_only: false is_exported: false container: "system" + metadata { + purpose: PURPOSE_UNSPECIFIED + } } parsed_flag { package: "com.android.aconfig.test" @@ -127,6 +139,9 @@ parsed_flag { is_fixed_read_only: true is_exported: false container: "system" + metadata { + purpose: PURPOSE_UNSPECIFIED + } } parsed_flag { package: "com.android.aconfig.test" @@ -154,6 +169,9 @@ parsed_flag { is_fixed_read_only: false is_exported: false container: "system" + metadata { + purpose: PURPOSE_BUGFIX + } } parsed_flag { package: "com.android.aconfig.test" @@ -176,6 +194,9 @@ parsed_flag { is_fixed_read_only: false is_exported: false container: "system" + metadata { + purpose: PURPOSE_UNSPECIFIED + } } "#; diff --git a/tools/aconfig/tests/test.aconfig b/tools/aconfig/tests/test.aconfig index e44d15466e..310d0a640f 100644 --- a/tools/aconfig/tests/test.aconfig +++ b/tools/aconfig/tests/test.aconfig @@ -10,6 +10,9 @@ flag { namespace: "aconfig_test" description: "This flag is ENABLED + READ_ONLY" bug: "abc" + metadata { + purpose: PURPOSE_BUGFIX + } } # This flag's final value is calculated from: