Merge "aconfig: add proto bug field" am: 37cb74b3e4

Original change: https://android-review.googlesource.com/c/platform/build/+/2634549

Change-Id: I0c846a9c290ab59f1dc3a46e9d519f9a318c5793
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

tools/aconfig/Android.bp

@@ -27,6 +27,9 @@ rust_defaults {
         "libserde_json",
         "libtinytemplate",
     ],
+    proc_macros: [
+        "libpaste",
+    ]
 }
 
 rust_binary_host {

tools/aconfig/Cargo.toml

@@ -11,6 +11,7 @@ cargo = []
 [dependencies]
 anyhow = "1.0.69"
 clap = { version = "4.1.8", features = ["derive"] }
+paste = "1.0.11"
 protobuf = "3.2.0"
 serde = { version = "1.0.152", features = ["derive"] }
 serde_json = "1.0.93"

tools/aconfig/protos/aconfig.proto

@@ -38,6 +38,7 @@ message flag_declaration {
   optional string name = 1;
   optional string namespace = 2;
   optional string description = 3;
+  repeated string bug = 4;
 };
 
 message flag_declarations {
@@ -70,9 +71,10 @@ message parsed_flag {
   optional string name = 2;
   optional string namespace = 3;
   optional string description = 4;
-  optional flag_state state = 5;
+  repeated string bug = 5;
-  optional flag_permission permission = 6;
+  optional flag_state state = 6;
-  repeated tracepoint trace = 7;
+  optional flag_permission permission = 7;
+  repeated tracepoint trace = 8;
 }
 
 message parsed_flags {

tools/aconfig/src/commands.rs

@@ -74,6 +74,7 @@ pub fn parse_flags(package: &str, declarations: Vec<Input>, values: Vec<Input>)
             parsed_flag.set_name(flag_declaration.take_name());
             parsed_flag.set_namespace(flag_declaration.take_namespace());
             parsed_flag.set_description(flag_declaration.take_description());
+            parsed_flag.bug.append(&mut flag_declaration.bug);
             parsed_flag.set_state(DEFAULT_FLAG_STATE);
             parsed_flag.set_permission(DEFAULT_FLAG_PERMISSION);
             let mut tracepoint = ProtoTracepoint::new();
@@ -311,6 +312,97 @@ mod tests {
         assert!(text.contains("com.android.aconfig.test/disabled_ro: DISABLED READ_ONLY"));
     }
 
+    #[test]
+    fn test_dump_protobuf_format() {
+        let text_proto = r#"
+parsed_flag {
+  package: "com.android.aconfig.test"
+  name: "disabled_ro"
+  namespace: "aconfig_test"
+  description: "This flag is DISABLED + READ_ONLY"
+  state: DISABLED
+  permission: READ_ONLY
+  trace {
+    source: "tests/test.aconfig"
+    state: DISABLED
+    permission: READ_WRITE
+  }
+  trace {
+    source: "tests/first.values"
+    state: DISABLED
+    permission: READ_ONLY
+  }
+  bug: "123"
+}
+parsed_flag {
+  package: "com.android.aconfig.test"
+  name: "disabled_rw"
+  namespace: "aconfig_test"
+  description: "This flag is DISABLED + READ_WRITE"
+  state: DISABLED
+  permission: READ_WRITE
+  trace {
+    source: "tests/test.aconfig"
+    state: DISABLED
+    permission: READ_WRITE
+  }
+  bug: "456"
+}
+parsed_flag {
+  package: "com.android.aconfig.test"
+  name: "enabled_ro"
+  namespace: "aconfig_test"
+  description: "This flag is ENABLED + READ_ONLY"
+  state: ENABLED
+  permission: READ_ONLY
+  trace {
+    source: "tests/test.aconfig"
+    state: DISABLED
+    permission: READ_WRITE
+  }
+  trace {
+    source: "tests/first.values"
+    state: DISABLED
+    permission: READ_WRITE
+  }
+  trace {
+    source: "tests/second.values"
+    state: ENABLED
+    permission: READ_ONLY
+  }
+  bug: "789"
+  bug: "abc"
+}
+parsed_flag {
+  package: "com.android.aconfig.test"
+  name: "enabled_rw"
+  namespace: "aconfig_test"
+  description: "This flag is ENABLED + READ_WRITE"
+  state: ENABLED
+  permission: READ_WRITE
+  trace {
+    source: "tests/test.aconfig"
+    state: DISABLED
+    permission: READ_WRITE
+  }
+  trace {
+    source: "tests/first.values"
+    state: ENABLED
+    permission: READ_WRITE
+  }
+}
+"#;
+        let expected = protobuf::text_format::parse_from_str::<ProtoParsedFlags>(text_proto)
+            .unwrap()
+            .write_to_bytes()
+            .unwrap();
+
+        let input = parse_test_flags_as_input();
+        let actual = dump_parsed_flags(vec![input], DumpFormat::Protobuf).unwrap();
+
+        assert_eq!(expected, actual);
+    }
+
     fn parse_test_flags_as_input() -> Input {
         let parsed_flags = crate::test::parse_test_flags();
         let binary_proto = parsed_flags.write_to_bytes().unwrap();

tools/aconfig/src/protos.rs

@@ -62,29 +62,39 @@ mod auto_generated {
 pub use auto_generated::*;
 
 use anyhow::Result;
+use paste::paste;
 
 fn try_from_text_proto<T>(s: &str) -> Result<T>
 where
     T: protobuf::MessageFull,
 {
-    // warning: parse_from_str does not check if required fields are set
     protobuf::text_format::parse_from_str(s).map_err(|e| e.into())
 }
 
+macro_rules! ensure_required_fields {
+    ($type:expr, $struct:expr, $($field:expr),+) => {
+        $(
+        paste! {
+            ensure!($struct.[<has_ $field>](), "bad {}: missing {}", $type, $field);
+        }
+        )+
+    };
+}
+
 pub mod flag_declaration {
     use super::*;
     use crate::codegen;
     use anyhow::ensure;
 
     pub fn verify_fields(pdf: &ProtoFlagDeclaration) -> Result<()> {
-        ensure!(pdf.has_name(), "bad flag declaration: missing name");
+        ensure_required_fields!("flag declaration", pdf, "name", "namespace", "description");
-        ensure!(pdf.has_namespace(), "bad flag declaration: missing namespace");
-        ensure!(pdf.has_description(), "bad flag declaration: missing description");
 
         ensure!(codegen::is_valid_name_ident(pdf.name()), "bad flag declaration: bad name");
         ensure!(codegen::is_valid_name_ident(pdf.namespace()), "bad flag declaration: bad name");
         ensure!(!pdf.description().is_empty(), "bad flag declaration: empty description");
 
+        // ProtoFlagDeclaration.bug: Vec<String>: may be empty, no checks needed
+
         Ok(())
     }
 }
@@ -101,7 +111,7 @@ pub mod flag_declarations {
     }
 
     pub fn verify_fields(pdf: &ProtoFlagDeclarations) -> Result<()> {
-        ensure!(pdf.has_package(), "bad flag declarations: missing package");
+        ensure_required_fields!("flag declarations", pdf, "package");
 
         ensure!(
             codegen::is_valid_package_ident(pdf.package()),
@@ -121,10 +131,7 @@ pub mod flag_value {
     use anyhow::ensure;
 
     pub fn verify_fields(fv: &ProtoFlagValue) -> Result<()> {
-        ensure!(fv.has_package(), "bad flag value: missing package");
+        ensure_required_fields!("flag value", fv, "package", "name", "state", "permission");
-        ensure!(fv.has_name(), "bad flag value: missing name");
-        ensure!(fv.has_state(), "bad flag value: missing state");
-        ensure!(fv.has_permission(), "bad flag value: missing permission");
 
         ensure!(codegen::is_valid_package_ident(fv.package()), "bad flag value: bad package");
         ensure!(codegen::is_valid_name_ident(fv.name()), "bad flag value: bad name");
@@ -155,9 +162,7 @@ pub mod tracepoint {
     use anyhow::ensure;
 
     pub fn verify_fields(tp: &ProtoTracepoint) -> Result<()> {
-        ensure!(tp.has_source(), "bad tracepoint: missing source");
+        ensure_required_fields!("tracepoint", tp, "source", "state", "permission");
-        ensure!(tp.has_state(), "bad tracepoint: missing state");
-        ensure!(tp.has_permission(), "bad tracepoint: missing permission");
 
         ensure!(!tp.source().is_empty(), "bad tracepoint: empty source");
 
@@ -171,12 +176,16 @@ pub mod parsed_flag {
     use anyhow::ensure;
 
     pub fn verify_fields(pf: &ProtoParsedFlag) -> Result<()> {
-        ensure!(pf.has_package(), "bad parsed flag: missing package");
+        ensure_required_fields!(
-        ensure!(pf.has_name(), "bad parsed flag: missing name");
+            "parsed flag",
-        ensure!(pf.has_namespace(), "bad parsed flag: missing namespace");
+            pf,
-        ensure!(pf.has_description(), "bad parsed flag: missing description");
+            "package",
-        ensure!(pf.has_state(), "bad parsed flag: missing state");
+            "name",
-        ensure!(pf.has_permission(), "bad parsed flag: missing permission");
+            "namespace",
+            "description",
+            "state",
+            "permission"
+        );
 
         ensure!(codegen::is_valid_package_ident(pf.package()), "bad parsed flag: bad package");
         ensure!(codegen::is_valid_name_ident(pf.name()), "bad parsed flag: bad name");
@@ -187,6 +196,8 @@ pub mod parsed_flag {
             super::tracepoint::verify_fields(tp)?;
         }
 
+        // ProtoParsedFlag.bug: Vec<String>: may be empty, no checks needed
+
         Ok(())
     }
 }
@@ -251,6 +262,8 @@ flag {
     name: "first"
     namespace: "first_ns"
     description: "This is the description of the first flag."
+    bug: "123"
+    bug: "abc"
 }
 flag {
     name: "second"
@@ -265,10 +278,14 @@ flag {
         assert_eq!(first.name(), "first");
         assert_eq!(first.namespace(), "first_ns");
         assert_eq!(first.description(), "This is the description of the first flag.");
+        assert_eq!(first.bug.len(), 2);
+        assert_eq!(first.bug[0], "123");
+        assert_eq!(first.bug[1], "abc");
         let second = flag_declarations.flag.iter().find(|pf| pf.name() == "second").unwrap();
         assert_eq!(second.name(), "second");
         assert_eq!(second.namespace(), "second_ns");
         assert_eq!(second.description(), "This is the description of the second flag.");
+        assert_eq!(second.bug.len(), 0);
 
         // bad input: missing package in flag declarations
         let error = flag_declarations::try_from_text_proto(

tools/aconfig/tests/test.aconfig

@@ -7,6 +7,7 @@ flag {
     name: "disabled_ro"
     namespace: "aconfig_test"
     description: "This flag is DISABLED + READ_ONLY"
+    bug: "123"
 }
 
 # This flag's final value is calculated from:
@@ -15,6 +16,7 @@ flag {
     name: "disabled_rw"
     namespace: "aconfig_test"
     description: "This flag is DISABLED + READ_WRITE"
+    bug: "456"
 }
 
 # This flag's final value is calculated from:
@@ -25,6 +27,8 @@ flag {
     name: "enabled_ro"
     namespace: "aconfig_test"
     description: "This flag is ENABLED + READ_ONLY"
+    bug: "789"
+    bug: "abc"
 }
 
 # This flag's final value is calculated from:
@@ -34,4 +38,5 @@ flag {
     name: "enabled_rw"
     namespace: "aconfig_test"
     description: "This flag is ENABLED + READ_WRITE"
+    # no bug field: bug is not mandatory
 }