Merge branch 'main' of ssh://github.com/viamrobotics/rdk into sean-pr

.github/workflows/docker.yml

@@ -76,7 +76,7 @@ jobs:
 
   test:
     needs: docker-cache
-    uses: viamrobotics/rdk/.github/workflows/test.yml@main
+    uses: ./.github/workflows/test.yml
     secrets:
       MONGODB_TEST_OUTPUT_URI: ${{ secrets.MONGODB_TEST_OUTPUT_URI }}
       DOCKER_PUBLIC_READONLY_PAT: ${{ secrets.DOCKER_PUBLIC_READONLY_PAT }}

.github/workflows/test.yml

@@ -73,7 +73,7 @@ jobs:
       - name: Run go unit tests
         run: |
           chmod -R a+rwx . # temporary fix for arm runners
-          sudo apt-get install -y python3-venv
+          sudo apt-get update && sudo apt-get install -y python3-venv
           sudo --preserve-env=MONGODB_TEST_OUTPUT_URI,GITHUB_SHA,GITHUB_RUN_ID,GITHUB_RUN_NUMBER,GITHUB_RUN_ATTEMPT,GITHUB_X_PR_BASE_SHA,GITHUB_X_PR_BASE_REF,GITHUB_X_HEAD_REF,GITHUB_X_HEAD_SHA,GITHUB_REPOSITORY -Hu testbot bash -lc 'make test-go'
 
       - name: Upload test.json
@@ -108,7 +108,7 @@ jobs:
             --platform linux/arm/v7 \
             -v `pwd`:/rdk \
             ghcr.io/viamrobotics/rdk-devenv:armhf-cache \
-            sudo -Hu testbot bash -lc 'sudo apt-get install -y python3-venv && cd /rdk && go test -v ./...'
+            sudo -Hu testbot bash -lc 'sudo apt-get update && sudo apt-get install -y python3-venv && cd /rdk && go test -v ./...'
 
   motion_tests:
     name: Test Longer-running Motion Plans if affected

cli/app.go

@@ -1893,7 +1893,6 @@ var app = &cli.App{
 							Name:      "status",
 							Usage:     "display part status",
 							UsageText: createUsageText("machines part status", []string{generalFlagPart}, true, false),
-							// TODO(RSDK-9286) do we need to ask for og and location and machine and part here?
 							Flags: []cli.Flag{
 								&AliasStringFlag{
 									cli.StringFlag{

cli/module_generate.go

@@ -70,6 +70,9 @@ func GenerateModuleAction(cCtx *cli.Context, args generateModuleArgs) error {
 }
 
 func (c *viamClient) generateModuleAction(cCtx *cli.Context, args generateModuleArgs) error {
+	if err := c.ensureLoggedIn(); err != nil {
+		return err
+	}
 	var newModule *modulegen.ModuleInputs
 	var err error
 
@@ -207,10 +210,14 @@ func promptUser(module *modulegen.ModuleInputs) error {
 		}
 		// we differentiate generic-service and generic-component in `modulegen.Resources`
 		// but they still have the type listed. This carveout prevents the user prompt from
-		// suggesting `Generic Component Component` or `Generic Service Service` as an option
+		// suggesting `Generic Component Component` or `Generic Service Service` as an option,
+		// either visually or under the hood
 		var resType string
 		if words[0] == "Generic" {
 			resType = strings.Join(words[:2], " ")
+			// specific carveout to ensure that the `resource` is either `generic service` or
+			// `generic component`, as opposed to `generic_service service`
+			resource = strings.ToLower(resType)
 		} else {
 			resType = strings.Join(words, " ")
 		}
@@ -322,6 +329,12 @@ func wrapResolveOrg(cCtx *cli.Context, c *viamClient, newModule *modulegen.Modul
 	return nil
 }
 
+// TODO(RSDK-9758) - this logic will never be relevant currently because we're now checking if
+// we're logged in at the first opportunity in `viam module generate`, and returning an error if
+// not. However, I (ethan) am leaving this logic here because we will likely want to revisit if
+// and how to use it more broadly (not just for `viam module generate` but for _all_ CLI commands),
+// and because disentangling it immediately may be complicated and delay the current attempt to
+// solve the problems this causes (see RSDK-9452).
 func catchResolveOrgErr(cCtx *cli.Context, c *viamClient, newModule *modulegen.ModuleInputs, caughtErr error) error {
 	if strings.Contains(caughtErr.Error(), "not logged in") || strings.Contains(caughtErr.Error(), "error while refreshing token") {
 		originalWriter := cCtx.App.Writer

config/module.go

@@ -4,7 +4,7 @@ import (
 	"bufio"
 	"context"
 	"encoding/json"
-	stderrors "errors"
+	"errors"
 	"fmt"
 	"os"
 	"os/exec"
@@ -15,7 +15,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/pkg/errors"
 	goutils "go.viam.com/utils"
 
 	"go.viam.com/rdk/logging"
@@ -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)
 		}
 	}
 
@@ -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
@@ -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)
@@ -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 {
@@ -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")
@@ -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)
@@ -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)
 				}
 			}
 
@@ -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)
 			}
 		}
 
@@ -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)
 			}
 		}
 
@@ -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")

config/module_test.go

@@ -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) {

go.mod

@@ -77,7 +77,7 @@ require (
 	go.uber.org/zap v1.27.0
 	go.viam.com/api v0.1.380
 	go.viam.com/test v1.2.4
-	go.viam.com/utils v0.1.123
+	go.viam.com/utils v0.1.126
 	goji.io v2.0.2+incompatible
 	golang.org/x/image v0.19.0
 	golang.org/x/mobile v0.0.0-20240112133503-c713f31d574b

go.sum

@@ -1517,8 +1517,8 @@ go.viam.com/api v0.1.380 h1:VgRHDlPBku+kjIp4omxmPNmRVZezytFUUOFJ2xpRFR8=
 go.viam.com/api v0.1.380/go.mod h1:g5eipXHNm0rQmW7DWya6avKcmzoypLmxnMlAaIsE5Ls=
 go.viam.com/test v1.2.4 h1:JYgZhsuGAQ8sL9jWkziAXN9VJJiKbjoi9BsO33TW3ug=
 go.viam.com/test v1.2.4/go.mod h1:zI2xzosHdqXAJ/kFqcN+OIF78kQuTV2nIhGZ8EzvaJI=
-go.viam.com/utils v0.1.123 h1:0nxG3Rp9MmFn+qFbPQ4qSptz+hvm9MENbPXvKUBgRqU=
-go.viam.com/utils v0.1.123/go.mod h1:g1CaEkf7aJCrSI/Sfkx+6cBS1+Y3fPT2pvMQbp7TTBI=
+go.viam.com/utils v0.1.126 h1:ecFlzln5/u1NqzVMOVxwgwbkg4dDWvQmcCS2fMg0ZNU=
+go.viam.com/utils v0.1.126/go.mod h1:g1CaEkf7aJCrSI/Sfkx+6cBS1+Y3fPT2pvMQbp7TTBI=
 go4.org/unsafe/assume-no-moving-gc v0.0.0-20230525183740-e7c30c78aeb2 h1:WJhcL4p+YeDxmZWg141nRm7XC8IDmhz7lk5GpadO1Sg=
 go4.org/unsafe/assume-no-moving-gc v0.0.0-20230525183740-e7c30c78aeb2/go.mod h1:FftLjUGFEDu5k8lt0ddY+HcrH/qU/0qk+H8j9/nTl3E=
 gocv.io/x/gocv v0.25.0/go.mod h1:Rar2PS6DV+T4FL+PM535EImD/h13hGVaHhnCu1xarBs=

robot/impl/resource_manager_test.go

@@ -15,7 +15,6 @@ import (
 	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/grpcreflect"
 	"go.mongodb.org/mongo-driver/bson/primitive"
-	"go.uber.org/zap/zapcore"
 	armpb "go.viam.com/api/component/arm/v1"
 	basepb "go.viam.com/api/component/base/v1"
 	boardpb "go.viam.com/api/component/board/v1"
@@ -1581,7 +1580,7 @@ func TestReconfigure(t *testing.T) {
 }
 
 func TestRemoteConnClosedOnReconfigure(t *testing.T) {
-	logger, observer := logging.NewObservedTestLogger(t)
+	logger := logging.NewTestLogger(t)
 
 	ctx := context.Background()
 
@@ -1679,10 +1678,6 @@ func TestRemoteConnClosedOnReconfigure(t *testing.T) {
 		test.That(t, err, test.ShouldBeNil)
 		test.That(t, moving, test.ShouldBeFalse)
 		test.That(t, speed, test.ShouldEqual, 0.0)
-
-		// Also check that there are no error logs associated with the main robot trying to reconnect to remote2
-		// Leaked remote connections will cause the test to fail due to goroutine leaks
-		test.That(t, observer.FilterLevelExact(zapcore.ErrorLevel).Len(), test.ShouldEqual, 0)
 	})
 
 	t.Run("remotes with different resources", func(t *testing.T) {
@@ -1755,10 +1750,6 @@ func TestRemoteConnClosedOnReconfigure(t *testing.T) {
 		moving, err = arm1.IsMoving(ctx)
 		test.That(t, err, test.ShouldBeNil)
 		test.That(t, moving, test.ShouldBeFalse)
-
-		// Also check that there are no error logs associated with the main robot trying to reconnect to remote2
-		// Leaked remote connections will cause the test to fail due to goroutine leaks
-		test.That(t, observer.FilterLevelExact(zapcore.ErrorLevel).Len(), test.ShouldEqual, 0)
 	})
 }
 

utils/variables.go

@@ -1,11 +1,10 @@
 package utils
 
 import (
+	"fmt"
 	"reflect"
 	"regexp"
 	"strings"
-
-	"github.com/pkg/errors"
 )
 
 var (
@@ -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
 }
@@ -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
 }