Skip to content

Commit

Permalink
RSDK-9721: Improve firstRun error message for missing meta.json. (#4726)
Browse files Browse the repository at this point in the history
  • Loading branch information
dgottlieb authored Jan 21, 2025
1 parent d4a0e86 commit 1a27697
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 25 deletions.
36 changes: 20 additions & 16 deletions config/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"bufio"
"context"
"encoding/json"
stderrors "errors"
"errors"
"fmt"
"os"
"os/exec"
Expand All @@ -15,7 +15,6 @@ import (
"sync"
"time"

"github.com/pkg/errors"
goutils "go.viam.com/utils"

"go.viam.com/rdk/logging"
Expand Down Expand Up @@ -104,7 +103,7 @@ func (m *Module) validate(path string) error {
if m.Type == ModuleTypeLocal {
_, err := os.Stat(m.ExePath)
if err != nil {
return errors.Wrapf(err, "module %s executable path error", path)
return fmt.Errorf("module %s executable path error: %w", path, err)
}
}

Expand All @@ -113,7 +112,7 @@ func (m *Module) validate(path string) error {
}

if m.Name == reservedModuleName {
return errors.Errorf("module %s cannot use the reserved name of %s", path, reservedModuleName)
return fmt.Errorf("module %s cannot use the reserved name of %s", path, reservedModuleName)
}

return nil
Expand Down Expand Up @@ -168,7 +167,7 @@ func (m Module) exeDir(packagesDir string) (string, error) {
func parseJSONFile[T any](path string) (*T, error) {
f, err := os.Open(path) //nolint:gosec
if err != nil {
return nil, errors.Wrap(err, "reading json file")
return nil, fmt.Errorf("reading json file: %w", err)
}
var target T
err = json.NewDecoder(f).Decode(&target)
Expand Down Expand Up @@ -223,7 +222,7 @@ func (m Module) EvaluateExePath(packagesDir string) (string, error) {
meta, err := parseJSONFile[JSONManifest](metaPath)
if err != nil {
// note: this error deprecates the side-by-side case because the side-by-side case is deprecated.
return "", errors.Wrapf(err, "couldn't find meta.json inside tarball %s (or next to it)", m.ExePath)
return "", fmt.Errorf("couldn't find meta.json inside tarball %s (or next to it): %w", m.ExePath, err)
}
entrypoint, err := utils.SafeJoinDir(exeDir, meta.Entrypoint)
if err != nil {
Expand Down Expand Up @@ -258,8 +257,13 @@ func (m *Module) FirstRun(
return err
}

// check if FirstRun already ran successfully for this module version by
// checking if a success marker file exists on disk.
// check if FirstRun already ran successfully for this module version by checking if a success
// marker file exists on disk. An example module directory structure:
//
// .viam/packages/data/module/e76d1b3b-0468-4efd-bb7f-fb1d2b352fcb-viamrtsp-0_1_0-linux-amd64/
// .viam/packages/data/module/e76d1b3b-0468-4efd-bb7f-fb1d2b352fcb-viamrtsp-0_1_0-linux-amd64/bin/
// .viam/packages/data/module/e76d1b3b-0468-4efd-bb7f-fb1d2b352fcb-viamrtsp-0_1_0-linux-amd64/bin.first_run_succeeded
// .viam/packages/data/module/e76d1b3b-0468-4efd-bb7f-fb1d2b352fcb-viamrtsp-0_1_0-linux-amd64/bin/viamrtsp
firstRunSuccessPath := unpackedModDir + FirstRunSuccessSuffix
if _, err := os.Stat(firstRunSuccessPath); !errors.Is(err, os.ErrNotExist) {
logger.Info("first run already ran")
Expand All @@ -272,7 +276,7 @@ func (m *Module) FirstRun(
var pathErr *os.PathError
switch {
case errors.As(err, &pathErr):
logger.Debugw("meta.json not found, skipping first run", "error", err)
logger.Infow("meta.json does not exist, skipping first run")
return nil
case err != nil:
logger.Warnw("failed to parse meta.json, skipping first run", "error", err)
Expand Down Expand Up @@ -404,7 +408,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*
if registryErr != nil {
// return from getJSONManifest() if the error returned does NOT indicate that the file wasn't found
if !os.IsNotExist(registryErr) {
return nil, "", errors.Wrap(registryErr, "registry module")
return nil, "", fmt.Errorf("registry module: %w", registryErr)
}
}

Expand All @@ -425,10 +429,10 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*
if registryTarballErr != nil {
if !os.IsNotExist(registryTarballErr) {
if online {
return nil, "", errors.Wrap(registryTarballErr, "registry module")
return nil, "", fmt.Errorf("registry module: %w", registryTarballErr)
}

return nil, "", errors.Wrap(registryTarballErr, "local tarball")
return nil, "", fmt.Errorf("local tarball: %w", registryTarballErr)
}
}

Expand All @@ -449,7 +453,7 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*
meta, localTarballErr = findMetaJSONFile(exeDir)
if localTarballErr != nil {
if !os.IsNotExist(localTarballErr) {
return nil, "", errors.Wrap(localTarballErr, "local tarball")
return nil, "", fmt.Errorf("local tarball: %w", localTarballErr)
}
}

Expand All @@ -460,14 +464,14 @@ func (m Module) getJSONManifest(unpackedModDir string, env map[string]string) (*

if online {
if !ok {
return nil, "", errors.Wrap(registryTarballErr, "registry module: failed to find meta.json. VIAM_MODULE_ROOT not set")
return nil, "", fmt.Errorf("registry module: failed to find meta.json. VIAM_MODULE_ROOT not set: %w", registryTarballErr)
}

return nil, "", errors.Wrap(stderrors.Join(registryErr, registryTarballErr), "registry module: failed to find meta.json")
return nil, "", fmt.Errorf("registry module: failed to find meta.json: %w", errors.Join(registryErr, registryTarballErr))
}

if !localNonTarball {
return nil, "", errors.Wrap(stderrors.Join(registryTarballErr, localTarballErr), "local tarball: failed to find meta.json")
return nil, "", fmt.Errorf("local tarball: failed to find meta.json: %w", errors.Join(registryTarballErr, localTarballErr))
}

return nil, "", errors.New("local non-tarball: did not search for meta.json")
Expand Down
2 changes: 1 addition & 1 deletion config/module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func TestRegistryModuleFirstRun(t *testing.T) {

err := module.FirstRun(ctx, localPackagesDir, dataDir, env, logger)
test.That(t, err, test.ShouldBeNil)
test.That(t, observedLogs.FilterMessage("meta.json not found, skipping first run").Len(), test.ShouldEqual, 1)
test.That(t, observedLogs.FilterMessage("meta.json does not exist, skipping first run").Len(), test.ShouldEqual, 1)
})

t.Run("MetaFileInvalid", func(t *testing.T) {
Expand Down
15 changes: 7 additions & 8 deletions utils/variables.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
package utils

import (
"fmt"
"reflect"
"regexp"
"strings"

"github.com/pkg/errors"
)

var (
Expand All @@ -24,10 +23,10 @@ var (
// ValidateResourceName validates that the resource follows our naming requirements.
func ValidateResourceName(name string) error {
if len(name) > 60 {
return errors.Errorf("name %q must be 60 characters or fewer", name)
return fmt.Errorf("name %q must be 60 characters or fewer", name)
}
if !validResourceNameRegex.MatchString(name) {
return errors.Errorf("name %q %s", name, validResourceNameExplanation)
return fmt.Errorf("name %q %s", name, validResourceNameExplanation)
}
return nil
}
Expand All @@ -37,21 +36,21 @@ func ValidateResourceName(name string) error {
// accepts valid socket paths.
func ValidateModuleName(name string) error {
if len(name) > 200 {
return errors.Errorf("module name %q must be 200 characters or fewer", name)
return fmt.Errorf("module name %q must be 200 characters or fewer", name)
}
if !validResourceNameRegex.MatchString(name) {
return errors.Errorf("module name %q %s", name, validResourceNameExplanation)
return fmt.Errorf("module name %q %s", name, validResourceNameExplanation)
}
return nil
}

// ValidatePackageName validates that the package follows our naming requirements.
func ValidatePackageName(name string) error {
if len(name) > 200 {
return errors.Errorf("package name %q must be 200 characters or fewer", name)
return fmt.Errorf("package name %q must be 200 characters or fewer", name)
}
if !validResourceNameRegex.MatchString(name) {
return errors.Errorf("package name %q %s", name, validResourceNameExplanation)
return fmt.Errorf("package name %q %s", name, validResourceNameExplanation)
}
return nil
}
Expand Down

0 comments on commit 1a27697

Please sign in to comment.