From ad913d56ac197b9f6ed4ec0874c901ebf4f198cb Mon Sep 17 00:00:00 2001 From: Greg Kaiser Date: Fri, 8 May 2020 19:12:34 +0000 Subject: [PATCH 1/3] Revert "Fix snapshot of a host/device cc_library with stubs" Revert submission 1302576 Bug: 156054601 Reason for revert: Presumed root cause of build break. Reverted Changes: Ifc8116e11:Detect invalid arch specific properties in snapsho... I7ebd33307:Adds support for 'ignored-on-host' I167b47a13:Fix snapshot of a host/device cc_library with stub... Change-Id: Ibccce5286605bb71c6be3b3550ba86d8b7e24fa7 --- cc/library_sdk_member.go | 6 +-- sdk/cc_sdk_test.go | 83 ---------------------------------------- sdk/update.go | 7 ---- 3 files changed, 1 insertion(+), 95 deletions(-) diff --git a/cc/library_sdk_member.go b/cc/library_sdk_member.go index a7a1de251..730012c5b 100644 --- a/cc/library_sdk_member.go +++ b/cc/library_sdk_member.go @@ -342,11 +342,7 @@ type nativeLibInfoProperties struct { // The specific stubs version for the lib variant, or empty string if stubs // are not in use. - // - // Marked 'ignored-on-host' as the StubsVersion() from which this is initialized is - // not set on host and the stubs.versions property which this is written to is does - // not vary by arch so cannot be android specific. - StubsVersion string `sdk:"ignored-on-host"` + StubsVersion string // outputFile is not exported as it is always arch specific. outputFile android.Path diff --git a/sdk/cc_sdk_test.go b/sdk/cc_sdk_test.go index b77447aea..733f7ac22 100644 --- a/sdk/cc_sdk_test.go +++ b/sdk/cc_sdk_test.go @@ -1805,86 +1805,3 @@ sdk_snapshot { } `)) } - -func TestDeviceAndHostSnapshotWithStubsLibrary(t *testing.T) { - result := testSdkWithCc(t, ` - sdk { - name: "mysdk", - host_supported: true, - native_shared_libs: ["stubslib"], - } - - cc_library { - name: "internaldep", - host_supported: true, - } - - cc_library { - name: "stubslib", - host_supported: true, - shared_libs: ["internaldep"], - stubs: { - symbol_file: "some/where/stubslib.map.txt", - versions: ["1", "2", "3"], - }, - } - `) - - result.CheckSnapshot("mysdk", "", - checkAndroidBpContents(` -// This is auto-generated. DO NOT EDIT. - -cc_prebuilt_library_shared { - name: "mysdk_stubslib@current", - sdk_member_name: "stubslib", - host_supported: true, - installable: false, - stubs: { - versions: ["3"], - }, - target: { - android_arm64: { - srcs: ["android/arm64/lib/stubslib.so"], - }, - android_arm: { - srcs: ["android/arm/lib/stubslib.so"], - }, - linux_glibc_x86_64: { - srcs: ["linux_glibc/x86_64/lib/stubslib.so"], - }, - linux_glibc_x86: { - srcs: ["linux_glibc/x86/lib/stubslib.so"], - }, - }, -} - -cc_prebuilt_library_shared { - name: "stubslib", - prefer: false, - host_supported: true, - stubs: { - versions: ["3"], - }, - target: { - android_arm64: { - srcs: ["android/arm64/lib/stubslib.so"], - }, - android_arm: { - srcs: ["android/arm/lib/stubslib.so"], - }, - linux_glibc_x86_64: { - srcs: ["linux_glibc/x86_64/lib/stubslib.so"], - }, - linux_glibc_x86: { - srcs: ["linux_glibc/x86/lib/stubslib.so"], - }, - }, -} - -sdk_snapshot { - name: "mysdk@current", - host_supported: true, - native_shared_libs: ["mysdk_stubslib@current"], -} -`)) -} diff --git a/sdk/update.go b/sdk/update.go index d43a42d6e..991428eec 100644 --- a/sdk/update.go +++ b/sdk/update.go @@ -982,13 +982,6 @@ func (osInfo *osTypeSpecificInfo) addToPropertySet(ctx *memberContext, bpModule } } -func (osInfo *osTypeSpecificInfo) isHostVariant() bool { - osClass := osInfo.osType.Class - return osClass == android.Host || osClass == android.HostCross -} - -var _ isHostVariant = (*osTypeSpecificInfo)(nil) - func (osInfo *osTypeSpecificInfo) String() string { return fmt.Sprintf("OsType{%s}", osInfo.osType) } From e08e03fa965f6deac48f7ac164b3f9ff507f100d Mon Sep 17 00:00:00 2001 From: Greg Kaiser Date: Fri, 8 May 2020 19:12:34 +0000 Subject: [PATCH 2/3] Revert "Adds support for 'ignored-on-host'" Revert submission 1302576 Bug: 156054601 Reason for revert: Presumed root cause of build break. Reverted Changes: Ifc8116e11:Detect invalid arch specific properties in snapsho... I7ebd33307:Adds support for 'ignored-on-host' I167b47a13:Fix snapshot of a host/device cc_library with stub... Change-Id: I2a7ac0ef0232177eefc26542c11dc675d6f4cab2 --- android/sdk.go | 9 --------- sdk/sdk_test.go | 5 +---- sdk/update.go | 45 --------------------------------------------- 3 files changed, 1 insertion(+), 58 deletions(-) diff --git a/android/sdk.go b/android/sdk.go index e823106e8..36afc3df2 100644 --- a/android/sdk.go +++ b/android/sdk.go @@ -351,15 +351,6 @@ type SdkMemberType interface { // values that differ by arch, fields not tagged as such must have common values across // all variants. // - // * Additional field tags can be specified on a field that will ignore certain values - // for the purpose of common value optimization. A value that is ignored must have the - // default value for the property type. This is to ensure that significant value are not - // ignored by accident. The purpose of this is to allow the snapshot generation to reflect - // the behavior of the runtime. e.g. if a property is ignored on the host then a property - // that is common for android can be treated as if it was common for android and host as - // the setting for host is ignored anyway. - // * `sdk:"ignored-on-host" - this indicates the property is ignored on the host variant. - // // * The sdk module type populates the BpModule structure, creating the arch specific // structure and calls AddToPropertySet(...) on the properties struct to add the member // specific properties in the correct place in the structure. diff --git a/sdk/sdk_test.go b/sdk/sdk_test.go index ae1a4923a..898ecea68 100644 --- a/sdk/sdk_test.go +++ b/sdk/sdk_test.go @@ -289,12 +289,9 @@ func TestCommonValueOptimization(t *testing.T) { } extractor := newCommonValueExtractor(common) + extractor.extractCommonProperties(common, structs) h := TestHelper{t} - - err := extractor.extractCommonProperties(common, structs) - h.AssertDeepEquals("unexpected error", nil, err) - h.AssertDeepEquals("common properties not correct", &testPropertiesStruct{ name: "common", diff --git a/sdk/update.go b/sdk/update.go index 991428eec..bcc2c778b 100644 --- a/sdk/update.go +++ b/sdk/update.go @@ -1215,26 +1215,11 @@ func (s *sdk) getPossibleOsTypes() []android.OsType { // struct (or one of its embedded structs). type fieldAccessorFunc func(structValue reflect.Value) reflect.Value -// Checks the metadata to determine whether the property should be ignored for the -// purposes of common value extraction or not. -type extractorMetadataPredicate func(metadata propertiesContainer) bool - -// Indicates whether optimizable properties are provided by a host variant or -// not. -type isHostVariant interface { - isHostVariant() bool -} - // A property that can be optimized by the commonValueExtractor. type extractorProperty struct { // The name of the field for this property. name string - // Filter that can use metadata associated with the properties being optimized - // to determine whether the field should be ignored during common value - // optimization. - filter extractorMetadataPredicate - // Retrieves the value on which common value optimization will be performed. getter fieldAccessorFunc @@ -1288,20 +1273,6 @@ func (e *commonValueExtractor) gatherFields(structType reflect.Type, containingS continue } - var filter extractorMetadataPredicate - - // Add a filter - if proptools.HasTag(field, "sdk", "ignored-on-host") { - filter = func(metadata propertiesContainer) bool { - if m, ok := metadata.(isHostVariant); ok { - if m.isHostVariant() { - return false - } - } - return true - } - } - // Save a copy of the field index for use in the function. fieldIndex := f @@ -1333,7 +1304,6 @@ func (e *commonValueExtractor) gatherFields(structType reflect.Type, containingS } else { property := extractorProperty{ name, - filter, fieldGetter, reflect.Zero(field.Type), proptools.HasTag(field, "android", "arch_variant"), @@ -1402,12 +1372,6 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac for _, property := range e.properties { fieldGetter := property.getter - filter := property.filter - if filter == nil { - filter = func(metadata propertiesContainer) bool { - return true - } - } // Check to see if all the structures have the same value for the field. The commonValue // is nil on entry to the loop and if it is nil on exit then there is no common value or @@ -1425,15 +1389,6 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac itemValue := reflect.ValueOf(container.optimizableProperties()) fieldValue := fieldGetter(itemValue) - if !filter(container) { - expectedValue := property.emptyValue.Interface() - actualValue := fieldValue.Interface() - if !reflect.DeepEqual(expectedValue, actualValue) { - return fmt.Errorf("field %q is supposed to be ignored for %q but is set to %#v instead of %#v", property, container, actualValue, expectedValue) - } - continue - } - if commonValue == nil { // Use the first value as the commonProperties value. commonValue = &fieldValue From e509447c9348f5dbfc29393665d74caab9ad22c6 Mon Sep 17 00:00:00 2001 From: Greg Kaiser Date: Fri, 8 May 2020 19:12:34 +0000 Subject: [PATCH 3/3] Revert "Detect invalid arch specific properties in snapshot" Revert submission 1302576 Bug: 156054601 Reason for revert: Presumed root cause of build break. Reverted Changes: Ifc8116e11:Detect invalid arch specific properties in snapsho... I7ebd33307:Adds support for 'ignored-on-host' I167b47a13:Fix snapshot of a host/device cc_library with stub... Change-Id: Id7eba0bdde5c579e10e9b42d94a7cfab5f34995f --- android/sdk.go | 8 +------- cc/library_sdk_member.go | 10 +++++----- java/java.go | 2 +- sdk/sdk_test.go | 33 +++++---------------------------- sdk/update.go | 32 +++----------------------------- 5 files changed, 15 insertions(+), 70 deletions(-) diff --git a/android/sdk.go b/android/sdk.go index 36afc3df2..873e08942 100644 --- a/android/sdk.go +++ b/android/sdk.go @@ -342,15 +342,9 @@ type SdkMemberType interface { // // * The variant property structs are analysed to find exported (capitalized) fields which // have common values. Those fields are cleared and the common value added to the common - // properties. - // - // A field annotated with a tag of `sdk:"keep"` will be treated as if it + // properties. A field annotated with a tag of `sdk:"keep"` will be treated as if it // was not capitalized, i.e. not optimized for common values. // - // A field annotated with a tag of `android:"arch_variant"` will be allowed to have - // values that differ by arch, fields not tagged as such must have common values across - // all variants. - // // * The sdk module type populates the BpModule structure, creating the arch specific // structure and calls AddToPropertySet(...) on the properties struct to add the member // specific properties in the correct place in the structure. diff --git a/cc/library_sdk_member.go b/cc/library_sdk_member.go index 730012c5b..2c8e31158 100644 --- a/cc/library_sdk_member.go +++ b/cc/library_sdk_member.go @@ -307,7 +307,7 @@ type nativeLibInfoProperties struct { // The list of possibly common exported include dirs. // // This field is exported as its contents may not be arch specific. - ExportedIncludeDirs android.Paths `android:"arch_variant"` + ExportedIncludeDirs android.Paths // The list of arch specific exported generated include dirs. // @@ -322,23 +322,23 @@ type nativeLibInfoProperties struct { // The list of possibly common exported system include dirs. // // This field is exported as its contents may not be arch specific. - ExportedSystemIncludeDirs android.Paths `android:"arch_variant"` + ExportedSystemIncludeDirs android.Paths // The list of possibly common exported flags. // // This field is exported as its contents may not be arch specific. - ExportedFlags []string `android:"arch_variant"` + ExportedFlags []string // The set of shared libraries // // This field is exported as its contents may not be arch specific. - SharedLibs []string `android:"arch_variant"` + SharedLibs []string // The set of system shared libraries. Note nil and [] are semantically // distinct - see BaseLinkerProperties.System_shared_libs. // // This field is exported as its contents may not be arch specific. - SystemSharedLibs []string `android:"arch_variant"` + SystemSharedLibs []string // The specific stubs version for the lib variant, or empty string if stubs // are not in use. diff --git a/java/java.go b/java/java.go index 9d75c74c7..472d3dac4 100644 --- a/java/java.go +++ b/java/java.go @@ -1906,7 +1906,7 @@ func (mt *librarySdkMemberType) CreateVariantPropertiesStruct() android.SdkMembe type librarySdkMemberProperties struct { android.SdkMemberPropertiesBase - JarToExport android.Path `android:"arch_variant"` + JarToExport android.Path AidlIncludeDirs android.Paths } diff --git a/sdk/sdk_test.go b/sdk/sdk_test.go index 898ecea68..095f83607 100644 --- a/sdk/sdk_test.go +++ b/sdk/sdk_test.go @@ -226,8 +226,8 @@ func TestSDkInstall(t *testing.T) { } type EmbeddedPropertiesStruct struct { - S_Embedded_Common string `android:"arch_variant"` - S_Embedded_Different string `android:"arch_variant"` + S_Embedded_Common string + S_Embedded_Different string } type testPropertiesStruct struct { @@ -235,11 +235,11 @@ type testPropertiesStruct struct { private string Public_Kept string `sdk:"keep"` S_Common string - S_Different string `android:"arch_variant"` + S_Different string A_Common []string - A_Different []string `android:"arch_variant"` + A_Different []string F_Common *bool - F_Different *bool `android:"arch_variant"` + F_Different *bool EmbeddedPropertiesStruct } @@ -346,26 +346,3 @@ func TestCommonValueOptimization(t *testing.T) { }, structs[1]) } - -func TestCommonValueOptimization_InvalidArchSpecificVariants(t *testing.T) { - common := &testPropertiesStruct{name: "common"} - structs := []propertiesContainer{ - &testPropertiesStruct{ - name: "struct-0", - S_Common: "should-be-but-is-not-common0", - }, - &testPropertiesStruct{ - name: "struct-1", - S_Common: "should-be-but-is-not-common1", - }, - } - - extractor := newCommonValueExtractor(common) - - h := TestHelper{t} - - err := extractor.extractCommonProperties(common, structs) - h.AssertErrorMessageEquals("unexpected error", `field "S_Common" is not tagged as "arch_variant" but has arch specific properties: - "struct-0" has value "should-be-but-is-not-common0" - "struct-1" has value "should-be-but-is-not-common1"`, err) -} diff --git a/sdk/update.go b/sdk/update.go index bcc2c778b..03a5c0379 100644 --- a/sdk/update.go +++ b/sdk/update.go @@ -1225,9 +1225,6 @@ type extractorProperty struct { // The empty value for the field. emptyValue reflect.Value - - // True if the property can support arch variants false otherwise. - archVariant bool } func (p extractorProperty) String() string { @@ -1306,7 +1303,6 @@ func (e *commonValueExtractor) gatherFields(structType reflect.Type, containingS name, fieldGetter, reflect.Zero(field.Type), - proptools.HasTag(field, "android", "arch_variant"), } e.properties = append(e.properties, property) } @@ -1374,16 +1370,10 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac fieldGetter := property.getter // Check to see if all the structures have the same value for the field. The commonValue - // is nil on entry to the loop and if it is nil on exit then there is no common value or - // all the values have been filtered out, otherwise it points to the common value. + // is nil on entry to the loop and if it is nil on exit then there is no common value, + // otherwise it points to the common value. var commonValue *reflect.Value - // Assume that all the values will be the same. - // - // While similar to this is not quite the same as commonValue == nil. If all the values - // have been filtered out then this will be false but commonValue == nil will be true. - valuesDiffer := false - for i := 0; i < sliceValue.Len(); i++ { container := sliceValue.Index(i).Interface().(propertiesContainer) itemValue := reflect.ValueOf(container.optimizableProperties()) @@ -1397,13 +1387,12 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac // no value in common so break out. if !reflect.DeepEqual(fieldValue.Interface(), commonValue.Interface()) { commonValue = nil - valuesDiffer = true break } } } - // If the fields all have common value then store it in the common struct field + // If the fields all have a common value then store it in the common struct field // and set the input struct's field to the empty value. if commonValue != nil { emptyValue := property.emptyValue @@ -1415,21 +1404,6 @@ func (e *commonValueExtractor) extractCommonProperties(commonProperties interfac fieldValue.Set(emptyValue) } } - - if valuesDiffer && !property.archVariant { - // The values differ but the property does not support arch variants so it - // is an error. - var details strings.Builder - for i := 0; i < sliceValue.Len(); i++ { - container := sliceValue.Index(i).Interface().(propertiesContainer) - itemValue := reflect.ValueOf(container.optimizableProperties()) - fieldValue := fieldGetter(itemValue) - - _, _ = fmt.Fprintf(&details, "\n %q has value %q", container.String(), fieldValue.Interface()) - } - - return fmt.Errorf("field %q is not tagged as \"arch_variant\" but has arch specific properties:%s", property.String(), details.String()) - } } return nil