Skip to content

Commit

Permalink
Revert^4 "Implement detecting container violations."
Browse files Browse the repository at this point in the history
This change introduces a method to detect violating inter-container
dependencies between modules. The method is run in
`ModuleBase.GenerateBuildActions`, after the container info provider is
set. Given that the provider of the direct dependencies would have been
set at this time, the method utilizes this information to determine
the violations, which are introduced in https://r.android.com/3141104.

Note that this enforcement does not turn all inter-container
dependencies as errors. Instead, it will only turn dependencies that
matches the pre-defined violations into errors. Even if the dependency
matches the violation, an error will not be thrown if the dependency
satisfies any of the exception functions (e.g. the dependent module is
stubs, or the two modules belong to the same apexes).

Test: m nothing --no-skip-soong-tests
Bug: 338660802
Change-Id: Ib9ddc0761fa46f1309b1a1a4f759d9a4e04fd70e
  • Loading branch information
jihoonkang0829 committed Aug 29, 2024
1 parent a1527e5 commit 85bc193
Show file tree
Hide file tree
Showing 17 changed files with 225 additions and 19 deletions.
12 changes: 12 additions & 0 deletions android/apex.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package android

import (
"fmt"
"reflect"
"slices"
"sort"
"strconv"
Expand Down Expand Up @@ -145,6 +146,17 @@ func (i ApexInfo) InApexModule(apexModuleName string) bool {
return false
}

// To satisfy the comparable interface
func (i ApexInfo) Equal(other any) bool {
otherApexInfo, ok := other.(ApexInfo)
return ok && i.ApexVariationName == otherApexInfo.ApexVariationName &&
i.MinSdkVersion == otherApexInfo.MinSdkVersion &&
i.Updatable == otherApexInfo.Updatable &&
i.UsePlatformApis == otherApexInfo.UsePlatformApis &&
reflect.DeepEqual(i.InApexVariants, otherApexInfo.InApexVariants) &&
reflect.DeepEqual(i.InApexModules, otherApexInfo.InApexModules)
}

// ApexTestForInfo stores the contents of APEXes for which this module is a test - although this
// module is not part of the APEX - and thus has access to APEX internals.
type ApexTestForInfo struct {
Expand Down
65 changes: 65 additions & 0 deletions android/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
package android

import (
"fmt"
"reflect"
"slices"
"strings"

"github.com/google/blueprint"
)
Expand Down Expand Up @@ -395,6 +397,40 @@ func (c *ContainersInfo) UpdatableApex() bool {

var ContainersInfoProvider = blueprint.NewProvider[ContainersInfo]()

func satisfyAllowedExceptions(ctx ModuleContext, allowedExceptionLabels []exceptionHandleFuncLabel, m, dep Module) bool {
for _, label := range allowedExceptionLabels {
if exceptionHandleFunctionsTable[label](ctx, m, dep) {
return true
}
}
return false
}

func (c *ContainersInfo) GetViolations(mctx ModuleContext, m, dep Module, depInfo ContainersInfo) []string {
var violations []string

// Any containers that the module belongs to but the dependency does not belong to must be examined.
_, containersUniqueToModule, _ := ListSetDifference(c.belongingContainers, depInfo.belongingContainers)

// Apex container should be examined even if both the module and the dependency belong to
// the apex container to check that the two modules belong to the same apex.
if InList(ApexContainer, c.belongingContainers) && !InList(ApexContainer, containersUniqueToModule) {
containersUniqueToModule = append(containersUniqueToModule, ApexContainer)
}

for _, containerUniqueToModule := range containersUniqueToModule {
for _, restriction := range containerUniqueToModule.restricted {
if InList(restriction.dependency, depInfo.belongingContainers) {
if !satisfyAllowedExceptions(mctx, restriction.allowedExceptions, m, dep) {
violations = append(violations, restriction.errorMessage)
}
}
}
}

return violations
}

func generateContainerInfo(ctx ModuleContext) ContainersInfo {
var containers []*container

Expand Down Expand Up @@ -436,3 +472,32 @@ func setContainerInfo(ctx ModuleContext) {
SetProvider(ctx, ContainersInfoProvider, containersInfo)
}
}

func checkContainerViolations(ctx ModuleContext) {
if _, ok := ctx.Module().(InstallableModule); ok {
containersInfo, _ := getContainerModuleInfo(ctx, ctx.Module())
ctx.VisitDirectDepsIgnoreBlueprint(func(dep Module) {
if !dep.Enabled(ctx) {
return
}

// Pre-existing violating dependencies are tracked in containerDependencyViolationAllowlist.
// If this dependency is allowlisted, do not check for violation.
// If not, check if this dependency matches any restricted dependency and
// satisfies any exception functions, which allows bypassing the
// restriction. If all of the exceptions are not satisfied, throw an error.
if depContainersInfo, ok := getContainerModuleInfo(ctx, dep); ok {
if allowedViolations, ok := ContainerDependencyViolationAllowlist[ctx.ModuleName()]; ok && InList(dep.Name(), allowedViolations) {
return
} else {
violations := containersInfo.GetViolations(ctx, ctx.Module(), dep, depContainersInfo)
if len(violations) > 0 {
errorMessage := fmt.Sprintf("%s cannot depend on %s. ", ctx.ModuleName(), dep.Name())
errorMessage += strings.Join(violations, " ")
ctx.ModuleErrorf(errorMessage)
}
}
}
})
}
}
3 changes: 3 additions & 0 deletions android/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -1778,6 +1778,9 @@ func (m *ModuleBase) GenerateBuildActions(blueprintCtx blueprint.ModuleContext)
}

setContainerInfo(ctx)
if ctx.Config().Getenv("DISABLE_CONTAINER_CHECK") != "true" {
checkContainerViolations(ctx)
}

ctx.licenseMetadataFile = PathForModuleOut(ctx, "meta_lic")

Expand Down
12 changes: 12 additions & 0 deletions apex/aconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ func TestValidationAcrossContainersExportedPass(t *testing.T) {
apex_available: [
"myapex",
],
sdk_version: "none",
system_modules: "none",
}`,
},
{
Expand Down Expand Up @@ -122,6 +124,8 @@ func TestValidationAcrossContainersExportedPass(t *testing.T) {
apex_available: [
"myapex",
],
sdk_version: "none",
system_modules: "none",
}`,
},
{
Expand Down Expand Up @@ -345,6 +349,8 @@ func TestValidationAcrossContainersNotExportedFail(t *testing.T) {
apex_available: [
"myapex",
],
sdk_version: "none",
system_modules: "none",
}`,
expectedError: `.*my_java_library_foo/myapex depends on my_java_aconfig_library_foo/otherapex/production across containers`,
},
Expand Down Expand Up @@ -392,6 +398,8 @@ func TestValidationAcrossContainersNotExportedFail(t *testing.T) {
apex_available: [
"myapex",
],
sdk_version: "none",
system_modules: "none",
}`,
expectedError: `.*my_android_app_foo/myapex depends on my_java_aconfig_library_foo/otherapex/production across containers`,
},
Expand Down Expand Up @@ -693,6 +701,8 @@ func TestValidationAcrossContainersNotExportedFail(t *testing.T) {
apex_available: [
"myapex",
],
sdk_version: "none",
system_modules: "none",
}`,
expectedError: `.*my_android_app_foo/myapex depends on my_java_aconfig_library_foo/otherapex/production across containers`,
},
Expand Down Expand Up @@ -769,6 +779,8 @@ func TestValidationNotPropagateAcrossShared(t *testing.T) {
apex_available: [
"myapex",
],
sdk_version: "none",
system_modules: "none",
}`,
},
}
Expand Down
Loading

0 comments on commit 85bc193

Please sign in to comment.