[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 <atyfto@google.com>
Fuchsia-Auto-Submit: Oliver Newman <olivernewman@google.com>
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
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 @@
}
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 @@
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 @@
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 @@
writableRoot: doc.WritableRoot,
vars: vars,
passthroughEnv: doc.PassthroughEnv,
- }
+ }, nil
}
var shacStates []*shacState
if o.Recurse {
@@ -363,13 +375,27 @@
}
}
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 @@
{
"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 @@
},
"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 @@
// 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 {
