Repository: jetpack-io/devbox
Pull request #1826 (2024-02-21 01:21:40)

[UX] Show add (nix build) stdout
## Summary

I mostly copied over what we have from `nix.ProfileInstall`. I thinks this is OK? It will pipe all output and errors to stderr so user can see progress and if anything breaks it will show up there as well.

## How was it tested?

Tried installing something slow:
<img width="756" alt="image" src="https://github.com/jetpack-io/devbox/assets/544948/bab51a0a-b167-4a9d-ae95-b2b1a9f2179a">




Patch:
diff --git a/internal/devbox/packages.go b/internal/devbox/packages.go
index af4ced31..7ead5468 100644
--- a/internal/devbox/packages.go
+++ b/internal/devbox/packages.go
@@ -443,7 +443,8 @@ func (d *Devbox) installNixPackagesToStore(ctx context.Context) error {
 		args := &nix.BuildArgs{
 			AllowInsecure: pkg.HasAllowInsecure(),
 			// --no-link to avoid generating the result objects
-			Flags: []string{"--no-link"},
+			Flags:  []string{"--no-link"},
+			Writer: d.stderr,
 		}
 		err = nix.Build(ctx, args, installable)
 		if err != nil {
diff --git a/internal/devbox/update.go b/internal/devbox/update.go
index ff55932a..5ead3b46 100644
--- a/internal/devbox/update.go
+++ b/internal/devbox/update.go
@@ -149,7 +149,7 @@ func (d *Devbox) mergeResolvedPackageToLockfile(
 				// overwrite an existing StorePath, but to ensure correctness we should ensure that all StorePaths
 				// come from the same package version.
 				existingSysInfo, exists := existing.Systems[sysName]
-				if !exists || existingSysInfo.StorePath != newSysInfo.StorePath {
+				if !exists || !existingSysInfo.Equals(newSysInfo) {
 					updated = true
 				}
 			}
diff --git a/internal/devpkg/narinfo_cache.go b/internal/devpkg/narinfo_cache.go
index 9eb9da8c..584802bc 100644
--- a/internal/devpkg/narinfo_cache.go
+++ b/internal/devpkg/narinfo_cache.go
@@ -112,7 +112,7 @@ func (p *Package) fetchNarInfoStatus() (bool, error) {
 		)
 	}
 
-	pathParts := nix.NewStorePathParts(sysInfo.StorePath)
+	pathParts := nix.NewStorePathParts(sysInfo.DefaultStorePath())
 	reqURL := BinaryCache + "/" + pathParts.Hash + ".narinfo"
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
diff --git a/internal/devpkg/package.go b/internal/devpkg/package.go
index d240b602..b502480c 100644
--- a/internal/devpkg/package.go
+++ b/internal/devpkg/package.go
@@ -207,7 +207,7 @@ func resolve(pkg *Package) error {
 		return err
 	}
 	if inCache, err := pkg.IsInBinaryCache(); err == nil && inCache {
-		pkg.storePath = resolved.Systems[nix.System()].StorePath
+		pkg.storePath = resolved.Systems[nix.System()].DefaultStorePath()
 	}
 	parsed, err := flake.ParseInstallable(resolved.Resolved)
 	if err != nil {
@@ -576,7 +576,7 @@ func (p *Package) InputAddressedPath() (string, error) {
 	}
 
 	sysInfo := entry.Systems[nix.System()]
-	return sysInfo.StorePath, nil
+	return sysInfo.DefaultStorePath(), nil
 }
 
 func (p *Package) HasAllowInsecure() bool {
@@ -600,6 +600,7 @@ func (p *Package) StoreName() (string, error) {
 }
 
 func (p *Package) EnsureUninstallableIsInLockfile() error {
+	// TODO savil: Should !p.isInstallable() be the opposite i.e. p.IsInstallable()?
 	// TODO savil: Do we need the IsDevboxPackage check here?
 	if !p.IsInstallable() || !p.IsDevboxPackage {
 		return nil
diff --git a/internal/lock/lockfile.go b/internal/lock/lockfile.go
index 3227e428..8b256be6 100644
--- a/internal/lock/lockfile.go
+++ b/internal/lock/lockfile.go
@@ -46,6 +46,10 @@ func GetFile(project devboxProject) (*File, error) {
 	if err != nil {
 		return nil, err
 	}
+
+	// If the lockfile has legacy StorePath fields, we need to convert them to the new format
+	ensurePackagesHaveOutputs(lockFile.Packages)
+
 	return lockFile, nil
 }
 
@@ -92,12 +96,27 @@ func (f *File) Resolve(pkg string) (*Package, error) {
 	return f.Packages[pkg], nil
 }
 
-func (f *File) ForceResolve(pkg string) (*Package, error) {
-	delete(f.Packages, pkg)
-	return f.Resolve(pkg)
-}
-
 func (f *File) Save() error {
+	isDirty, err := f.isDirty()
+	if err != nil {
+		return err
+	}
+	// In SystemInfo, preserve legacy StorePath field and clear out modern Outputs before writing
+	// Reason: We want to update `devbox.lock` file only upon a user action
+	// such as `devbox update` or `devbox add` or `devbox remove`.
+	for pkgName, pkg := range f.Packages {
+		for sys, sysInfo := range pkg.Systems {
+			if !isDirty && sysInfo.StorePath != "" {
+				f.Packages[pkgName].Systems[sys].Outputs = nil
+			} else {
+				f.Packages[pkgName].Systems[sys].StorePath = ""
+			}
+		}
+	}
+	// We set back the Outputs, if needed, after writing the file, so that future
+	// users of the `lock.File` struct will have the correct data.
+	defer ensurePackagesHaveOutputs(f.Packages)
+
 	return cuecfg.WriteFile(lockFilePath(f.devboxProject.ProjectDir()), f)
 }
 
diff --git a/internal/lock/package.go b/internal/lock/package.go
index 7c065684..0898b5fd 100644
--- a/internal/lock/package.go
+++ b/internal/lock/package.go
@@ -3,6 +3,11 @@
 
 package lock
 
+import (
+	"fmt"
+	"slices"
+)
+
 const (
 	nixpkgSource       string = "nixpkg"
 	devboxSearchSource string = "devbox-search"
@@ -22,13 +27,30 @@ type Package struct {
 }
 
 type SystemInfo struct {
-	// StorePath is the input-addressed path for the nix package in /nix/store
-	// It is the cache key in the Binary Cache Store (cache.nixos.org)
-	// It is of the form /nix/store/<hash>-<name>-<version>
-	// <name> may be different from the canonicalName so we store the full store path.
+	Outputs []Output `json:"outputs,omitempty"`
+
+	// Legacy Format
 	StorePath string `json:"store_path,omitempty"`
 }
 
+// Output refers to a nix package output. This struct is derived from searcher.Output
+type Output struct {
+	// Name is the output's name. Nix appends the name to
+	// the output's store path unless it's the default name
+	// of "out". Output names can be anything, but
+	// conventionally they follow the various "make install"
+	// directories such as "bin", "lib", "src", "man", etc.
+	Name string `json:"name,omitempty"`
+
+	// Path is the absolute store path (with the /nix/store/
+	// prefix) of the output.
+	Path string `json:"path,omitempty"`
+
+	// Default indicates if Nix installs this output by
+	// default.
+	Default bool `json:"default,omitempty"`
+}
+
 func (p *Package) GetSource() string {
 	if p == nil {
 		return ""
@@ -43,9 +65,49 @@ func (p *Package) IsAllowInsecure() bool {
 	return p.AllowInsecure
 }
 
+// Useful for debugging when we print the struct
+func (i *SystemInfo) String() string {
+	return fmt.Sprintf("%+v", *i)
+}
+
+func (i *SystemInfo) DefaultStorePath() string {
+	if i == nil || len(i.Outputs) == 0 {
+		return ""
+	}
+
+	for _, output := range i.Outputs {
+		if output.Default {
+			return output.Path
+		}
+	}
+
+	return i.Outputs[0].Path
+}
+
 func (i *SystemInfo) Equals(other *SystemInfo) bool {
 	if i == nil || other == nil {
 		return i == other
 	}
-	return *i == *other
+
+	return slices.Equal(i.Outputs, other.Outputs)
+}
+
+// ensurePackagesHaveOutputs is used for backwards-compatibility with the old
+// lockfile format where each SystemInfo had a StorePath but no Outputs.
+func ensurePackagesHaveOutputs(packages map[string]*Package) {
+	for _, pkg := range packages {
+		for sys, sysInfo := range pkg.Systems {
+			// If we have a StorePath and no Outputs, we need to convert to the new format.
+			// Note: for a non-empty StorePath, Outputs should be empty, but being cautious.
+			if sysInfo.StorePath != "" && len(sysInfo.Outputs) == 0 {
+				pkg.Systems[sys].Outputs = []Output{
+					{
+						Default: true,
+						Name:    "out",
+						Path:    sysInfo.StorePath,
+					},
+				}
+			}
+		}
+	}
 }
diff --git a/internal/lock/resolve.go b/internal/lock/resolve.go
index da8e0ed2..2399b432 100644
--- a/internal/lock/resolve.go
+++ b/internal/lock/resolve.go
@@ -102,8 +102,16 @@ func resolveV2(ctx context.Context, name, version string) (*Package, error) {
 	}
 	for sys, info := range resolved.Systems {
 		if len(info.Outputs) != 0 {
+			outputs := make([]Output, len(info.Outputs))
+			for i, out := range info.Outputs {
+				outputs[i] = Output{
+					Name:    out.Name,
+					Path:    out.Path,
+					Default: out.Default,
+				}
+			}
 			pkg.Systems[sys] = &SystemInfo{
-				StorePath: info.Outputs[0].Path,
+				Outputs: outputs,
 			}
 		}
 	}
@@ -166,6 +174,13 @@ func buildLockSystemInfos(pkg *searcher.PackageVersion) (map[string]*SystemInfo,
 	sysInfos := map[string]*SystemInfo{}
 	for sysName, storePath := range sysStorePaths {
 		sysInfos[sysName] = &SystemInfo{
+			Outputs: []Output{
+				{
+					Default: true,
+					Name:    "out",
+					Path:    storePath,
+				},
+			},
 			StorePath: storePath,
 		}
 	}
diff --git a/internal/nix/build.go b/internal/nix/build.go
index 9c3d3a2e..1b13cb0e 100644
--- a/internal/nix/build.go
+++ b/internal/nix/build.go
@@ -2,17 +2,16 @@
 
 import (
 	"context"
-	"fmt"
+	"io"
 	"os"
-	"os/exec"
 
-	"github.com/pkg/errors"
 	"go.jetpack.io/devbox/internal/debug"
 )
 
 type BuildArgs struct {
 	AllowInsecure bool
 	Flags         []string
+	Writer        io.Writer
 }
 
 func Build(ctx context.Context, args *BuildArgs, installables ...string) error {
@@ -26,14 +25,13 @@ func Build(ctx context.Context, args *BuildArgs, installables ...string) error {
 		cmd.Env = allowInsecureEnv(cmd.Env)
 	}
 
+	// If nix build runs as tty, the output is much nicer. If we ever
+	// need to change this to our own writers, consider that you may need
+	// to implement your own nicer output. --print-build-logs flag may be useful.
+	cmd.Stdin = os.Stdin
+	cmd.Stdout = args.Writer
+	cmd.Stderr = args.Writer
+
 	debug.Log("Running cmd: %s\n", cmd)
-	_, err := cmd.Output()
-	if err != nil {
-		if exitErr := (&exec.ExitError{}); errors.As(err, &exitErr) {
-			debug.Log("Nix build exit code: %d, output: %s\n", exitErr.ExitCode(), exitErr.Stderr)
-			return fmt.Errorf("nix build exit code: %d, output: %s, err: %w", exitErr.ExitCode(), exitErr.Stderr, err)
-		}
-		return err
-	}
-	return nil
+	return cmd.Run()
 }


Test patch:
diff --git a/internal/devbox/update_test.go b/internal/devbox/update_test.go
index bdb2ed95..4ca606ae 100644
--- a/internal/devbox/update_test.go
+++ b/internal/devbox/update_test.go
@@ -39,7 +39,13 @@ func TestUpdateNewCurrentSysInfoIsAdded(t *testing.T) {
 		Resolved: "resolved-flake-reference",
 		Systems: map[string]*lock.SystemInfo{
 			sys: {
-				StorePath: "store_path1",
+				Outputs: []lock.Output{
+					{
+						Name:    "out",
+						Default: true,
+						Path:    "store_path1",
+					},
+				},
 			},
 		},
 	}
@@ -57,7 +63,7 @@ func TestUpdateNewCurrentSysInfoIsAdded(t *testing.T) {
 
 	require.Contains(t, lockfile.Packages, raw)
 	require.Contains(t, lockfile.Packages[raw].Systems, sys)
-	require.Equal(t, "store_path1", lockfile.Packages[raw].Systems[sys].StorePath)
+	require.Equal(t, "store_path1", lockfile.Packages[raw].Systems[sys].DefaultStorePath())
 }
 
 func TestUpdateNewSysInfoIsAdded(t *testing.T) {
@@ -72,10 +78,22 @@ func TestUpdateNewSysInfoIsAdded(t *testing.T) {
 		Resolved: "resolved-flake-reference",
 		Systems: map[string]*lock.SystemInfo{
 			sys1: {
-				StorePath: "store_path1",
+				Outputs: []lock.Output{
+					{
+						Name:    "out",
+						Default: true,
+						Path:    "store_path1",
+					},
+				},
 			},
 			sys2: {
-				StorePath: "store_path2",
+				Outputs: []lock.Output{
+					{
+						Name:    "out",
+						Default: true,
+						Path:    "store_path2",
+					},
+				},
 			},
 		},
 	}
@@ -85,7 +103,13 @@ func TestUpdateNewSysInfoIsAdded(t *testing.T) {
 				Resolved: "resolved-flake-reference",
 				Systems: map[string]*lock.SystemInfo{
 					sys1: {
-						StorePath: "store_path1",
+						Outputs: []lock.Output{
+							{
+								Name:    "out",
+								Default: true,
+								Path:    "store_path1",
+							},
+						},
 					},
 					// Missing sys2
 				},
@@ -99,7 +123,7 @@ func TestUpdateNewSysInfoIsAdded(t *testing.T) {
 	require.Contains(t, lockfile.Packages, raw)
 	require.Contains(t, lockfile.Packages[raw].Systems, sys1)
 	require.Contains(t, lockfile.Packages[raw].Systems, sys2)
-	require.Equal(t, "store_path2", lockfile.Packages[raw].Systems[sys2].StorePath)
+	require.Equal(t, "store_path2", lockfile.Packages[raw].Systems[sys2].DefaultStorePath())
 }
 
 func TestUpdateOtherSysInfoIsReplaced(t *testing.T) {
@@ -114,10 +138,22 @@ func TestUpdateOtherSysInfoIsReplaced(t *testing.T) {
 		Resolved: "resolved-flake-reference",
 		Systems: map[string]*lock.SystemInfo{
 			sys1: {
-				StorePath: "store_path1",
+				Outputs: []lock.Output{
+					{
+						Name:    "out",
+						Default: true,
+						Path:    "store_path1",
+					},
+				},
 			},
 			sys2: {
-				StorePath: "store_path2",
+				Outputs: []lock.Output{
+					{
+						Name:    "out",
+						Default: true,
+						Path:    "store_path2",
+					},
+				},
 			},
 		},
 	}
@@ -127,10 +163,22 @@ func TestUpdateOtherSysInfoIsReplaced(t *testing.T) {
 				Resolved: "resolved-flake-reference",
 				Systems: map[string]*lock.SystemInfo{
 					sys1: {
-						StorePath: "store_path1",
+						Outputs: []lock.Output{
+							{
+								Name:    "out",
+								Default: true,
+								Path:    "store_path1",
+							},
+						},
 					},
 					sys2: {
-						StorePath: "mismatching_store_path",
+						Outputs: []lock.Output{
+							{
+								Name:    "out",
+								Default: true,
+								Path:    "mismatching_store_path",
+							},
+						},
 					},
 				},
 			},
@@ -143,8 +191,8 @@ func TestUpdateOtherSysInfoIsReplaced(t *testing.T) {
 	require.Contains(t, lockfile.Packages, raw)
 	require.Contains(t, lockfile.Packages[raw].Systems, sys1)
 	require.Contains(t, lockfile.Packages[raw].Systems, sys2)
-	require.Equal(t, "store_path1", lockfile.Packages[raw].Systems[sys1].StorePath)
-	require.Equal(t, "store_path2", lockfile.Packages[raw].Systems[sys2].StorePath)
+	require.Equal(t, "store_path1", lockfile.Packages[raw].Systems[sys1].DefaultStorePath())
+	require.Equal(t, "store_path2", lockfile.Packages[raw].Systems[sys2].DefaultStorePath())
 }
 
 func currentSystem(_t *testing.T) string {