From 80040d2d2c540b975a024fb484239d0ea8818340 Mon Sep 17 00:00:00 2001 From: Zhi Dou Date: Tue, 19 Dec 2023 19:39:22 +0000 Subject: [PATCH 1/2] aconfig: modify_parsed_flags_based_on_mode return Result This commit changes modify_parsed_flags_based_on_mode to return Result instead of vector. This change will have two consequences. 1. Error will be thrown if no flags fit into the current codegen mode 2. Error will be thrown if the flag declaration is empty Bug: 311152507 Test: atest aconfig.test Change-Id: I69b9a84312faed9f757bf3974b3cea49c5c5e285 --- tools/aconfig/src/codegen/cpp.rs | 2 +- tools/aconfig/src/codegen/mod.rs | 12 +++++++++++- tools/aconfig/src/codegen/rust.rs | 2 +- tools/aconfig/src/commands.rs | 28 ++++++++++++++++++++++------ 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/tools/aconfig/src/codegen/cpp.rs b/tools/aconfig/src/codegen/cpp.rs index 06e5ccab75..d6bebbabf9 100644 --- a/tools/aconfig/src/codegen/cpp.rs +++ b/tools/aconfig/src/codegen/cpp.rs @@ -1028,7 +1028,7 @@ bool com_android_aconfig_test_enabled_ro() { expected_src: &str, ) { let modified_parsed_flags = - crate::commands::modify_parsed_flags_based_on_mode(parsed_flags, mode); + crate::commands::modify_parsed_flags_based_on_mode(parsed_flags, mode).unwrap(); let generated = generate_cpp_code(crate::test::TEST_PACKAGE, modified_parsed_flags.into_iter(), mode) .unwrap(); diff --git a/tools/aconfig/src/codegen/mod.rs b/tools/aconfig/src/codegen/mod.rs index fc61b7b27b..476d2b39b1 100644 --- a/tools/aconfig/src/codegen/mod.rs +++ b/tools/aconfig/src/codegen/mod.rs @@ -55,9 +55,19 @@ pub fn create_device_config_ident(package: &str, flag_name: &str) -> Result std::fmt::Result { + match self { + CodegenMode::Exported => write!(f, "exported"), + CodegenMode::Production => write!(f, "production"), + CodegenMode::Test => write!(f, "test"), + } + } } #[cfg(test)] diff --git a/tools/aconfig/src/codegen/rust.rs b/tools/aconfig/src/codegen/rust.rs index 6cf0b3208e..56cb311754 100644 --- a/tools/aconfig/src/codegen/rust.rs +++ b/tools/aconfig/src/codegen/rust.rs @@ -557,7 +557,7 @@ pub fn enabled_ro_exported() -> bool { fn test_generate_rust_code(mode: CodegenMode) { let parsed_flags = crate::test::parse_test_flags(); let modified_parsed_flags = - crate::commands::modify_parsed_flags_based_on_mode(parsed_flags, mode); + crate::commands::modify_parsed_flags_based_on_mode(parsed_flags, mode).unwrap(); let generated = generate_rust_code(crate::test::TEST_PACKAGE, modified_parsed_flags.into_iter(), mode) .unwrap(); diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index 23667bb9df..7d9dd69354 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -200,7 +200,7 @@ pub fn create_java_lib(mut input: Input, codegen_mode: CodegenMode) -> Result Result> { let parsed_flags = input.try_parse_flags()?; - let modified_parsed_flags = modify_parsed_flags_based_on_mode(parsed_flags, codegen_mode); + let modified_parsed_flags = modify_parsed_flags_based_on_mode(parsed_flags, codegen_mode)?; let Some(package) = find_unique_package(&modified_parsed_flags) else { bail!("no parsed flags, or the parsed flags use different packages"); }; @@ -210,7 +210,7 @@ pub fn create_cpp_lib(mut input: Input, codegen_mode: CodegenMode) -> Result Result { let parsed_flags = input.try_parse_flags()?; - let modified_parsed_flags = modify_parsed_flags_based_on_mode(parsed_flags, codegen_mode); + let modified_parsed_flags = modify_parsed_flags_based_on_mode(parsed_flags, codegen_mode)?; let Some(package) = find_unique_package(&modified_parsed_flags) else { bail!("no parsed flags, or the parsed flags use different packages"); }; @@ -331,7 +331,7 @@ fn filter_parsed_flags( pub fn modify_parsed_flags_based_on_mode( parsed_flags: ProtoParsedFlags, codegen_mode: CodegenMode, -) -> Vec { +) -> Result> { fn exported_mode_flag_modifier(mut parsed_flag: ProtoParsedFlag) -> ProtoParsedFlag { parsed_flag.set_state(ProtoFlagState::DISABLED); parsed_flag.set_permission(ProtoFlagPermission::READ_WRITE); @@ -339,7 +339,7 @@ pub fn modify_parsed_flags_based_on_mode( parsed_flag } - match codegen_mode { + let modified_parsed_flags: Vec<_> = match codegen_mode { CodegenMode::Exported => parsed_flags .parsed_flag .into_iter() @@ -349,7 +349,12 @@ pub fn modify_parsed_flags_based_on_mode( CodegenMode::Production | CodegenMode::Test => { parsed_flags.parsed_flag.into_iter().collect() } + }; + if modified_parsed_flags.is_empty() { + bail!("{} library contains no exported flags.", codegen_mode); } + + Ok(modified_parsed_flags) } #[cfg(test)] @@ -652,7 +657,8 @@ mod tests { fn test_modify_parsed_flags_based_on_mode_prod() { let parsed_flags = crate::test::parse_test_flags(); let p_parsed_flags = - modify_parsed_flags_based_on_mode(parsed_flags.clone(), CodegenMode::Production); + modify_parsed_flags_based_on_mode(parsed_flags.clone(), CodegenMode::Production) + .unwrap(); assert_eq!(parsed_flags.parsed_flag.len(), p_parsed_flags.len()); for (i, item) in p_parsed_flags.iter().enumerate() { assert!(parsed_flags.parsed_flag[i].eq(item)); @@ -662,7 +668,8 @@ mod tests { #[test] fn test_modify_parsed_flags_based_on_mode_exported() { let parsed_flags = crate::test::parse_test_flags(); - let p_parsed_flags = modify_parsed_flags_based_on_mode(parsed_flags, CodegenMode::Exported); + let p_parsed_flags = + modify_parsed_flags_based_on_mode(parsed_flags, CodegenMode::Exported).unwrap(); assert_eq!(3, p_parsed_flags.len()); for flag in p_parsed_flags.iter() { assert_eq!(ProtoFlagState::DISABLED, flag.state()); @@ -670,5 +677,14 @@ mod tests { assert!(!flag.is_fixed_read_only()); assert!(flag.is_exported()); } + + let mut parsed_flags = crate::test::parse_test_flags(); + parsed_flags.parsed_flag.retain_mut(|pf| !pf.is_exported()); + let error = + modify_parsed_flags_based_on_mode(parsed_flags, CodegenMode::Exported).unwrap_err(); + assert_eq!( + format!("{} library contains no exported flags.", CodegenMode::Exported), + format!("{:?}", error) + ); } } From eeed7990de66ad36950ea2682b9b60415cdd2f08 Mon Sep 17 00:00:00 2001 From: Zhi Dou Date: Tue, 19 Dec 2023 20:20:22 +0000 Subject: [PATCH 2/2] aconfig: modify and filter flags before passing into java codegen Before this change java codegen filter flags for exported mode in the template. This change move the filter process to commands as other codegen. Thus the codegen code will only generate code based on the passed in flags. Bug: 311152507 Test: atest aconfig.test aconfig.test.java AconfigJavaHostTest Change-Id: I74045709cde19e6c687c3eb0d94050ea40cf5042 --- tools/aconfig/src/codegen/java.rs | 65 ++++++------------- tools/aconfig/src/commands.rs | 35 +--------- .../FakeFeatureFlagsImpl.java.template | 21 ------ .../templates/FeatureFlags.java.template | 6 -- .../templates/FeatureFlagsImpl.java.template | 27 ++------ tools/aconfig/templates/Flags.java.template | 16 +---- 6 files changed, 28 insertions(+), 142 deletions(-) diff --git a/tools/aconfig/src/codegen/java.rs b/tools/aconfig/src/codegen/java.rs index 6a7d7c1e27..a02a7e2d90 100644 --- a/tools/aconfig/src/codegen/java.rs +++ b/tools/aconfig/src/codegen/java.rs @@ -14,7 +14,7 @@ * limitations under the License. */ -use anyhow::{anyhow, Result}; +use anyhow::Result; use serde::Serialize; use std::collections::{BTreeMap, BTreeSet}; use std::path::PathBuf; @@ -35,8 +35,6 @@ where { let flag_elements: Vec = parsed_flags_iter.map(|pf| create_flag_element(package, &pf)).collect(); - let exported_flag_elements: Vec = - flag_elements.iter().filter(|elem| elem.exported).cloned().collect(); let namespace_flags = gen_flags_by_namespace(&flag_elements); let properties_set: BTreeSet = flag_elements.iter().map(|fe| format_property_name(&fe.device_config_namespace)).collect(); @@ -45,13 +43,8 @@ where let runtime_lookup_required = flag_elements.iter().any(|elem| elem.is_read_write) || library_exported; - if library_exported && exported_flag_elements.is_empty() { - return Err(anyhow!("exported library contains no exported flags")); - } - let context = Context { flag_elements, - exported_flag_elements, namespace_flags, is_test_mode, runtime_lookup_required, @@ -110,7 +103,6 @@ fn gen_flags_by_namespace(flags: &[FlagElement]) -> Vec { #[derive(Serialize)] struct Context { pub flag_elements: Vec, - pub exported_flag_elements: Vec, pub namespace_flags: Vec, pub is_test_mode: bool, pub runtime_lookup_required: bool, @@ -134,7 +126,6 @@ struct FlagElement { pub is_read_write: bool, pub method_name: String, pub properties: String, - pub exported: bool, } fn create_flag_element(package: &str, pf: &ProtoParsedFlag) -> FlagElement { @@ -148,7 +139,6 @@ fn create_flag_element(package: &str, pf: &ProtoParsedFlag) -> FlagElement { is_read_write: pf.permission() == ProtoFlagPermission::READ_WRITE, method_name: format_java_method_name(pf.name()), properties: format_property_name(pf.namespace()), - exported: pf.is_exported.unwrap_or(false), } } @@ -376,12 +366,12 @@ mod tests { #[test] fn test_generate_java_code_production() { let parsed_flags = crate::test::parse_test_flags(); - let generated_files = generate_java_code( - crate::test::TEST_PACKAGE, - parsed_flags.parsed_flag.into_iter(), - CodegenMode::Production, - ) - .unwrap(); + let mode = CodegenMode::Production; + let modified_parsed_flags = + crate::commands::modify_parsed_flags_based_on_mode(parsed_flags, mode).unwrap(); + let generated_files = + generate_java_code(crate::test::TEST_PACKAGE, modified_parsed_flags.into_iter(), mode) + .unwrap(); let expect_flags_content = EXPECTED_FLAG_COMMON_CONTENT.to_string() + r#" private static FeatureFlags FEATURE_FLAGS = new FeatureFlagsImpl(); @@ -534,12 +524,12 @@ mod tests { #[test] fn test_generate_java_code_exported() { let parsed_flags = crate::test::parse_test_flags(); - let generated_files = generate_java_code( - crate::test::TEST_PACKAGE, - parsed_flags.parsed_flag.into_iter(), - CodegenMode::Exported, - ) - .unwrap(); + let mode = CodegenMode::Exported; + let modified_parsed_flags = + crate::commands::modify_parsed_flags_based_on_mode(parsed_flags, mode).unwrap(); + let generated_files = + generate_java_code(crate::test::TEST_PACKAGE, modified_parsed_flags.into_iter(), mode) + .unwrap(); let expect_flags_content = r#" package com.android.aconfig.test; @@ -594,7 +584,6 @@ mod tests { /** @hide */ public final class FeatureFlagsImpl implements FeatureFlags { private static boolean aconfig_test_is_cached = false; - private static boolean other_namespace_is_cached = false; private static boolean disabledRwExported = false; private static boolean enabledFixedRoExported = false; private static boolean enabledRoExported = false; @@ -622,22 +611,6 @@ mod tests { aconfig_test_is_cached = true; } - private void load_overrides_other_namespace() { - try { - Properties properties = DeviceConfig.getProperties("other_namespace"); - } catch (NullPointerException e) { - throw new RuntimeException( - "Cannot read value from namespace other_namespace " - + "from DeviceConfig. It could be that the code using flag " - + "executed before SettingsProvider initialization. Please use " - + "fixed read-only flag by adding is_fixed_read_only: true in " - + "flag declaration.", - e - ); - } - other_namespace_is_cached = true; - } - @Override @UnsupportedAppUsage public boolean disabledRwExported() { @@ -751,12 +724,12 @@ mod tests { #[test] fn test_generate_java_code_test() { let parsed_flags = crate::test::parse_test_flags(); - let generated_files = generate_java_code( - crate::test::TEST_PACKAGE, - parsed_flags.parsed_flag.into_iter(), - CodegenMode::Test, - ) - .unwrap(); + let mode = CodegenMode::Test; + let modified_parsed_flags = + crate::commands::modify_parsed_flags_based_on_mode(parsed_flags, mode).unwrap(); + let generated_files = + generate_java_code(crate::test::TEST_PACKAGE, modified_parsed_flags.into_iter(), mode) + .unwrap(); let expect_flags_content = EXPECTED_FLAG_COMMON_CONTENT.to_string() + r#" diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index 7d9dd69354..cd53371eae 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -190,12 +190,12 @@ pub fn parse_flags( pub fn create_java_lib(mut input: Input, codegen_mode: CodegenMode) -> Result> { let parsed_flags = input.try_parse_flags()?; - let filtered_parsed_flags = filter_parsed_flags(parsed_flags, codegen_mode); - let Some(package) = find_unique_package(&filtered_parsed_flags) else { + let modified_parsed_flags = modify_parsed_flags_based_on_mode(parsed_flags, codegen_mode)?; + let Some(package) = find_unique_package(&modified_parsed_flags) else { bail!("no parsed flags, or the parsed flags use different packages"); }; let package = package.to_string(); - generate_java_code(&package, filtered_parsed_flags.into_iter(), codegen_mode) + generate_java_code(&package, modified_parsed_flags.into_iter(), codegen_mode) } pub fn create_cpp_lib(mut input: Input, codegen_mode: CodegenMode) -> Result> { @@ -316,18 +316,6 @@ fn find_unique_container(parsed_flags: &ProtoParsedFlags) -> Option<&str> { Some(container) } -fn filter_parsed_flags( - parsed_flags: ProtoParsedFlags, - codegen_mode: CodegenMode, -) -> Vec { - match codegen_mode { - CodegenMode::Exported => { - parsed_flags.parsed_flag.into_iter().filter(|pf| pf.is_exported()).collect() - } - _ => parsed_flags.parsed_flag, - } -} - pub fn modify_parsed_flags_based_on_mode( parsed_flags: ProtoParsedFlags, codegen_mode: CodegenMode, @@ -628,23 +616,6 @@ mod tests { ); } - #[test] - fn test_filter_parsed_flags() { - let mut input = parse_test_flags_as_input(); - let parsed_flags = input.try_parse_flags().unwrap(); - - let filtered_parsed_flags = - filter_parsed_flags(parsed_flags.clone(), CodegenMode::Exported); - assert_eq!(3, filtered_parsed_flags.len()); - - let filtered_parsed_flags = - filter_parsed_flags(parsed_flags.clone(), CodegenMode::Production); - assert_eq!(9, filtered_parsed_flags.len()); - - let filtered_parsed_flags = filter_parsed_flags(parsed_flags.clone(), CodegenMode::Test); - assert_eq!(9, filtered_parsed_flags.len()); - } - fn parse_test_flags_as_input() -> Input { let parsed_flags = crate::test::parse_test_flags(); let binary_proto = parsed_flags.write_to_bytes().unwrap(); diff --git a/tools/aconfig/templates/FakeFeatureFlagsImpl.java.template b/tools/aconfig/templates/FakeFeatureFlagsImpl.java.template index 8010b884c4..933d6a77cb 100644 --- a/tools/aconfig/templates/FakeFeatureFlagsImpl.java.template +++ b/tools/aconfig/templates/FakeFeatureFlagsImpl.java.template @@ -12,23 +12,11 @@ public class FakeFeatureFlagsImpl implements FeatureFlags \{ } {{ for item in flag_elements}} -{{ if library_exported }} - -{{ if item.exported }} @Override @UnsupportedAppUsage public boolean {item.method_name}() \{ return getValue(Flags.FLAG_{item.flag_name_constant_suffix}); } -{{ endif }} - -{{ else }} - @Override - @UnsupportedAppUsage - public boolean {item.method_name}() \{ - return getValue(Flags.FLAG_{item.flag_name_constant_suffix}); - } -{{ endif }} {{ endfor}} public void setFlag(String flagName, boolean value) \{ if (!this.mFlagMap.containsKey(flagName)) \{ @@ -52,20 +40,11 @@ public class FakeFeatureFlagsImpl implements FeatureFlags \{ } private Map mFlagMap = new HashMap<>( - {{ if library_exported }} - Map.ofEntries( - {{-for item in exported_flag_elements}} - Map.entry(Flags.FLAG_{item.flag_name_constant_suffix}, false) - {{ -if not @last }},{{ endif }} - {{ -endfor }} - ) - {{ else }} Map.ofEntries( {{-for item in flag_elements}} Map.entry(Flags.FLAG_{item.flag_name_constant_suffix}, false) {{ -if not @last }},{{ endif }} {{ -endfor }} ) - {{ endif }} ); } diff --git a/tools/aconfig/templates/FeatureFlags.java.template b/tools/aconfig/templates/FeatureFlags.java.template index 180f8828d2..d6af62cca8 100644 --- a/tools/aconfig/templates/FeatureFlags.java.template +++ b/tools/aconfig/templates/FeatureFlags.java.template @@ -6,14 +6,9 @@ import android.compat.annotation.UnsupportedAppUsage; public interface FeatureFlags \{ {{ for item in flag_elements }} {{ if library_exported }} - -{{ if item.exported }} @UnsupportedAppUsage boolean {item.method_name}(); -{{ endif }} - {{ else }} - {{ -if not item.is_read_write }} {{ -if item.default_value }} @com.android.aconfig.annotations.AssumeTrueForR8 @@ -23,7 +18,6 @@ public interface FeatureFlags \{ {{ endif }} @UnsupportedAppUsage boolean {item.method_name}(); - {{ endif }} {{ endfor }} } diff --git a/tools/aconfig/templates/FeatureFlagsImpl.java.template b/tools/aconfig/templates/FeatureFlagsImpl.java.template index 7a52ceb4ee..cf49c17298 100644 --- a/tools/aconfig/templates/FeatureFlagsImpl.java.template +++ b/tools/aconfig/templates/FeatureFlagsImpl.java.template @@ -15,12 +15,8 @@ public final class FeatureFlagsImpl implements FeatureFlags \{ {{ for flag in flag_elements }} {{ if library_exported }} -{{ if flag.exported }} private static boolean {flag.method_name} = false; -{{ endif }} - {{ else }} - {{- if flag.is_read_write }} private static boolean {flag.method_name} = {flag.default_value}; {{- endif- }} @@ -34,19 +30,13 @@ public final class FeatureFlagsImpl implements FeatureFlags \{ {{- for flag in namespace_with_flags.flags }} {{ if library_exported }} - - {{ if flag.exported }} {flag.method_name} = properties.getBoolean("{flag.device_config_flag}", false); - {{ endif }} - {{ else }} - {{ if flag.is_read_write }} {flag.method_name} = properties.getBoolean("{flag.device_config_flag}", {flag.default_value}); {{ endif }} - {{ endif }} {{ endfor }} } catch (NullPointerException e) \{ @@ -65,23 +55,15 @@ public final class FeatureFlagsImpl implements FeatureFlags \{ {{ endif- }} {{ for flag in flag_elements }} -{{ if library_exported }} - -{{ if flag.exported }} @Override @UnsupportedAppUsage public boolean {flag.method_name}() \{ + {{ -if library_exported }} if (!{flag.device_config_namespace}_is_cached) \{ load_overrides_{flag.device_config_namespace}(); } return {flag.method_name}; - } -{{ endif }} - -{{ else }} - @Override - @UnsupportedAppUsage - public boolean {flag.method_name}() \{ + {{ else }} {{ -if flag.is_read_write }} if (!{flag.device_config_namespace}_is_cached) \{ load_overrides_{flag.device_config_namespace}(); @@ -89,10 +71,9 @@ public final class FeatureFlagsImpl implements FeatureFlags \{ return {flag.method_name}; {{ else }} return {flag.default_value}; - {{ endif- }} + {{ -endif- }} + {{ -endif }} } -{{ endif }} - {{ endfor }} } {{ else }} diff --git a/tools/aconfig/templates/Flags.java.template b/tools/aconfig/templates/Flags.java.template index 9f4c52f978..ff942e5bfc 100644 --- a/tools/aconfig/templates/Flags.java.template +++ b/tools/aconfig/templates/Flags.java.template @@ -6,28 +6,16 @@ import android.compat.annotation.UnsupportedAppUsage; /** @hide */ public final class Flags \{ {{- for item in flag_elements}} - {{ if library_exported }} - {{ if item.exported }} /** @hide */ public static final String FLAG_{item.flag_name_constant_suffix} = "{item.device_config_flag}"; - {{ endif }} - {{ else }} - /** @hide */ - public static final String FLAG_{item.flag_name_constant_suffix} = "{item.device_config_flag}"; - {{ endif }} {{- endfor }} {{ for item in flag_elements}} -{{ if library_exported }} - -{{ if item.exported }} +{{ -if library_exported }} @UnsupportedAppUsage public static boolean {item.method_name}() \{ return FEATURE_FLAGS.{item.method_name}(); } -{{ endif }} - -{{ else }} - +{{ -else }} {{ -if not item.is_read_write }} {{ -if item.default_value }} @com.android.aconfig.annotations.AssumeTrueForR8