Separate dist processing from make specific output

Previously, the GetDistForGoals(Module) func combined the processing
of the dist properties with generating the make specific rules for
generating that dist. That has a couple of problems:
1. It combines two pieces of functionality into one method which is
   bad practice.
2. It makes it hard to test because the make specific output ends up
   containing absolute paths to temporary directories created by the
   test.
3. It makes switching to a non-make output difficult and fragile as
   changing the output will also require changing the tests.

This change adds an intermediate data structure to contain the result
of the dist processing. That processing is done by the new method
getDistContributions(Module) which returns the new intermediate
structure. It also adds generateDistContributionsForMake(..) to
generate the make output. The GetDistForGoals(Module) func uses them to
implement the previous behavior.

It adds identical tests to those in TestGetDistForGoals() but leaves
those tests alone to show that this refactoring does not change the
behavior. Follow up changes will clean up TestGetDistForGoals(). It
also adds a test for generateDistContributionsForMake(..).

Bug: 174226317
Test: m nothing
      m dist sdk - before and after this change, compare result to
      make sure that there are no significant differences.
Change-Id: I458b7c8e4485bf66d3498f50df85a8d65fc2ee00

android/androidmk.go

@@ -177,13 +177,51 @@ func (a *AndroidMkEntries) AddStrings(name string, value ...string) {
 	a.EntryMap[name] = append(a.EntryMap[name], value...)
 }
 
-// Compute the list of Make strings to declare phone goals and dist-for-goals
-// calls from the module's dist and dists properties.
-func (a *AndroidMkEntries) GetDistForGoals(mod blueprint.Module) []string {
+// The contributions to the dist.
+type distContributions struct {
+	// List of goals and the dist copy instructions.
+	copiesForGoals []*copiesForGoals
+}
+
+// getCopiesForGoals returns a copiesForGoals into which copy instructions that
+// must be processed when building one or more of those goals can be added.
+func (d *distContributions) getCopiesForGoals(goals string) *copiesForGoals {
+	copiesForGoals := &copiesForGoals{goals: goals}
+	d.copiesForGoals = append(d.copiesForGoals, copiesForGoals)
+	return copiesForGoals
+}
+
+// Associates a list of dist copy instructions with a set of goals for which they
+// should be run.
+type copiesForGoals struct {
+	// goals are a space separated list of build targets that will trigger the
+	// copy instructions.
+	goals string
+
+	// A list of instructions to copy a module's output files to somewhere in the
+	// dist directory.
+	copies []distCopy
+}
+
+// Adds a copy instruction.
+func (d *copiesForGoals) addCopyInstruction(from Path, dest string) {
+	d.copies = append(d.copies, distCopy{from, dest})
+}
+
+// Instruction on a path that must be copied into the dist.
+type distCopy struct {
+	// The path to copy from.
+	from Path
+
+	// The destination within the dist directory to copy to.
+	dest string
+}
+
+// Compute the contributions that the module makes to the dist.
+func (a *AndroidMkEntries) getDistContributions(mod blueprint.Module) *distContributions {
 	amod := mod.(Module).base()
 	name := amod.BaseModuleName()
 
-	var ret []string
 	var availableTaggedDists TaggedDistFiles
 
 	if a.DistFiles != nil {
@@ -195,6 +233,9 @@ func (a *AndroidMkEntries) GetDistForGoals(mod blueprint.Module) []string {
 		return nil
 	}
 
+	// Collate the contributions this module makes to the dist.
+	distContributions := &distContributions{}
+
 	// Iterate over this module's dist structs, merged from the dist and dists properties.
 	for _, dist := range amod.Dists() {
 		// Get the list of goals this dist should be enabled for. e.g. sdk, droidcore
@@ -224,9 +265,9 @@ func (a *AndroidMkEntries) GetDistForGoals(mod blueprint.Module) []string {
 			panic(fmt.Errorf(errorMessage, goals, name, tagPaths))
 		}
 
-		ret = append(ret, fmt.Sprintf(".PHONY: %s\n", goals))
+		copiesForGoals := distContributions.getCopiesForGoals(goals)
 
-		// Create dist-for-goals calls for each path in the dist'd files.
+		// Iterate over each path adding a copy instruction to copiesForGoals
 		for _, path := range tagPaths {
 			// It's possible that the Path is nil from errant modules. Be defensive here.
 			if path == nil {
@@ -261,15 +302,41 @@ func (a *AndroidMkEntries) GetDistForGoals(mod blueprint.Module) []string {
 				}
 			}
 
+			copiesForGoals.addCopyInstruction(path, dest)
+		}
+	}
+
+	return distContributions
+}
+
+// generateDistContributionsForMake generates make rules that will generate the
+// dist according to the instructions in the supplied distContribution.
+func generateDistContributionsForMake(distContributions *distContributions) []string {
+	var ret []string
+	for _, d := range distContributions.copiesForGoals {
+		ret = append(ret, fmt.Sprintf(".PHONY: %s\n", d.goals))
+		// Create dist-for-goals calls for each of the copy instructions.
+		for _, c := range d.copies {
 			ret = append(
 				ret,
-				fmt.Sprintf("$(call dist-for-goals,%s,%s:%s)\n", goals, path.String(), dest))
+				fmt.Sprintf("$(call dist-for-goals,%s,%s:%s)\n", d.goals, c.from.String(), c.dest))
 		}
 	}
 
 	return ret
 }
 
+// Compute the list of Make strings to declare phony goals and dist-for-goals
+// calls from the module's dist and dists properties.
+func (a *AndroidMkEntries) GetDistForGoals(mod blueprint.Module) []string {
+	distContributions := a.getDistContributions(mod)
+	if distContributions == nil {
+		return nil
+	}
+
+	return generateDistContributionsForMake(distContributions)
+}
+
 func (a *AndroidMkEntries) fillInEntries(config Config, bpPath string, mod blueprint.Module) {
 	a.EntryMap = make(map[string][]string)
 	amod := mod.(Module).base()

android/androidmk_test.go

@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"io"
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -110,6 +111,27 @@ func TestAndroidMkSingleton_PassesUpdatedAndroidMkDataToCustomCallback(t *testin
 	assertEqual([]string{"qux"}, m.data.Target_required)
 }
 
+func TestGenerateDistContributionsForMake(t *testing.T) {
+	dc := &distContributions{
+		copiesForGoals: []*copiesForGoals{
+			{
+				goals: "my_goal",
+				copies: []distCopy{
+					distCopyForTest("one.out", "one.out"),
+					distCopyForTest("two.out", "other.out"),
+				},
+			},
+		},
+	}
+
+	makeOutput := generateDistContributionsForMake(dc)
+
+	assertStringEquals(t, `.PHONY: my_goal
+$(call dist-for-goals,my_goal,one.out:one.out)
+$(call dist-for-goals,my_goal,two.out:other.out)
+`, strings.Join(makeOutput, ""))
+}
+
 func TestGetDistForGoals(t *testing.T) {
 	testCases := []struct {
 		name                   string
@@ -293,3 +315,280 @@ func TestGetDistForGoals(t *testing.T) {
 		})
 	}
 }
+
+func distCopyForTest(from, to string) distCopy {
+	return distCopy{PathForTesting(from), to}
+}
+
+func TestGetDistContributions(t *testing.T) {
+	compareContributions := func(d1 *distContributions, d2 *distContributions) error {
+		if d1 == nil || d2 == nil {
+			if d1 != d2 {
+				return fmt.Errorf("pointer mismatch, expected both to be nil but they were %p and %p", d1, d2)
+			} else {
+				return nil
+			}
+		}
+		if expected, actual := len(d1.copiesForGoals), len(d2.copiesForGoals); expected != actual {
+			return fmt.Errorf("length mismatch, expected %d found %d", expected, actual)
+		}
+
+		for i, copies1 := range d1.copiesForGoals {
+			copies2 := d2.copiesForGoals[i]
+			if expected, actual := copies1.goals, copies2.goals; expected != actual {
+				return fmt.Errorf("goals mismatch at position %d: expected %q found %q", i, expected, actual)
+			}
+
+			if expected, actual := len(copies1.copies), len(copies2.copies); expected != actual {
+				return fmt.Errorf("length mismatch in copy instructions at position %d, expected %d found %d", i, expected, actual)
+			}
+
+			for j, c1 := range copies1.copies {
+				c2 := copies2.copies[j]
+				if expected, actual := NormalizePathForTesting(c1.from), NormalizePathForTesting(c2.from); expected != actual {
+					return fmt.Errorf("paths mismatch at position %d.%d: expected %q found %q", i, j, expected, actual)
+				}
+
+				if expected, actual := c1.dest, c2.dest; expected != actual {
+					return fmt.Errorf("dest mismatch at position %d.%d: expected %q found %q", i, j, expected, actual)
+				}
+			}
+		}
+
+		return nil
+	}
+
+	formatContributions := func(d *distContributions) string {
+		buf := &strings.Builder{}
+		if d == nil {
+			fmt.Fprint(buf, "nil")
+		} else {
+			for _, copiesForGoals := range d.copiesForGoals {
+				fmt.Fprintf(buf, "    Goals: %q {\n", copiesForGoals.goals)
+				for _, c := range copiesForGoals.copies {
+					fmt.Fprintf(buf, "        %s -> %s\n", NormalizePathForTesting(c.from), c.dest)
+				}
+				fmt.Fprint(buf, "    }\n")
+			}
+		}
+		return buf.String()
+	}
+
+	testHelper := func(t *testing.T, name, bp string, expectedContributions *distContributions) {
+		t.Helper()
+		t.Run(name, func(t *testing.T) {
+			t.Helper()
+
+			config, module := buildConfigAndCustomModuleFoo(t, bp)
+			entries := AndroidMkEntriesForTest(t, config, "", module)
+			if len(entries) != 1 {
+				t.Errorf("Expected a single AndroidMk entry, got %d", len(entries))
+			}
+			distContributions := entries[0].getDistContributions(module)
+
+			if err := compareContributions(expectedContributions, distContributions); err != nil {
+				t.Errorf("%s\nExpected Contributions\n%sActualContributions\n%s",
+					err,
+					formatContributions(expectedContributions),
+					formatContributions(distContributions))
+			}
+		})
+	}
+
+	testHelper(t, "dist-without-tag", `
+			custom {
+				name: "foo",
+				dist: {
+					targets: ["my_goal"]
+				}
+			}
+`,
+		&distContributions{
+			copiesForGoals: []*copiesForGoals{
+				{
+					goals: "my_goal",
+					copies: []distCopy{
+						distCopyForTest("one.out", "one.out"),
+					},
+				},
+			},
+		})
+
+	testHelper(t, "dist-with-tag", `
+			custom {
+				name: "foo",
+				dist: {
+					targets: ["my_goal"],
+					tag: ".another-tag",
+				}
+			}
+`,
+		&distContributions{
+			copiesForGoals: []*copiesForGoals{
+				{
+					goals: "my_goal",
+					copies: []distCopy{
+						distCopyForTest("another.out", "another.out"),
+					},
+				},
+			},
+		})
+
+	testHelper(t, "dists-with-tag", `
+			custom {
+				name: "foo",
+				dists: [
+					{
+						targets: ["my_goal"],
+						tag: ".another-tag",
+					},
+				],
+			}
+`,
+		&distContributions{
+			copiesForGoals: []*copiesForGoals{
+				{
+					goals: "my_goal",
+					copies: []distCopy{
+						distCopyForTest("another.out", "another.out"),
+					},
+				},
+			},
+		})
+
+	testHelper(t, "multiple-dists-with-and-without-tag", `
+			custom {
+				name: "foo",
+				dists: [
+					{
+						targets: ["my_goal"],
+					},
+					{
+						targets: ["my_second_goal", "my_third_goal"],
+					},
+				],
+			}
+`,
+		&distContributions{
+			copiesForGoals: []*copiesForGoals{
+				{
+					goals: "my_goal",
+					copies: []distCopy{
+						distCopyForTest("one.out", "one.out"),
+					},
+				},
+				{
+					goals: "my_second_goal my_third_goal",
+					copies: []distCopy{
+						distCopyForTest("one.out", "one.out"),
+					},
+				},
+			},
+		})
+
+	testHelper(t, "dist-plus-dists-without-tags", `
+			custom {
+				name: "foo",
+				dist: {
+					targets: ["my_goal"],
+				},
+				dists: [
+					{
+						targets: ["my_second_goal", "my_third_goal"],
+					},
+				],
+			}
+`,
+		&distContributions{
+			copiesForGoals: []*copiesForGoals{
+				{
+					goals: "my_second_goal my_third_goal",
+					copies: []distCopy{
+						distCopyForTest("one.out", "one.out"),
+					},
+				},
+				{
+					goals: "my_goal",
+					copies: []distCopy{
+						distCopyForTest("one.out", "one.out"),
+					},
+				},
+			},
+		})
+
+	testHelper(t, "dist-plus-dists-with-tags", `
+			custom {
+				name: "foo",
+				dist: {
+					targets: ["my_goal", "my_other_goal"],
+					tag: ".multiple",
+				},
+				dists: [
+					{
+						targets: ["my_second_goal"],
+						tag: ".multiple",
+					},
+					{
+						targets: ["my_third_goal"],
+						dir: "test/dir",
+					},
+					{
+						targets: ["my_fourth_goal"],
+						suffix: ".suffix",
+					},
+					{
+						targets: ["my_fifth_goal"],
+						dest: "new-name",
+					},
+					{
+						targets: ["my_sixth_goal"],
+						dest: "new-name",
+						dir: "some/dir",
+						suffix: ".suffix",
+					},
+				],
+			}
+`,
+		&distContributions{
+			copiesForGoals: []*copiesForGoals{
+				{
+					goals: "my_second_goal",
+					copies: []distCopy{
+						distCopyForTest("two.out", "two.out"),
+						distCopyForTest("three/four.out", "four.out"),
+					},
+				},
+				{
+					goals: "my_third_goal",
+					copies: []distCopy{
+						distCopyForTest("one.out", "test/dir/one.out"),
+					},
+				},
+				{
+					goals: "my_fourth_goal",
+					copies: []distCopy{
+						distCopyForTest("one.out", "one.suffix.out"),
+					},
+				},
+				{
+					goals: "my_fifth_goal",
+					copies: []distCopy{
+						distCopyForTest("one.out", "new-name"),
+					},
+				},
+				{
+					goals: "my_sixth_goal",
+					copies: []distCopy{
+						distCopyForTest("one.out", "some/dir/new-name.suffix"),
+					},
+				},
+				{
+					goals: "my_goal my_other_goal",
+					copies: []distCopy{
+						distCopyForTest("two.out", "two.out"),
+						distCopyForTest("three/four.out", "four.out"),
+					},
+				},
+			},
+		})
+}