From 32f3898f0b61b27944ceff041d637c1af828cd86 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 22 Feb 2018 11:47:25 -0800 Subject: [PATCH 1/5] Remove unused intermediates parameter from ExistentPathForSource Test: m checkbuild Change-Id: Id2c0a5039c2ec3b3795385c135ffec022ccd691e --- android/package_ctx.go | 2 +- android/paths.go | 16 +++++----------- cc/pgo.go | 2 +- java/java.go | 6 +++--- 4 files changed, 10 insertions(+), 16 deletions(-) diff --git a/android/package_ctx.go b/android/package_ctx.go index 0dbcea521..849eb72a8 100644 --- a/android/package_ctx.go +++ b/android/package_ctx.go @@ -240,7 +240,7 @@ func (p PackageContext) PrefixedExistentPathsForSourcesVariable( return p.VariableFunc(name, func(config Config) (string, error) { ctx := &configErrorWrapper{p, config, []error{}} - paths := ExistentPathsForSources(ctx, "", paths) + paths := ExistentPathsForSources(ctx, paths) if len(ctx.errors) > 0 { return "", ctx.errors[0] } diff --git a/android/paths.go b/android/paths.go index e941e964d..10a9dc635 100644 --- a/android/paths.go +++ b/android/paths.go @@ -191,9 +191,8 @@ func PathsForSource(ctx PathContext, paths []string) Paths { if ctx.Config().AllowMissingDependencies() { if modCtx, ok := ctx.(ModuleContext); ok { ret := make(Paths, 0, len(paths)) - intermediates := pathForModule(modCtx).withRel("missing") for _, path := range paths { - p := ExistentPathForSource(ctx, intermediates.String(), path) + p := ExistentPathForSource(ctx, path) if p.Valid() { ret = append(ret, p.Path()) } else { @@ -213,10 +212,10 @@ func PathsForSource(ctx PathContext, paths []string) Paths { // ExistentPathsForSources returns a list of Paths rooted from SrcDir that are // found in the tree. If any are not found, they are omitted from the list, // and dependencies are added so that we're re-run when they are added. -func ExistentPathsForSources(ctx PathContext, intermediates string, paths []string) Paths { +func ExistentPathsForSources(ctx PathContext, paths []string) Paths { ret := make(Paths, 0, len(paths)) for _, path := range paths { - p := ExistentPathForSource(ctx, intermediates, path) + p := ExistentPathForSource(ctx, path) if p.Valid() { ret = append(ret, p.Path()) } @@ -531,12 +530,7 @@ func PathForSource(ctx PathContext, pathComponents ...string) SourcePath { // ExistentPathForSource returns an OptionalPath with the SourcePath if the // path exists, or an empty OptionalPath if it doesn't exist. Dependencies are added // so that the ninja file will be regenerated if the state of the path changes. -func ExistentPathForSource(ctx PathContext, intermediates string, pathComponents ...string) OptionalPath { - if len(pathComponents) == 0 { - // For when someone forgets the 'intermediates' argument - panic("Missing path(s)") - } - +func ExistentPathForSource(ctx PathContext, pathComponents ...string) OptionalPath { p := validatePath(ctx, pathComponents...) path := SourcePath{basePath{p, ctx.Config(), ""}} @@ -765,7 +759,7 @@ func PathForVndkRefAbiDump(ctx ModuleContext, version, fileName string, vndkOrNd } refDumpFileStr := "prebuilts/abi-dumps/" + vndkOrNdkDir + "/" + version + "/" + archName + "/" + sourceOrBinaryDir + "/" + fileName + ext - return ExistentPathForSource(ctx, "", refDumpFileStr) + return ExistentPathForSource(ctx, refDumpFileStr) } // PathForModuleOut returns a Path representing the paths... under the module's diff --git a/cc/pgo.go b/cc/pgo.go index 779ef39de..10c8daca3 100644 --- a/cc/pgo.go +++ b/cc/pgo.go @@ -102,7 +102,7 @@ func (props *PgoProperties) addProfileGatherFlags(ctx ModuleContext, flags Flags func (props *PgoProperties) getPgoProfileFile(ctx BaseModuleContext) android.OptionalPath { // Test if the profile_file is present in any of the PGO profile projects for _, profileProject := range getPgoProfileProjects(ctx.DeviceConfig()) { - path := android.ExistentPathForSource(ctx, "", profileProject, *props.Pgo.Profile_file) + path := android.ExistentPathForSource(ctx, profileProject, *props.Pgo.Profile_file) if path.Valid() { return path } diff --git a/java/java.go b/java/java.go index 4a7d70438..ae48bd658 100644 --- a/java/java.go +++ b/java/java.go @@ -371,8 +371,8 @@ func decodeSdkDep(ctx android.BaseContext, v string) sdkDep { jar = filepath.Join(dir, "core.jar") } aidl := filepath.Join(dir, "framework.aidl") - jarPath := android.ExistentPathForSource(ctx, "sdkdir", jar) - aidlPath := android.ExistentPathForSource(ctx, "sdkdir", aidl) + jarPath := android.ExistentPathForSource(ctx, jar) + aidlPath := android.ExistentPathForSource(ctx, aidl) if (!jarPath.Valid() || !aidlPath.Valid()) && ctx.Config().AllowMissingDependencies() { return sdkDep{ @@ -530,7 +530,7 @@ func (j *Module) aidlFlags(ctx android.ModuleContext, aidlPreprocess android.Opt flags = append(flags, android.JoinWithPrefix(j.exportAidlIncludeDirs.Strings(), "-I")) flags = append(flags, android.JoinWithPrefix(aidlIncludes.Strings(), "-I")) flags = append(flags, "-I"+android.PathForModuleSrc(ctx).String()) - if src := android.ExistentPathForSource(ctx, "", ctx.ModuleDir(), "src"); src.Valid() { + if src := android.ExistentPathForSource(ctx, ctx.ModuleDir(), "src"); src.Valid() { flags = append(flags, "-I"+src.String()) } From dc75ae70f39837514bf23222b4eada098eb87aa3 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 22 Feb 2018 13:48:13 -0800 Subject: [PATCH 2/5] Add t.Run and t.Helper to paths_test.go Test: paths_test.go Change-Id: Iea2b07815fe82a346c5384571ebc2b57538722cc --- android/paths_test.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/android/paths_test.go b/android/paths_test.go index 61a172fa0..7e7ecbdfd 100644 --- a/android/paths_test.go +++ b/android/paths_test.go @@ -110,17 +110,21 @@ var validatePathTestCases = append(commonValidatePathTestCases, []strsTestCase{ func TestValidateSafePath(t *testing.T) { for _, testCase := range validateSafePathTestCases { - ctx := &configErrorWrapper{} - out := validateSafePath(ctx, testCase.in...) - check(t, "validateSafePath", p(testCase.in), out, ctx.errors, testCase.out, testCase.err) + t.Run(strings.Join(testCase.in, ","), func(t *testing.T) { + ctx := &configErrorWrapper{} + out := validateSafePath(ctx, testCase.in...) + check(t, "validateSafePath", p(testCase.in), out, ctx.errors, testCase.out, testCase.err) + }) } } func TestValidatePath(t *testing.T) { for _, testCase := range validatePathTestCases { - ctx := &configErrorWrapper{} - out := validatePath(ctx, testCase.in...) - check(t, "validatePath", p(testCase.in), out, ctx.errors, testCase.out, testCase.err) + t.Run(strings.Join(testCase.in, ","), func(t *testing.T) { + ctx := &configErrorWrapper{} + out := validatePath(ctx, testCase.in...) + check(t, "validatePath", p(testCase.in), out, ctx.errors, testCase.out, testCase.err) + }) } } @@ -133,6 +137,7 @@ func TestOptionalPath(t *testing.T) { } func checkInvalidOptionalPath(t *testing.T, path OptionalPath) { + t.Helper() if path.Valid() { t.Errorf("Uninitialized OptionalPath should not be valid") } @@ -150,9 +155,11 @@ func checkInvalidOptionalPath(t *testing.T, path OptionalPath) { func check(t *testing.T, testType, testString string, got interface{}, err []error, expected interface{}, expectedErr []error) { + t.Helper() printedTestCase := false e := func(s string, expected, got interface{}) { + t.Helper() if !printedTestCase { t.Errorf("test case %s: %s", testType, testString) printedTestCase = true From 1ccfcc36bd27c0c1f96c77c628933267c7e50906 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 22 Feb 2018 13:54:26 -0800 Subject: [PATCH 3/5] Propagate errors out of validatePath The next patch will need to more complicated custom error handling, so make validatePath return an error and let the caller handle it. Test: paths_test.go Change-Id: I4fe11c3f319303d779596709f4819e828b5bdb9b --- android/paths.go | 137 ++++++++++++++++++++++++++++-------------- android/paths_test.go | 10 ++- 2 files changed, 100 insertions(+), 47 deletions(-) diff --git a/android/paths.go b/android/paths.go index 10a9dc635..3d4d3f30e 100644 --- a/android/paths.go +++ b/android/paths.go @@ -70,7 +70,14 @@ var _ moduleErrorf = blueprint.ModuleContext(nil) // reportPathError will register an error with the attached context. It // attempts ctx.ModuleErrorf for a better error message first, then falls // back to ctx.Errorf. -func reportPathError(ctx PathContext, format string, args ...interface{}) { +func reportPathError(ctx PathContext, err error) { + reportPathErrorf(ctx, "%s", err.Error()) +} + +// reportPathErrorf will register an error with the attached context. It +// attempts ctx.ModuleErrorf for a better error message first, then falls +// back to ctx.Errorf. +func reportPathErrorf(ctx PathContext, format string, args ...interface{}) { if mctx, ok := ctx.(moduleErrorf); ok { mctx.ModuleErrorf(format, args...) } else if ectx, ok := ctx.(errorfContext); ok { @@ -121,7 +128,7 @@ func GenPathWithExt(ctx ModuleContext, subdir string, p Path, ext string) Module if path, ok := p.(genPathProvider); ok { return path.genPathWithExt(ctx, subdir, ext) } - reportPathError(ctx, "Tried to create generated file from unsupported path: %s(%s)", reflect.TypeOf(p).Name(), p) + reportPathErrorf(ctx, "Tried to create generated file from unsupported path: %s(%s)", reflect.TypeOf(p).Name(), p) return PathForModuleGen(ctx) } @@ -131,7 +138,7 @@ func ObjPathWithExt(ctx ModuleContext, subdir string, p Path, ext string) Module if path, ok := p.(objPathProvider); ok { return path.objPathWithExt(ctx, subdir, ext) } - reportPathError(ctx, "Tried to create object file from unsupported path: %s (%s)", reflect.TypeOf(p).Name(), p) + reportPathErrorf(ctx, "Tried to create object file from unsupported path: %s (%s)", reflect.TypeOf(p).Name(), p) return PathForModuleObj(ctx) } @@ -142,7 +149,7 @@ func ResPathWithName(ctx ModuleContext, p Path, name string) ModuleResPath { if path, ok := p.(resPathProvider); ok { return path.resPathWithName(ctx, name) } - reportPathError(ctx, "Tried to create res file from unsupported path: %s (%s)", reflect.TypeOf(p).Name(), p) + reportPathErrorf(ctx, "Tried to create res file from unsupported path: %s (%s)", reflect.TypeOf(p).Name(), p) return PathForModuleRes(ctx) } @@ -245,7 +252,7 @@ func pathsForModuleSrcFromFullPath(ctx ModuleContext, paths []string) Paths { for _, p := range paths { path := filepath.Clean(p) if !strings.HasPrefix(path, prefix) { - reportPathError(ctx, "Path '%s' is not in module source directory '%s'", p, prefix) + reportPathErrorf(ctx, "Path '%s' is not in module source directory '%s'", p, prefix) continue } ret = append(ret, PathForModuleSrc(ctx, path[len(prefix):])) @@ -476,21 +483,24 @@ func (p SourcePath) withRel(rel string) SourcePath { // safePathForSource is for paths that we expect are safe -- only for use by go // code that is embedding ninja variables in paths func safePathForSource(ctx PathContext, path string) SourcePath { - p := validateSafePath(ctx, path) + p, err := validateSafePath(path) + if err != nil { + reportPathError(ctx, err) + } ret := SourcePath{basePath{p, ctx.Config(), ""}} abs, err := filepath.Abs(ret.String()) if err != nil { - reportPathError(ctx, "%s", err.Error()) + reportPathError(ctx, err) return ret } buildroot, err := filepath.Abs(ctx.Config().buildDir) if err != nil { - reportPathError(ctx, "%s", err.Error()) + reportPathError(ctx, err) return ret } if strings.HasPrefix(abs, buildroot) { - reportPathError(ctx, "source path %s is in output", abs) + reportPathErrorf(ctx, "source path %s is in output", abs) return ret } @@ -501,28 +511,32 @@ func safePathForSource(ctx PathContext, path string) SourcePath { // neither escapes the source dir nor is in the out dir. // On error, it will return a usable, but invalid SourcePath, and report a ModuleError. func PathForSource(ctx PathContext, pathComponents ...string) SourcePath { - p := validatePath(ctx, pathComponents...) + p, err := validatePath(pathComponents...) ret := SourcePath{basePath{p, ctx.Config(), ""}} + if err != nil { + reportPathError(ctx, err) + return ret + } abs, err := filepath.Abs(ret.String()) if err != nil { - reportPathError(ctx, "%s", err.Error()) + reportPathError(ctx, err) return ret } buildroot, err := filepath.Abs(ctx.Config().buildDir) if err != nil { - reportPathError(ctx, "%s", err.Error()) + reportPathError(ctx, err) return ret } if strings.HasPrefix(abs, buildroot) { - reportPathError(ctx, "source path %s is in output", abs) + reportPathErrorf(ctx, "source path %s is in output", abs) return ret } if exists, _, err := ctx.Fs().Exists(ret.String()); err != nil { - reportPathError(ctx, "%s: %s", ret, err.Error()) + reportPathErrorf(ctx, "%s: %s", ret, err.Error()) } else if !exists { - reportPathError(ctx, "source path %s does not exist", ret) + reportPathErrorf(ctx, "source path %s does not exist", ret) } return ret } @@ -531,26 +545,30 @@ func PathForSource(ctx PathContext, pathComponents ...string) SourcePath { // path exists, or an empty OptionalPath if it doesn't exist. Dependencies are added // so that the ninja file will be regenerated if the state of the path changes. func ExistentPathForSource(ctx PathContext, pathComponents ...string) OptionalPath { - p := validatePath(ctx, pathComponents...) + p, err := validatePath(pathComponents...) + if err != nil { + reportPathError(ctx, err) + return OptionalPath{} + } path := SourcePath{basePath{p, ctx.Config(), ""}} abs, err := filepath.Abs(path.String()) if err != nil { - reportPathError(ctx, "%s", err.Error()) + reportPathError(ctx, err) return OptionalPath{} } buildroot, err := filepath.Abs(ctx.Config().buildDir) if err != nil { - reportPathError(ctx, "%s", err.Error()) + reportPathError(ctx, err) return OptionalPath{} } if strings.HasPrefix(abs, buildroot) { - reportPathError(ctx, "source path %s is in output", abs) + reportPathErrorf(ctx, "source path %s is in output", abs) return OptionalPath{} } if pathtools.IsGlob(path.String()) { - reportPathError(ctx, "path may not contain a glob: %s", path.String()) + reportPathErrorf(ctx, "path may not contain a glob: %s", path.String()) return OptionalPath{} } @@ -559,7 +577,7 @@ func ExistentPathForSource(ctx PathContext, pathComponents ...string) OptionalPa // a single file. files, err := gctx.GlobWithDeps(path.String(), nil) if err != nil { - reportPathError(ctx, "glob: %s", err.Error()) + reportPathErrorf(ctx, "glob: %s", err.Error()) return OptionalPath{} } @@ -571,7 +589,7 @@ func ExistentPathForSource(ctx PathContext, pathComponents ...string) OptionalPa // AddNinjaFileDeps files, dirs, err := pathtools.Glob(path.String(), nil) if err != nil { - reportPathError(ctx, "glob: %s", err.Error()) + reportPathErrorf(ctx, "glob: %s", err.Error()) return OptionalPath{} } @@ -593,7 +611,10 @@ func (p SourcePath) String() string { // Join creates a new SourcePath with paths... joined with the current path. The // provided paths... may not use '..' to escape from the current path. func (p SourcePath) Join(ctx PathContext, paths ...string) SourcePath { - path := validatePath(ctx, paths...) + path, err := validatePath(paths...) + if err != nil { + reportPathError(ctx, err) + } return p.withRel(path) } @@ -606,17 +627,17 @@ func (p SourcePath) OverlayPath(ctx ModuleContext, path Path) OptionalPath { } else if srcPath, ok := path.(SourcePath); ok { relDir = srcPath.path } else { - reportPathError(ctx, "Cannot find relative path for %s(%s)", reflect.TypeOf(path).Name(), path) + reportPathErrorf(ctx, "Cannot find relative path for %s(%s)", reflect.TypeOf(path).Name(), path) return OptionalPath{} } dir := filepath.Join(p.config.srcDir, p.path, relDir) // Use Glob so that we are run again if the directory is added. if pathtools.IsGlob(dir) { - reportPathError(ctx, "Path may not contain a glob: %s", dir) + reportPathErrorf(ctx, "Path may not contain a glob: %s", dir) } paths, err := ctx.GlobWithDeps(dir, []string{}) if err != nil { - reportPathError(ctx, "glob: %s", err.Error()) + reportPathErrorf(ctx, "glob: %s", err.Error()) return OptionalPath{} } if len(paths) == 0 { @@ -624,7 +645,7 @@ func (p SourcePath) OverlayPath(ctx ModuleContext, path Path) OptionalPath { } relPath, err := filepath.Rel(p.config.srcDir, paths[0]) if err != nil { - reportPathError(ctx, "%s", err.Error()) + reportPathError(ctx, err) return OptionalPath{} } return OptionalPathForPath(PathForSource(ctx, relPath)) @@ -646,7 +667,10 @@ var _ Path = OutputPath{} // validated to not escape the build dir. // On error, it will return a usable, but invalid OutputPath, and report a ModuleError. func PathForOutput(ctx PathContext, pathComponents ...string) OutputPath { - path := validatePath(ctx, pathComponents...) + path, err := validatePath(pathComponents...) + if err != nil { + reportPathError(ctx, err) + } return OutputPath{basePath{path, ctx.Config(), ""}} } @@ -663,14 +687,20 @@ func (p OutputPath) RelPathString() string { // Join creates a new OutputPath with paths... joined with the current path. The // provided paths... may not use '..' to escape from the current path. func (p OutputPath) Join(ctx PathContext, paths ...string) OutputPath { - path := validatePath(ctx, paths...) + path, err := validatePath(paths...) + if err != nil { + reportPathError(ctx, err) + } return p.withRel(path) } // PathForIntermediates returns an OutputPath representing the top-level // intermediates directory. func PathForIntermediates(ctx PathContext, paths ...string) OutputPath { - path := validatePath(ctx, paths...) + path, err := validatePath(paths...) + if err != nil { + reportPathError(ctx, err) + } return PathForOutput(ctx, ".intermediates", path) } @@ -687,7 +717,10 @@ var _ resPathProvider = ModuleSrcPath{} // PathForModuleSrc returns a ModuleSrcPath representing the paths... under the // module's local source directory. func PathForModuleSrc(ctx ModuleContext, paths ...string) ModuleSrcPath { - p := validatePath(ctx, paths...) + p, err := validatePath(paths...) + if err != nil { + reportPathError(ctx, err) + } path := ModuleSrcPath{PathForSource(ctx, ctx.ModuleDir(), p)} path.basePath.rel = p return path @@ -765,7 +798,10 @@ func PathForVndkRefAbiDump(ctx ModuleContext, version, fileName string, vndkOrNd // PathForModuleOut returns a Path representing the paths... under the module's // output directory. func PathForModuleOut(ctx ModuleContext, paths ...string) ModuleOutPath { - p := validatePath(ctx, paths...) + p, err := validatePath(paths...) + if err != nil { + reportPathError(ctx, err) + } return ModuleOutPath{ OutputPath: pathForModule(ctx).withRel(p), } @@ -784,7 +820,10 @@ var _ objPathProvider = ModuleGenPath{} // PathForModuleGen returns a Path representing the paths... under the module's // `gen' directory. func PathForModuleGen(ctx ModuleContext, paths ...string) ModuleGenPath { - p := validatePath(ctx, paths...) + p, err := validatePath(paths...) + if err != nil { + reportPathError(ctx, err) + } return ModuleGenPath{ ModuleOutPath: ModuleOutPath{ OutputPath: pathForModule(ctx).withRel("gen").withRel(p), @@ -812,7 +851,10 @@ var _ Path = ModuleObjPath{} // PathForModuleObj returns a Path representing the paths... under the module's // 'obj' directory. func PathForModuleObj(ctx ModuleContext, pathComponents ...string) ModuleObjPath { - p := validatePath(ctx, pathComponents...) + p, err := validatePath(pathComponents...) + if err != nil { + reportPathError(ctx, err) + } return ModuleObjPath{PathForModuleOut(ctx, "obj", p)} } @@ -827,7 +869,11 @@ var _ Path = ModuleResPath{} // PathForModuleRes returns a Path representing the paths... under the module's // 'res' directory. func PathForModuleRes(ctx ModuleContext, pathComponents ...string) ModuleResPath { - p := validatePath(ctx, pathComponents...) + p, err := validatePath(pathComponents...) + if err != nil { + reportPathError(ctx, err) + } + return ModuleResPath{PathForModuleOut(ctx, "res", p)} } @@ -873,36 +919,34 @@ func PathForModuleInstall(ctx ModuleInstallPathContext, pathComponents ...string // validateSafePath validates a path that we trust (may contain ninja variables). // Ensures that each path component does not attempt to leave its component. -func validateSafePath(ctx PathContext, pathComponents ...string) string { +func validateSafePath(pathComponents ...string) (string, error) { for _, path := range pathComponents { path := filepath.Clean(path) if path == ".." || strings.HasPrefix(path, "../") || strings.HasPrefix(path, "/") { - reportPathError(ctx, "Path is outside directory: %s", path) - return "" + return "", fmt.Errorf("Path is outside directory: %s", path) } } // TODO: filepath.Join isn't necessarily correct with embedded ninja // variables. '..' may remove the entire ninja variable, even if it // will be expanded to multiple nested directories. - return filepath.Join(pathComponents...) + return filepath.Join(pathComponents...), nil } // validatePath validates that a path does not include ninja variables, and that // each path component does not attempt to leave its component. Returns a joined // version of each path component. -func validatePath(ctx PathContext, pathComponents ...string) string { +func validatePath(pathComponents ...string) (string, error) { for _, path := range pathComponents { if strings.Contains(path, "$") { - reportPathError(ctx, "Path contains invalid character($): %s", path) - return "" + return "", fmt.Errorf("Path contains invalid character($): %s", path) } } - return validateSafePath(ctx, pathComponents...) + return validateSafePath(pathComponents...) } func PathForPhony(ctx PathContext, phony string) WritablePath { if strings.ContainsAny(phony, "$/") { - reportPathError(ctx, "Phony target contains invalid character ($ or /): %s", phony) + reportPathErrorf(ctx, "Phony target contains invalid character ($ or /): %s", phony) } return PhonyPath{basePath{phony, ctx.Config(), ""}} } @@ -925,7 +969,10 @@ func (p testPath) String() string { } func PathForTesting(paths ...string) Path { - p := validateSafePath(nil, paths...) + p, err := validateSafePath(paths...) + if err != nil { + panic(err) + } return testPath{basePath{path: p, rel: p}} } diff --git a/android/paths_test.go b/android/paths_test.go index 7e7ecbdfd..00757985b 100644 --- a/android/paths_test.go +++ b/android/paths_test.go @@ -112,7 +112,10 @@ func TestValidateSafePath(t *testing.T) { for _, testCase := range validateSafePathTestCases { t.Run(strings.Join(testCase.in, ","), func(t *testing.T) { ctx := &configErrorWrapper{} - out := validateSafePath(ctx, testCase.in...) + out, err := validateSafePath(testCase.in...) + if err != nil { + reportPathError(ctx, err) + } check(t, "validateSafePath", p(testCase.in), out, ctx.errors, testCase.out, testCase.err) }) } @@ -122,7 +125,10 @@ func TestValidatePath(t *testing.T) { for _, testCase := range validatePathTestCases { t.Run(strings.Join(testCase.in, ","), func(t *testing.T) { ctx := &configErrorWrapper{} - out := validatePath(ctx, testCase.in...) + out, err := validatePath(testCase.in...) + if err != nil { + reportPathError(ctx, err) + } check(t, "validatePath", p(testCase.in), out, ctx.errors, testCase.out, testCase.err) }) } From 94a321045a743e3f0ecc0185129d2c2a3684e144 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 22 Feb 2018 14:21:02 -0800 Subject: [PATCH 4/5] Move AllowMissingDependencies check from PathsForSource to PathForSource PathsForSource was handling the AllowMissingDependencies case, but PathForSource was not. Refactor PathForSource and ExistentPathForSource, and add logic to PathForSource to fall back to behavior similar to ExistentPathForSource when AllowMissingDependencies is set. Test: paths_test.go Change-Id: Id7925999a27ea75a05e9301bbf1eb9f9a6bc4652 --- android/paths.go | 138 +++++++++++++++++++++-------------------------- 1 file changed, 62 insertions(+), 76 deletions(-) diff --git a/android/paths.go b/android/paths.go index 3d4d3f30e..3bb5d1b78 100644 --- a/android/paths.go +++ b/android/paths.go @@ -195,20 +195,6 @@ type Paths []Path // PathsForSource returns Paths rooted from SrcDir func PathsForSource(ctx PathContext, paths []string) Paths { - if ctx.Config().AllowMissingDependencies() { - if modCtx, ok := ctx.(ModuleContext); ok { - ret := make(Paths, 0, len(paths)) - for _, path := range paths { - p := ExistentPathForSource(ctx, path) - if p.Valid() { - ret = append(ret, p.Path()) - } else { - modCtx.AddMissingDependencies([]string{path}) - } - } - return ret - } - } ret := make(Paths, len(paths)) for i, path := range paths { ret[i] = PathForSource(ctx, path) @@ -507,100 +493,100 @@ func safePathForSource(ctx PathContext, path string) SourcePath { return ret } -// PathForSource joins the provided path components and validates that the result -// neither escapes the source dir nor is in the out dir. -// On error, it will return a usable, but invalid SourcePath, and report a ModuleError. -func PathForSource(ctx PathContext, pathComponents ...string) SourcePath { +// pathForSource creates a SourcePath from pathComponents, but does not check that it exists. +func pathForSource(ctx PathContext, pathComponents ...string) (SourcePath, error) { p, err := validatePath(pathComponents...) ret := SourcePath{basePath{p, ctx.Config(), ""}} if err != nil { - reportPathError(ctx, err) - return ret + return ret, err } abs, err := filepath.Abs(ret.String()) if err != nil { - reportPathError(ctx, err) - return ret + return ret, err } buildroot, err := filepath.Abs(ctx.Config().buildDir) if err != nil { - reportPathError(ctx, err) - return ret + return ret, err } if strings.HasPrefix(abs, buildroot) { - reportPathErrorf(ctx, "source path %s is in output", abs) - return ret + return ret, fmt.Errorf("source path %s is in output", abs) } - if exists, _, err := ctx.Fs().Exists(ret.String()); err != nil { - reportPathErrorf(ctx, "%s: %s", ret, err.Error()) - } else if !exists { - reportPathErrorf(ctx, "source path %s does not exist", ret) + if pathtools.IsGlob(ret.String()) { + return ret, fmt.Errorf("path may not contain a glob: %s", ret.String()) } - return ret + + return ret, nil +} + +// existsWithDependencies returns true if the path exists, and adds appropriate dependencies to rerun if the +// path does not exist. +func existsWithDependencies(ctx PathContext, path SourcePath) (exists bool, err error) { + var files []string + + if gctx, ok := ctx.(PathGlobContext); ok { + // Use glob to produce proper dependencies, even though we only want + // a single file. + files, err = gctx.GlobWithDeps(path.String(), nil) + } else { + var deps []string + // We cannot add build statements in this context, so we fall back to + // AddNinjaFileDeps + files, deps, err = pathtools.Glob(path.String(), nil) + ctx.AddNinjaFileDeps(deps...) + } + + if err != nil { + return false, fmt.Errorf("glob: %s", err.Error()) + } + + return len(files) > 0, nil +} + +// PathForSource joins the provided path components and validates that the result +// neither escapes the source dir nor is in the out dir. +// On error, it will return a usable, but invalid SourcePath, and report a ModuleError. +func PathForSource(ctx PathContext, pathComponents ...string) SourcePath { + path, err := pathForSource(ctx, pathComponents...) + if err != nil { + reportPathError(ctx, err) + } + + if modCtx, ok := ctx.(ModuleContext); ok && ctx.Config().AllowMissingDependencies() { + exists, err := existsWithDependencies(ctx, path) + if err != nil { + reportPathError(ctx, err) + } + if !exists { + modCtx.AddMissingDependencies([]string{path.String()}) + } + } else if exists, _, err := ctx.Fs().Exists(path.String()); err != nil { + reportPathErrorf(ctx, "%s: %s", path, err.Error()) + } else if !exists { + reportPathErrorf(ctx, "source path %s does not exist", path) + } + return path } // ExistentPathForSource returns an OptionalPath with the SourcePath if the // path exists, or an empty OptionalPath if it doesn't exist. Dependencies are added // so that the ninja file will be regenerated if the state of the path changes. func ExistentPathForSource(ctx PathContext, pathComponents ...string) OptionalPath { - p, err := validatePath(pathComponents...) + path, err := pathForSource(ctx, pathComponents...) if err != nil { reportPathError(ctx, err) return OptionalPath{} } - path := SourcePath{basePath{p, ctx.Config(), ""}} - abs, err := filepath.Abs(path.String()) + exists, err := existsWithDependencies(ctx, path) if err != nil { reportPathError(ctx, err) return OptionalPath{} } - buildroot, err := filepath.Abs(ctx.Config().buildDir) - if err != nil { - reportPathError(ctx, err) + if !exists { return OptionalPath{} } - if strings.HasPrefix(abs, buildroot) { - reportPathErrorf(ctx, "source path %s is in output", abs) - return OptionalPath{} - } - - if pathtools.IsGlob(path.String()) { - reportPathErrorf(ctx, "path may not contain a glob: %s", path.String()) - return OptionalPath{} - } - - if gctx, ok := ctx.(PathGlobContext); ok { - // Use glob to produce proper dependencies, even though we only want - // a single file. - files, err := gctx.GlobWithDeps(path.String(), nil) - if err != nil { - reportPathErrorf(ctx, "glob: %s", err.Error()) - return OptionalPath{} - } - - if len(files) == 0 { - return OptionalPath{} - } - } else { - // We cannot add build statements in this context, so we fall back to - // AddNinjaFileDeps - files, dirs, err := pathtools.Glob(path.String(), nil) - if err != nil { - reportPathErrorf(ctx, "glob: %s", err.Error()) - return OptionalPath{} - } - - ctx.AddNinjaFileDeps(dirs...) - - if len(files) == 0 { - return OptionalPath{} - } - - ctx.AddNinjaFileDeps(path.String()) - } return OptionalPathForPath(path) } From 9d37831dd3f5a8133c0fc353ce1993e1585aa178 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 22 Feb 2018 14:39:17 -0800 Subject: [PATCH 5/5] Use PathForSource instead of PathsForSource PathForSource does the AllowMissingDependencies check now, use it instead of PathsForSource. Test: m checkbuild Change-Id: If1894fd98d8d757ebc3c1635d5fcea86f81bfc4a --- java/droiddoc.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/java/droiddoc.go b/java/droiddoc.go index efd430ada..ac6bd3388 100644 --- a/java/droiddoc.go +++ b/java/droiddoc.go @@ -374,10 +374,8 @@ func (d *Droiddoc) GenerateAndroidBuildActions(ctx android.ModuleContext) { } // templateDir (maybe missing) is relative to top of the source tree instead of current module. - templateDir := android.PathsForSource(ctx, []string{String(d.properties.Custom_template_dir)}) - if len(templateDir) > 0 { - implicits = append(implicits, ctx.GlobFiles(filepath.Join(templateDir[0].String(), "**/*"), nil)...) - } + templateDir := android.PathForSource(ctx, String(d.properties.Custom_template_dir)).String() + implicits = append(implicits, ctx.GlobFiles(filepath.Join(templateDir, "**/*"), nil)...) var htmlDirArgs string if len(d.properties.Html_dirs) > 0 { @@ -420,7 +418,7 @@ func (d *Droiddoc) GenerateAndroidBuildActions(ctx android.ModuleContext) { opts := "-source 1.8 -J-Xmx1600m -J-XX:-OmitStackTraceInFastThrow -XDignore.symbol.file " + "-doclet com.google.doclava.Doclava -docletpath ${config.JsilverJar}:${config.DoclavaJar} " + - "-templatedir " + String(d.properties.Custom_template_dir) + " " + htmlDirArgs + " " + htmlDir2Args + " " + + "-templatedir " + templateDir + " " + htmlDirArgs + " " + htmlDir2Args + " " + "-hdf page.build " + ctx.Config().BuildId() + "-" + ctx.Config().BuildNumberFromFile() + " " + "-hdf page.now " + `"$$(date -d @$$(cat ` + ctx.Config().Getenv("BUILD_DATETIME_FILE") + `) "+%d %b %Y %k:%M")"` + " " + args + " -stubs " + android.PathForModuleOut(ctx, "docs", "stubsDir").String()