Merge "Differentiate between no dist tag and an empty dist tag" am: 4326e0e094

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

Change-Id: I9044ee88fbf8bd7b0fc800ea11f4369816bb23b9

android/androidmk.go

@@ -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)

android/androidmk_test.go

@@ -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":

android/module.go

@@ -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

apex/apex.go

@@ -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)

java/java.go

@@ -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":