StrixOS/build_soong
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Differentiate between no dist tag and an empty dist tag

Browse Source 
Change https://r.android.com/1335521 added tag property to the Dist
struct so that it could be used to select one of a number of different
output files to copy to the dist instead of the single file that the
module type made available for dist. The output files were selected
by passing the tag to OutputFiles(tag).

Module types that wanted to support this new approach had to explicitly
set AndroidMkEntries.DistFiles = GenerateTaggedDistFiles(module).
Unfortunately, doing that had a side effect of changing the behavior of
dist entries without a tag.

That was because the change treated a tag that was not specified, as
being the same as "". So, prior to the change no tag meant use the
default dist file but after it meant use the paths returned by
OutputFiles(""). That changed the behavior of the java.Library type
which affected the behavior of the android_app module type.

Prior to the change the java_library would make the
Library.outputFile available for dist when no tag was specified. After
that change it would make Library.outputFile plus
Library.extraOutputFiles. The latter is usually empty except for
android_app which adds some extra files into there which will now be
copied to the dist. That change may have been intentional but there
was no mention of it in the change or the bug. Even if it wasn't
intentional it may still be beneficial.

Any module type that wants to add support for tags in dist runs the
risk of introducing similar changes in behavior. This change
differentiates between the tag not being set and the tag being set to
"" to avoid that possibility and to make the default behavior
explicit for those module types that have switched.

It does so as follows:
* Adds a DefaultDistTag constant that is used when the tag is not set.
  It is a string that is unlikely to be used as an actual tag as it
  does not start with a . and uses some special characters.
* The DefaultDistTag is used in MakeDefaultDistFiles(paths) to indicate
  that the supplied paths are the default ones and and also in
  GenerateTaggedDistFiles() for Dist structures that have no tag
  property set.
* The DefaultDistTag is passed to OutputFiles(tag) just in case the
  module type has explicitly defined the paths to associate with that
  tag in there. If it has then it overrides the legacy behavior. If it
  has not then it is just ignored and falls back to using the previous
  behavior.
* The java.Library.OutputFiles(tag) method explicitly handles the
  DefaultDistTag and returns Library.outputFile for it which restores
  the behavior from before the change that added dist.tag support.
* Similar change was made to apexBundle.OutputFiles(tag) in order to
  preserve its previous behaviour.
* The customModule used by TestGetDistContributions has been modified
  to also preserve its previous behavior after this change.

Test: m nothing
      m dist sdk - before and after this change, compare result to
      make sure that there are no significant differences.
      Test the effect on the apex by following instructions in
      http://b/172951145
Bug: 174226317
Change-Id: Ib8f0d9307751cc2ed34e3d9a5538d3c144666f6d
This commit is contained in:
Paul Duffin
2020-11-25 16:37:46 +00:00
parent 62269498be
commit 74f05598eb
5 changed files with 111 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
android/androidmk.go
View File

@@ -222,13 +222,23 @@ func (a *AndroidMkEntries) getDistContributions(mod blueprint.Module) *distContr
amod := mod.(Module).base()
name := amod.BaseModuleName()
// Collate the set of associated tag/paths available for copying to the dist.
// Start with an empty (nil) set.
var availableTaggedDists TaggedDistFiles
// Then merge in any that are provided explicitly by the module.
if a.DistFiles != nil {
availableTaggedDists = a.DistFiles
} else if a.OutputFile.Valid() {
availableTaggedDists = MakeDefaultDistFiles(a.OutputFile.Path())
} else {
// Merge the DistFiles into the set.
availableTaggedDists = availableTaggedDists.merge(a.DistFiles)
}
// If no paths have been provided for the DefaultDistTag and the output file is
// valid then add that as the default dist path.
if _, ok := availableTaggedDists[DefaultDistTag]; !ok && a.OutputFile.Valid() {
availableTaggedDists = availableTaggedDists.addPathsForTag(DefaultDistTag, a.OutputFile.Path())
}
if len(availableTaggedDists) == 0 {
// Nothing dist-able for this module.
return nil
}
@@ -245,7 +255,7 @@ func (a *AndroidMkEntries) getDistContributions(mod blueprint.Module) *distContr
var tag string
if dist.Tag == nil {
// If the dist struct does not specify a tag, use the default output files tag.
tag = ""
tag = DefaultDistTag
} else {
tag = *dist.Tag
}
@@ -259,10 +269,10 @@ func (a *AndroidMkEntries) getDistContributions(mod blueprint.Module) *distContr
}
if len(tagPaths) > 1 && (dist.Dest != nil || dist.Suffix != nil) {
errorMessage := "Cannot apply dest/suffix for more than one dist " +
"file for %s goals in module %s. The list of dist files, " +
errorMessage := "%s: Cannot apply dest/suffix for more than one dist " +
"file for %q goals tag %q in module %s. The list of dist files, " +
"which should have a single element, is:\n%s"
panic(fmt.Errorf(errorMessage, goals, name, tagPaths))
panic(fmt.Errorf(errorMessage, mod, goals, tag, name, tagPaths))
}
copiesForGoals := distContributions.getCopiesForGoals(goals)

34
android/androidmk_test.go
View File

@@ -35,6 +35,10 @@ type customModule struct {
data AndroidMkData
distFiles TaggedDistFiles
outputFile OptionalPath
// The paths that will be used as the default dist paths if no tag is
// specified.
defaultDistPaths Paths
}
const (
@@ -48,7 +52,14 @@ func (m *customModule) GenerateAndroidBuildActions(ctx ModuleContext) {
// If the dist_output_file: true then create an output file that is stored in
// the OutputFile property of the AndroidMkEntry.
if proptools.BoolDefault(m.properties.Dist_output_file, true) {
m.outputFile = OptionalPathForPath(PathForTesting("dist-output-file.out"))
path := PathForTesting("dist-output-file.out")
m.outputFile = OptionalPathForPath(path)
// Previous code would prioritize the DistFiles property over the OutputFile
// property in AndroidMkEntry when determining the default dist paths.
// Setting this first allows it to be overridden based on the
// default_dist_files setting replicating that previous behavior.
m.defaultDistPaths = Paths{path}
}
// Based on the setting of the default_dist_files property possibly create a
@@ -60,9 +71,22 @@ func (m *customModule) GenerateAndroidBuildActions(ctx ModuleContext) {
// Do nothing
case defaultDistFiles_Default:
m.distFiles = MakeDefaultDistFiles(PathForTesting("default-dist.out"))
path := PathForTesting("default-dist.out")
m.defaultDistPaths = Paths{path}
m.distFiles = MakeDefaultDistFiles(path)
case defaultDistFiles_Tagged:
// Module types that set AndroidMkEntry.DistFiles to the result of calling
// GenerateTaggedDistFiles(ctx) relied on no tag being treated as "" which
// meant that the default dist paths would be whatever was returned by
// OutputFiles(""). In order to preserve that behavior when treating no tag
// as being equal to DefaultDistTag this ensures that
// OutputFiles(DefaultDistTag) will return the same as OutputFiles("").
m.defaultDistPaths = PathsForTesting("one.out")
// This must be called after setting defaultDistPaths/outputFile as
// GenerateTaggedDistFiles calls into OutputFiles(tag) which may use those
// fields.
m.distFiles = m.GenerateTaggedDistFiles(ctx)
}
}
@@ -77,6 +101,12 @@ func (m *customModule) AndroidMk() AndroidMkData {
func (m *customModule) OutputFiles(tag string) (Paths, error) {
switch tag {
case DefaultDistTag:
if m.defaultDistPaths != nil {
return m.defaultDistPaths, nil
} else {
return nil, fmt.Errorf("default dist tag is not available")
}
case "":
return PathsForTesting("one.out"), nil
case ".multiple":

74
android/module.go
View File

@@ -503,8 +503,12 @@ type Dist struct {
// A suffix to add to the artifact file name (before any extension).
Suffix *string `android:"arch_variant"`
// A string tag to select the OutputFiles associated with the tag. Defaults to the
// the empty "" string.
// A string tag to select the OutputFiles associated with the tag.
//
// If no tag is specified then it will select the default dist paths provided
// by the module type. If a tag of "" is specified then it will return the
// default output files provided by the modules, i.e. the result of calling
// OutputFiles("").
Tag *string `android:"arch_variant"`
}
@@ -733,9 +737,45 @@ type distProperties struct {
Dists []Dist `android:"arch_variant"`
}
// The key to use in TaggedDistFiles when a Dist structure does not specify a
// tag property. This intentionally does not use "" as the default because that
// would mean that an empty tag would have a different meaning when used in a dist
// structure that when used to reference a specific set of output paths using the
// :module{tag} syntax, which passes tag to the OutputFiles(tag) method.
const DefaultDistTag = "<default-dist-tag>"
// A map of OutputFile tag keys to Paths, for disting purposes.
type TaggedDistFiles map[string]Paths
// addPathsForTag adds a mapping from the tag to the paths. If the map is nil
// then it will create a map, update it and then return it. If a mapping already
// exists for the tag then the paths are appended to the end of the current list
// of paths, ignoring any duplicates.
func (t TaggedDistFiles) addPathsForTag(tag string, paths ...Path) TaggedDistFiles {
if t == nil {
t = make(TaggedDistFiles)
}
for _, distFile := range paths {
if distFile != nil && !t[tag].containsPath(distFile) {
t[tag] = append(t[tag], distFile)
}
}
return t
}
// merge merges the entries from the other TaggedDistFiles object into this one.
// If the TaggedDistFiles is nil then it will create a new instance, merge the
// other into it, and then return it.
func (t TaggedDistFiles) merge(other TaggedDistFiles) TaggedDistFiles {
for tag, paths := range other {
t = t.addPathsForTag(tag, paths...)
}
return t
}
func MakeDefaultDistFiles(paths ...Path) TaggedDistFiles {
for _, path := range paths {
if path == nil {
@@ -744,7 +784,7 @@ func MakeDefaultDistFiles(paths ...Path) TaggedDistFiles {
}
// The default OutputFile tag is the empty "" string.
return TaggedDistFiles{"": paths}
return TaggedDistFiles{DefaultDistTag: paths}
}
type hostAndDeviceProperties struct {
@@ -1071,24 +1111,24 @@ func (m *ModuleBase) Dists() []Dist {
}
func (m *ModuleBase) GenerateTaggedDistFiles(ctx BaseModuleContext) TaggedDistFiles {
distFiles := make(TaggedDistFiles)
var distFiles TaggedDistFiles
for _, dist := range m.Dists() {
var tag string
var distFilesForTag Paths
if dist.Tag == nil {
tag = ""
} else {
tag = *dist.Tag
}
// If no tag is specified then it means to use the default dist paths so use
// the special tag name which represents that.
tag := proptools.StringDefault(dist.Tag, DefaultDistTag)
// Call the OutputFiles(tag) method to get the paths associated with the tag.
distFilesForTag, err := m.base().module.(OutputFileProducer).OutputFiles(tag)
if err != nil {
// If the tag was not supported and is not DefaultDistTag then it is an error.
// Failing to find paths for DefaultDistTag is not an error. It just means
// that the module type requires the legacy behavior.
if err != nil && tag != DefaultDistTag {
ctx.PropertyErrorf("dist.tag", "%s", err.Error())
continue
}
for _, distFile := range distFilesForTag {
if distFile != nil && !distFiles[tag].containsPath(distFile) {
distFiles[tag] = append(distFiles[tag], distFile)
}
}
distFiles = distFiles.addPathsForTag(tag, distFilesForTag...)
}
return distFiles

3
apex/apex.go
View File

@@ -1096,7 +1096,8 @@ var _ android.OutputFileProducer = (*apexBundle)(nil)
// Implements android.OutputFileProducer
func (a *apexBundle) OutputFiles(tag string) (android.Paths, error) {
switch tag {
case "":
case "", android.DefaultDistTag:
// This is the default dist path.
return android.Paths{a.outputFile}, nil
default:
return nil, fmt.Errorf("unsupported module reference tag %q", tag)

2
java/java.go
View File

@@ -486,6 +486,8 @@ func (j *Module) OutputFiles(tag string) (android.Paths, error) {
switch tag {
case "":
return append(android.Paths{j.outputFile}, j.extraOutputFiles...), nil
case android.DefaultDistTag:
return android.Paths{j.outputFile}, nil
case ".jar":
return android.Paths{j.implementationAndResourcesJar}, nil
case ".proguard_map":