From 45245cedfc897061f483fd5e8389123fe8cf8cb8 Mon Sep 17 00:00:00 2001 From: Oliver Newman Date: Wed, 4 Oct 2023 16:19:50 +0000 Subject: [PATCH] [engine] Improve error messages when run in invalid directory 1. --var flags are not validated until after shac determines that the working directory is valid (i.e. contains a shac.star file). 2. If a shac.star file cannot be discovered via git, print the expected location of the shac.star file. 3. If a --var flag is specified but no shac.textproto file exists, use a slightly different error message to make it more clear that shac.textproto doesn't exist yet. Change-Id: I6bed59f60b73e9ecff0caec97c152f2f7ea94fb7 Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/926852 Reviewed-by: Anthony Fandrianto Fuchsia-Auto-Submit: Oliver Newman Commit-Queue: Auto-Submit --- internal/engine/run.go | 58 +++++++++++++++++++++++++++---------- internal/engine/run_test.go | 41 +++++++++++++++++++++----- internal/engine/version.go | 2 +- 3 files changed, 77 insertions(+), 24 deletions(-) diff --git a/internal/engine/run.go b/internal/engine/run.go index 827c489..4901e3e 100644 --- a/internal/engine/run.go +++ b/internal/engine/run.go @@ -224,7 +224,9 @@ func runInner(ctx context.Context, o *Options, tmpdir string) error { } var b []byte doc := Document{} + configExists := false if b, err = os.ReadFile(absConfig); err == nil { + configExists = true // First parse the config file ignoring unknown fields and check only // min_shac_version, so users get an "unsupported version" error if they // set fields that are only available in a later version of shac (as @@ -277,17 +279,6 @@ func runInner(ctx context.Context, o *Options, tmpdir string) error { scm = &cachingSCM{scm: scm} } - vars := make(map[string]string) - for _, v := range doc.Vars { - vars[v.Name] = v.Default - } - for name, value := range o.Vars { - if _, ok := vars[name]; !ok { - return fmt.Errorf("var not declared in %s: %s", config, name) - } - vars[name] = value - } - pkgMgr := NewPackageManager(tmpdir) packages, err := pkgMgr.RetrievePackages(ctx, root, &doc) if err != nil { @@ -304,7 +295,28 @@ func runInner(ctx context.Context, o *Options, tmpdir string) error { packages: packages, } - newState := func(scm scmCheckout, subdir string, idx int) *shacState { + var vars map[string]string + + newState := func(scm scmCheckout, subdir string, idx int) (*shacState, error) { + // Lazy-load vars only once a shac.star file is detected, so that errors + // about missing shac.star files are prioritized over var validation + // errors. + if vars == nil { + vars = make(map[string]string) + for _, v := range doc.Vars { + vars[v.Name] = v.Default + } + for name, value := range o.Vars { + if _, ok := vars[name]; !ok { + if configExists { + return nil, fmt.Errorf("var not declared in %s: %s", config, name) + } + return nil, fmt.Errorf("var must be declared in a %s file: %s", config, name) + } + vars[name] = value + } + } + if subdir != "" { normalized := subdir + "/" if subdir == "." { @@ -327,7 +339,7 @@ func runInner(ctx context.Context, o *Options, tmpdir string) error { writableRoot: doc.WritableRoot, vars: vars, passthroughEnv: doc.PassthroughEnv, - } + }, nil } var shacStates []*shacState if o.Recurse { @@ -363,13 +375,27 @@ func runInner(ctx context.Context, o *Options, tmpdir string) error { } } if len(subdirs) == 0 { - return fmt.Errorf("no %s files found", entryPoint) + return fmt.Errorf("no %s files found in %s", entryPoint, root) } for i, s := range subdirs { - shacStates = append(shacStates, newState(scm, s, i)) + state, err := newState(scm, s, i) + if err != nil { + return err + } + shacStates = append(shacStates, state) } } else { - shacStates = []*shacState{newState(scm, "", 0)} + if _, err := os.Stat(filepath.Join(root, entryPoint)); err != nil { + if errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("no %s file in repository root: %s", entryPoint, root) + } + return err + } + state, err := newState(scm, "", 0) + if err != nil { + return err + } + shacStates = []*shacState{state} } // Parse the starlark files. Run everything from our errgroup. diff --git a/internal/engine/run_test.go b/internal/engine/run_test.go index 4f20284..da01076 100644 --- a/internal/engine/run_test.go +++ b/internal/engine/run_test.go @@ -113,19 +113,32 @@ func TestRun_Fail(t *testing.T) { { "no shac.star file", Options{ - Dir: t.TempDir(), + Dir: scratchDir, + EntryPoint: "entrypoint.star", }, - // TODO(olivernewman): This error should be more specific, like "no - // shac.star file found". - "shac.star not found", + fmt.Sprintf("no entrypoint.star file in repository root: %s", scratchDir), + }, + { + "no shac.star file and invalid vars", + Options{ + Dir: scratchDir, + EntryPoint: "entrypoint.star", + Vars: map[string]string{ + "this_is_an_invalid_var": "foo", + }, + }, + // Missing entrypoint file errors should be prioritized over invalid + // var errors. + fmt.Sprintf("no entrypoint.star file in repository root: %s", scratchDir), }, { "no shac.star files (recursive)", Options{ - Dir: t.TempDir(), - Recurse: true, + Dir: scratchDir, + Recurse: true, + EntryPoint: "entrypoint.star", }, - "no shac.star files found", + fmt.Sprintf("no entrypoint.star files found in %s", scratchDir), }, { "nonexistent directory", @@ -153,6 +166,20 @@ func TestRun_Fail(t *testing.T) { }, "var not declared in shac.textproto: unknown_var", }, + { + "invalid var with no config file", + func() Options { + root := t.TempDir() + writeFile(t, root, "shac.star", ``) + return Options{ + Dir: root, + Vars: map[string]string{ + "unknown_var": "", + }, + } + }(), + "var must be declared in a shac.textproto file: unknown_var", + }, } for i := range data { i := i diff --git a/internal/engine/version.go b/internal/engine/version.go index ef48033..9a5ab01 100644 --- a/internal/engine/version.go +++ b/internal/engine/version.go @@ -26,7 +26,7 @@ var ( // Version is the current tool version. // // TODO(maruel): Add proper version, preferably from git tag. - Version = shacVersion{0, 1, 10} + Version = shacVersion{0, 1, 11} ) func (v shacVersion) String() string {