fix: all paths should be encoded over-the-wire (#7118)

* fix: all paths should be encoded over-the-wire

Somehow, this was just being done entirely incorrectly, even though I
thought I got it the first time. The path conversions are now moved to
where we directly send/recv on the wire - then we only have to have a
minimal number of conversions.

Signed-off-by: Justin Chadwell <[email protected]>

* ci: add option to list all lint errors

Signed-off-by: Justin Chadwell <[email protected]>

* fix: wrap all errors using %w when formatting

This was causing some details of important errors to be lost! While I
don't *think* this was causing any bugs right now, I encountered this
while trying to work with propagated grpc status codes.

We can enable a linting check to make sure that this doesn't happen
again.

Signed-off-by: Justin Chadwell <[email protected]>

* fix: wrap file not exists errors with codes.NotFound

Different platforms have different error messages. And of course, on
windows, you get different error messages for `Stat` and `ReadFile`,
which is *super* fun. So instead of trying to cover every single little
possible message, we should actually have the client handle this neatly.

Signed-off-by: Justin Chadwell <[email protected]>

---------

Signed-off-by: Justin Chadwell <[email protected]>

.golangci.yml

@@ -5,6 +5,7 @@ linters:
  - dogsled
  - dupl
  - exportloopref
+ - errorlint
  - gocritic
  - gocyclo
  - gofmt
@@ -46,4 +47,9 @@ linters-settings:
 
  govet:
  enable:
- - nilness
+
+ errorlint:
+ errorf: true
+ errorf-multi: false
+ asserts: false
+ comparison: false

ci/engine.go

@@ -119,17 +119,26 @@ func (e *Engine) Service(
 }
 
 // Lint the engine
-func (e *Engine) Lint(ctx context.Context) error {
+func (e *Engine) Lint(
+ ctx context.Context,
+ // +optional
+ all bool,
+) error {
  pkgs := []string{""}
  // pkgs := []string{"", "ci"}
 
  ctr := dag.Container().
  From(consts.GolangLintImage).
  WithMountedDirectory("/app", util.GoDirectory(e.Dagger.Source))
+
+ cmd := []string{"golangci-lint", "run", "-v", "--timeout", "5m"}
+ if all {
+ cmd = append(cmd, "--max-issues-per-linter=0", "--max-same-issues=0")
+ }
  for _, pkg := range pkgs {
  ctr = ctr.
  WithWorkdir(path.Join("/app", pkg)).
- WithExec([]string{"golangci-lint", "run", "-v", "--timeout", "5m"})
+ WithExec(cmd)
  }
  _, err := ctr.Sync(ctx)
  return err

cmd/dagger/module.go

@@ -602,7 +602,7 @@ func getModuleConfigurationForSourceRef(
  }
  var modCfg modules.ModuleConfig
  if err := json.Unmarshal(contents, &modCfg); err != nil {
- return nil, fmt.Errorf("failed to unmarshal %s: %s", configPath, err)
+ return nil, fmt.Errorf("failed to unmarshal %s: %w", configPath, err)
  }
 
  namedDep, ok := modCfg.DependencyByName(srcRefStr)
@@ -677,13 +677,13 @@ func findUp(curDirPath string) (string, bool, error) {
  return curDirPath, true, nil
 
  default:
- return "", false, fmt.Errorf("failed to lstat %s: %s", configPath, err)
+ return "", false, fmt.Errorf("failed to lstat %s: %w", configPath, err)
  }
 
  // didn't exist, try parent unless we've hit the root or a git repo checkout root
  curDirAbsPath, err := filepath.Abs(curDirPath)
  if err != nil {
- return "", false, fmt.Errorf("failed to get absolute path for %s: %s", curDirPath, err)
+ return "", false, fmt.Errorf("failed to get absolute path for %s: %w", curDirPath, err)
  }
  if curDirAbsPath[len(curDirAbsPath)-1] == os.PathSeparator {
  // path ends in separator, we're at root

core/modfunc.go

@@ -296,7 +296,7 @@ func (fn *ModuleFunction) Call(ctx context.Context, caller *call.ID, opts *CallO
  dec := json.NewDecoder(strings.NewReader(string(outputBytes)))
  dec.UseNumber()
  if err := dec.Decode(&returnValue); err != nil {
- return nil, fmt.Errorf("failed to unmarshal result: %s", err)
+ return nil, fmt.Errorf("failed to unmarshal result: %w", err)
  }
 
  returnValueTyped, err := fn.returnType.ConvertFromSDKResult(ctx, returnValue)

core/schema/host.go

@@ -41,11 +41,11 @@ func (s *hostSchema) Install() {
  }) (*core.Directory, error) {
  dig, err := digest.Parse(args.Digest)
  if err != nil {
- return nil, fmt.Errorf("failed to parse digest: %s", err)
+ return nil, fmt.Errorf("failed to parse digest: %w", err)
  }
  uncompressedDig, err := digest.Parse(args.Uncompressed)
  if err != nil {
- return nil, fmt.Errorf("failed to parse digest: %s", err)
+ return nil, fmt.Errorf("failed to parse digest: %w", err)
  }
  blobDef, err := blob.LLB(specs.Descriptor{
  MediaType: args.MediaType,
@@ -58,7 +58,7 @@ func (s *hostSchema) Install() {
  },
  }).Marshal(ctx)
  if err != nil {
- return nil, fmt.Errorf("failed to marshal blob source: %s", err)
+ return nil, fmt.Errorf("failed to marshal blob source: %w", err)
  }
  return core.NewDirectory(parent, blobDef.ToPB(), "/", parent.Platform, nil), nil
  }).Doc("Retrieves a content-addressed blob."),

core/schema/modulesource.go

@@ -10,6 +10,8 @@ import (
  "strings"
 
  "golang.org/x/sync/errgroup"
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
 
  "github.com/dagger/dagger/core"
  "github.com/dagger/dagger/core/modules"
@@ -638,7 +640,7 @@ func (s *moduleSchema) moduleSourceResolveFromCaller(
 
  sourceRootRelPath, err := filepath.Rel(contextAbsPath, sourceRootAbsPath)
  if err != nil {
- return inst, fmt.Errorf("failed to get source root relative path: %s", err)
+ return inst, fmt.Errorf("failed to get source root relative path: %w", err)
  }
 
  collectedDeps := dagql.NewCacheMap[string, *callerLocalDep]()
@@ -656,7 +658,7 @@ func (s *moduleSchema) moduleSourceResolveFromCaller(
  for _, rootPath := range sourceRootPaths {
  rootRelPath, err := filepath.Rel(contextAbsPath, rootPath)
  if err != nil {
- return inst, fmt.Errorf("failed to get source root relative path: %s", err)
+ return inst, fmt.Errorf("failed to get source root relative path: %w", err)
  }
  if !filepath.IsLocal(rootRelPath) {
  return inst, fmt.Errorf("local module dep source path %q escapes context %q", rootRelPath, contextAbsPath)
@@ -695,7 +697,7 @@ func (s *moduleSchema) moduleSourceResolveFromCaller(
  absPath := filepath.Join(sourceRootAbsPath, path)
  relPath, err := filepath.Rel(contextAbsPath, absPath)
  if err != nil {
- return fmt.Errorf("failed to get relative path of config include/exclude: %s", err)
+ return fmt.Errorf("failed to get relative path of config include/exclude: %w", err)
  }
  if !filepath.IsLocal(relPath) {
  return fmt.Errorf("local module dep source include/exclude path %q escapes context %q", relPath, contextAbsPath)
@@ -720,7 +722,7 @@ func (s *moduleSchema) moduleSourceResolveFromCaller(
  // always include the config file
  configRelPath, err := filepath.Rel(contextAbsPath, filepath.Join(rootPath, modules.Filename))
  if err != nil {
- return inst, fmt.Errorf("failed to get relative path: %s", err)
+ return inst, fmt.Errorf("failed to get relative path: %w", err)
  }
  includeSet[configRelPath] = struct{}{}
 
@@ -732,7 +734,7 @@ func (s *moduleSchema) moduleSourceResolveFromCaller(
  sourceAbsSubpath := filepath.Join(rootPath, source)
  sourceRelSubpath, err := filepath.Rel(contextAbsPath, sourceAbsSubpath)
  if err != nil {
- return inst, fmt.Errorf("failed to get relative path: %s", err)
+ return inst, fmt.Errorf("failed to get relative path: %w", err)
  }
  if !filepath.IsLocal(sourceRelSubpath) {
  return inst, fmt.Errorf("local module source path %q escapes context %q", sourceRelSubpath, contextAbsPath)
@@ -909,7 +911,7 @@ func (s *moduleSchema) collectCallerLocalDeps(
  _, _, err := collectedDeps.GetOrInitialize(ctx, sourceRootAbsPath, func(ctx context.Context) (*callerLocalDep, error) {
  sourceRootRelPath, err := filepath.Rel(contextAbsPath, sourceRootAbsPath)
  if err != nil {
- return nil, fmt.Errorf("failed to get source root relative path: %s", err)
+ return nil, fmt.Errorf("failed to get source root relative path: %w", err)
  }
  if !filepath.IsLocal(sourceRootRelPath) {
  return nil, fmt.Errorf("local module dep source path %q escapes context %q", sourceRootRelPath, contextAbsPath)
@@ -921,10 +923,12 @@ func (s *moduleSchema) collectCallerLocalDeps(
  switch {
  case err == nil:
  if err := json.Unmarshal(configBytes, &modCfg); err != nil {
- return nil, fmt.Errorf("error unmarshaling config at %s: %s", configPath, err)
+ return nil, fmt.Errorf("error unmarshaling config at %s: %w", configPath, err)
  }
 
- case strings.Contains(err.Error(), "no such file or directory") || strings.Contains(err.Error(), "not found"):
+ // TODO: remove the strings.Contains check here (which aren't cross-platform),
+ // since we now set NotFound (since v0.11.2)
+ case status.Code(err) == codes.NotFound || strings.Contains(err.Error(), "no such file or directory"):
  // This is only allowed for the top-level module (which may be in the process of being newly initialized).
  // sentinel via nil modCfg unless there's WithSDK/WithDependencies/etc. to be applied
  if !topLevel {
@@ -935,7 +939,7 @@ func (s *moduleSchema) collectCallerLocalDeps(
  }
 
  default:
- return nil, fmt.Errorf("error reading config %s: %s", configPath, err)
+ return nil, fmt.Errorf("error reading config %s: %w", configPath, err)
  }
 
  if topLevel {
@@ -1004,7 +1008,7 @@ func (s *moduleSchema) collectCallerLocalDeps(
  callerCwd := callerCwdStat.Path
  sdkCallerRelPath, err := filepath.Rel(callerCwd, sdkPath)
  if err != nil {
- return nil, fmt.Errorf("failed to get relative path of local sdk: %s", err)
+ return nil, fmt.Errorf("failed to get relative path of local sdk: %w", err)
  }
  var sdkMod dagql.Instance[*core.Module]
  err = s.dag.Select(ctx, s.dag.Root(), &sdkMod,
@@ -1061,8 +1065,10 @@ func callerHostFindUpContext(
  if err == nil {
  return curDirPath, true, nil
  }
- if !strings.Contains(err.Error(), "no such file or directory") && !strings.Contains(err.Error(), "not found") {
- return "", false, fmt.Errorf("failed to lstat .git: %s", err)
+ // TODO: remove the strings.Contains check here (which aren't cross-platform),
+ // since we now set NotFound (since v0.11.2)
+ if status.Code(err) != codes.NotFound && !strings.Contains(err.Error(), "no such file or directory") {
+ return "", false, fmt.Errorf("failed to lstat .git: %w", err)
  }
 
  nextDirPath := filepath.Dir(curDirPath)

core/schema/query.go

@@ -89,13 +89,13 @@ func (s *querySchema) checkVersionCompatibility(ctx context.Context, _ *core.Que
  engineVersionStr := strings.TrimPrefix(engine.Version, "v")
  engineVersion, err := semver.Parse(engineVersionStr)
  if err != nil {
- return false, fmt.Errorf("failed to parse engine version as semver: %s", err)
+ return false, fmt.Errorf("failed to parse engine version as semver: %w", err)
  }
 
  sdkVersionStr := strings.TrimPrefix(args.Version, "v")
  sdkVersion, err := semver.Parse(sdkVersionStr)
  if err != nil {
- return false, fmt.Errorf("failed to parse SDK version as semver: %s", err)
+ return false, fmt.Errorf("failed to parse SDK version as semver: %w", err)
  }
 
  // If the Engine is a major version above the SDK version, fails

engine/buildkit/blob.go

@@ -32,7 +32,7 @@ func (c *Client) DefToBlob(
  }
  resultProxy, err := res.SingleRef()
  if err != nil {
- return nil, desc, fmt.Errorf("failed to get single ref: %s", err)
+ return nil, desc, fmt.Errorf("failed to get single ref: %w", err)
  }
  cachedRes, err := resultProxy.Result(ctx)
  if err != nil {
@@ -51,7 +51,7 @@ func (c *Client) DefToBlob(
  // is tricky and ultimately only result in a marginal performance optimization.
  err = ref.Extract(ctx, nil)
  if err != nil {
- return nil, desc, fmt.Errorf("failed to extract ref: %s", err)
+ return nil, desc, fmt.Errorf("failed to extract ref: %w", err)
  }
 
  remotes, err := ref.GetRemotes(ctx, true, cacheconfig.RefConfig{
@@ -64,7 +64,7 @@ func (c *Client) DefToBlob(
  },
  }, false, nil)
  if err != nil {
- return nil, desc, fmt.Errorf("failed to get remotes: %s", err)
+ return nil, desc, fmt.Errorf("failed to get remotes: %w", err)
  }
  if len(remotes) != 1 {
  return nil, desc, fmt.Errorf("expected 1 remote, got %d", len(remotes))
@@ -78,7 +78,7 @@ func (c *Client) DefToBlob(
 
  blobDef, err := blob.LLB(desc).Marshal(ctx)
  if err != nil {
- return nil, desc, fmt.Errorf("failed to marshal blob source: %s", err)
+ return nil, desc, fmt.Errorf("failed to marshal blob source: %w", err)
  }
  blobPB := blobDef.ToPB()
 

engine/buildkit/client.go

@@ -497,14 +497,14 @@ func (c *Client) UpstreamCacheExport(ctx context.Context, cacheExportFuncs []Res
  }
  cacheRes, err := ConvertToWorkerCacheResult(ctx, combinedResult)
  if err != nil {
- return fmt.Errorf("failed to convert result: %s", err)
+ return fmt.Errorf("failed to convert result: %w", err)
  }
  bklog.G(ctx).Debugf("converting to solverRes")
  solverRes, err := solverresult.ConvertResult(combinedResult, func(rf *ref) (bksolver.CachedResult, error) {
  return rf.resultProxy.Result(ctx)
  })
  if err != nil {
- return fmt.Errorf("failed to convert result: %s", err)
+ return fmt.Errorf("failed to convert result: %w", err)
  }
 
  sessionGroup := bksession.NewGroup(c.ID())
@@ -580,13 +580,13 @@ func (c *Client) ListenHostToContainer(
  clientMetadata, err := engine.ClientMetadataFromContext(ctx)
  if err != nil {
  cancel()
- return nil, nil, fmt.Errorf("failed to get requester session ID: %s", err)
+ return nil, nil, fmt.Errorf("failed to get requester session ID: %w", err)
  }
 
  clientCaller, err := c.SessionManager.Get(ctx, clientMetadata.ClientID, false)
  if err != nil {
  cancel()
- return nil, nil, fmt.Errorf("failed to get requester session: %s", err)
+ return nil, nil, fmt.Errorf("failed to get requester session: %w", err)
  }
 
  conn := clientCaller.Conn()
@@ -596,7 +596,7 @@ func (c *Client) ListenHostToContainer(
  listener, err := tunnelClient.Listen(ctx)
  if err != nil {
  cancel()
- return nil, nil, fmt.Errorf("failed to listen: %s", err)
+ return nil, nil, fmt.Errorf("failed to listen: %w", err)
  }
 
  err = listener.Send(&session.ListenRequest{
@@ -605,13 +605,13 @@ func (c *Client) ListenHostToContainer(
  })
  if err != nil {
  cancel()
- return nil, nil, fmt.Errorf("failed to send listen request: %s", err)
+ return nil, nil, fmt.Errorf("failed to send listen request: %w", err)
  }
 
  listenRes, err := listener.Recv()
  if err != nil {
  cancel()
- return nil, nil, fmt.Errorf("failed to receive listen response: %s", err)
+ return nil, nil, fmt.Errorf("failed to receive listen response: %w", err)
  }
 
  conns := map[string]net.Conn{}