Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSDK-9721: Improve firstRun error message for missing meta.json. #4726

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

RSDK-9721: Improve firstRun error message for missing meta.json. #4726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions config/module.go
View file
Open in desktop
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"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stacks are coming from my favorite library. I haven't 100% verified I used the corollary methods in "stderrors" properly. Particularly the Join/As/Is. I'm leaning on unit tests making the appropriate assertions.

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 @@ -141,7 +140,7 @@ func (m Module) NeedsSyntheticPackage() bool {
// SyntheticPackage creates a fake package for a module which can be used to access some package logic.
func (m Module) SyntheticPackage() (PackageConfig, error) {
if m.Type != ModuleTypeLocal {
return PackageConfig{}, errors.New("SyntheticPackage only works on local modules")
return PackageConfig{}, fmt.Errorf("SyntheticPackage only works on local modules")
}
var name string
if m.NeedsSyntheticPackage() {
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this was intentional. Or maybe it was. But it's certainly a bit odd to concatenate a "directory" with a "file suffix" without some "file prefix".

I started to "fix" this by using filepath.Join. And additionally being "backwards compatible" by checking for both bin.first_run_succeeded as well as bin/.first_run_succeeded.

In the end I figured it's not worth risking breaking this code for aesthetics. But found it useful to document a concrete example such that readers aren't left wondering if something is wrong.

// .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")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I waffled on whether this case should also be writing out a "first run succeeded" file. I opted not to -- just in case we ever had a bug with getJSONManifest incorrectly missing a file that it should be seeing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was wrapping errors how we failed this check?

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
15 changes: 7 additions & 8 deletions utils/variables.go
View file
Open in desktop
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"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My "find definition" jumping while looking at module.go brought me here

)

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
Loading