From af970a2e7abc1c3d348283bc185224bb5caa0872 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Mon, 23 Nov 2020 23:32:56 +0000 Subject: [PATCH] Generalize the handling of dist tags Previously in order to make use of dist entries with tag properties it was necessary for each module type to be modified to store the results of GenerateTaggedDistFiles(module) in the AndroidMkEntry.DistFiles property. This change generalizes that mechanism to work across all module types. This change does improve the behavior of a couple of tests where previously a dist with tag property was ignored because tagged dist files were not available now it is used correctly. Some module types do not implement OutputFileProducer interface and so they cannot handle dist with tag properties. This change makes it an error to attempt to do that. That necessitated adding OutputFiles(tag) method to customModule in queryview_test.go as some of the tests in that file used dist entries with tag properties. Follow up changes will remove the code that was made redundant by this. Test: m nothing m dist sdk - before and after this change, compare result to make sure that there are no significant differences. Bug: 174226317 Change-Id: Ifc54d67db10ce0d0fe8179c05b97a2be2113be4e --- android/androidmk.go | 9 +++++++ android/androidmk_test.go | 41 ++++++++++++++++++++++++------- android/module.go | 39 +++++++++++++++++++++-------- cmd/soong_build/queryview_test.go | 6 +++++ 4 files changed, 76 insertions(+), 19 deletions(-) diff --git a/android/androidmk.go b/android/androidmk.go index 42593bac4..063830b2c 100644 --- a/android/androidmk.go +++ b/android/androidmk.go @@ -238,6 +238,15 @@ func (a *AndroidMkEntries) getDistContributions(mod blueprint.Module) *distContr availableTaggedDists = availableTaggedDists.addPathsForTag(DefaultDistTag, a.OutputFile.Path()) } + // If the distFiles created by GenerateTaggedDistFiles contains paths for the + // DefaultDistTag then that takes priority so delete any existing paths. + if _, ok := amod.distFiles[DefaultDistTag]; ok { + delete(availableTaggedDists, DefaultDistTag) + } + + // Finally, merge the distFiles created by GenerateTaggedDistFiles. + availableTaggedDists = availableTaggedDists.merge(amod.distFiles) + if len(availableTaggedDists) == 0 { // Nothing dist-able for this module. return nil diff --git a/android/androidmk_test.go b/android/androidmk_test.go index ae9de552c..347b92eda 100644 --- a/android/androidmk_test.go +++ b/android/androidmk_test.go @@ -601,8 +601,6 @@ func TestGetDistContributions(t *testing.T) { { targets: ["my_goal"], }, - // The following is silently ignored because the dist files do not - // contain the tagged files. { targets: ["my_goal"], tag: ".multiple", @@ -617,6 +615,13 @@ func TestGetDistContributions(t *testing.T) { distCopyForTest("default-dist.out", "default-dist.out"), }, }, + { + goals: "my_goal", + copies: []distCopy{ + distCopyForTest("two.out", "two.out"), + distCopyForTest("three/four.out", "four.out"), + }, + }, }, }) @@ -631,15 +636,23 @@ func TestGetDistContributions(t *testing.T) { { targets: ["my_goal"], }, - // The following is silently ignored because the dist files do not - // contain the tagged files. { targets: ["my_goal"], tag: ".multiple", }, ], } -`, nil) +`, &distContributions{ + copiesForGoals: []*copiesForGoals{ + { + goals: "my_goal", + copies: []distCopy{ + distCopyForTest("two.out", "two.out"), + distCopyForTest("three/four.out", "four.out"), + }, + }, + }, + }) testHelper(t, "tagged-dist-files-default-output", ` custom { @@ -683,8 +696,6 @@ func TestGetDistContributions(t *testing.T) { { targets: ["my_goal"], }, - // The following is silently ignored because the dist files do not - // contain the tagged files. { targets: ["my_goal"], tag: ".multiple", @@ -699,6 +710,13 @@ func TestGetDistContributions(t *testing.T) { distCopyForTest("default-dist.out", "default-dist.out"), }, }, + { + goals: "my_goal", + copies: []distCopy{ + distCopyForTest("two.out", "two.out"), + distCopyForTest("three/four.out", "four.out"), + }, + }, }, }) @@ -711,8 +729,6 @@ func TestGetDistContributions(t *testing.T) { { targets: ["my_goal"], }, - // The following is silently ignored because the dist files do not - // contain the tagged files. { targets: ["my_goal"], tag: ".multiple", @@ -727,6 +743,13 @@ func TestGetDistContributions(t *testing.T) { distCopyForTest("dist-output-file.out", "dist-output-file.out"), }, }, + { + goals: "my_goal", + copies: []distCopy{ + distCopyForTest("two.out", "two.out"), + distCopyForTest("three/four.out", "four.out"), + }, + }, }, }) } diff --git a/android/module.go b/android/module.go index d64b5163c..d680baacb 100644 --- a/android/module.go +++ b/android/module.go @@ -1010,6 +1010,9 @@ type ModuleBase struct { noticeFiles Paths phonies map[string]Paths + // The files to copy to the dist as explicitly specified in the .bp file. + distFiles TaggedDistFiles + // Used by buildTargetSingleton to create checkbuild and per-directory build targets // Only set on the final variant of each module installTarget WritablePath @@ -1117,18 +1120,25 @@ func (m *ModuleBase) GenerateTaggedDistFiles(ctx BaseModuleContext) TaggedDistFi // 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 outputFileProducer, ok := m.module.(OutputFileProducer); ok { + // Call the OutputFiles(tag) method to get the paths associated with the tag. + distFilesForTag, err := outputFileProducer.OutputFiles(tag) - // 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 + // 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()) + } + + distFiles = distFiles.addPathsForTag(tag, distFilesForTag...) + } else if tag != DefaultDistTag { + // If the tag was specified then it is an error if the module does not + // implement OutputFileProducer because there is no other way of accessing + // the paths for the specified tag. + ctx.PropertyErrorf("dist.tag", + "tag %s not supported because the module does not implement OutputFileProducer", tag) } - - distFiles = distFiles.addPathsForTag(tag, distFilesForTag...) } return distFiles @@ -1658,6 +1668,15 @@ func (m *ModuleBase) GenerateBuildActions(blueprintCtx blueprint.ModuleContext) return } + // Create the set of tagged dist files after calling GenerateAndroidBuildActions + // as GenerateTaggedDistFiles() calls OutputFiles(tag) and so relies on the + // output paths being set which must be done before or during + // GenerateAndroidBuildActions. + m.distFiles = m.GenerateTaggedDistFiles(ctx) + if ctx.Failed() { + return + } + m.installFiles = append(m.installFiles, ctx.installFiles...) m.checkbuildFiles = append(m.checkbuildFiles, ctx.checkbuildFiles...) m.packagingSpecs = append(m.packagingSpecs, ctx.packagingSpecs...) diff --git a/cmd/soong_build/queryview_test.go b/cmd/soong_build/queryview_test.go index 525802a79..9471a9137 100644 --- a/cmd/soong_build/queryview_test.go +++ b/cmd/soong_build/queryview_test.go @@ -53,6 +53,12 @@ type customModule struct { android.ModuleBase } +// OutputFiles is needed because some instances of this module use dist with a +// tag property which requires the module implements OutputFileProducer. +func (m *customModule) OutputFiles(tag string) (android.Paths, error) { + return android.PathsForTesting("path" + tag), nil +} + func (m *customModule) GenerateAndroidBuildActions(ctx android.ModuleContext) { // nothing for now. }