code review feedback fixes

Author: savil

internal/devbox/nixprofile.go

@@ -5,8 +5,10 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/samber/lo"
 	"go.jetpack.io/devbox/internal/nix"
 	"go.jetpack.io/devbox/internal/nix/nixprofile"
+	"go.jetpack.io/devbox/internal/ux"
 )
 
 // syncNixProfile ensures the nix profile has the packages specified in wantStorePaths.
@@ -28,17 +30,17 @@ func (d *Devbox) syncNixProfile(ctx context.Context, wantStorePaths []string) er
 	}
 
 	// Diff the store paths and install/remove packages as needed
-	add, remove := diffStorePaths(gotStorePaths, wantStorePaths)
+	remove, add := lo.Difference(gotStorePaths, wantStorePaths)
 	if len(remove) > 0 {
 		packagesToRemove := make([]string, 0, len(remove))
 		for _, p := range remove {
 			storePath := nix.NewStorePathParts(p)
 			packagesToRemove = append(packagesToRemove, fmt.Sprintf("%s@%s", storePath.Name, storePath.Version))
 		}
 		if len(packagesToRemove) == 1 {
-			fmt.Fprintf(d.stderr, "Removing %s\n", strings.Join(packagesToRemove, ", "))
+			ux.Finfo(d.stderr, "Removing %s\n", strings.Join(packagesToRemove, ", "))
 		} else {
-			fmt.Fprintf(d.stderr, "Removing packages: %s\n", strings.Join(packagesToRemove, ", "))
+			ux.Finfo(d.stderr, "Removing packages: %s\n", strings.Join(packagesToRemove, ", "))
 		}
 
 		if err := nix.ProfileRemove(profilePath, remove...); err != nil {
@@ -70,27 +72,3 @@ func (d *Devbox) syncNixProfile(ctx context.Context, wantStorePaths []string) er
 	}
 	return nil
 }
-
-func diffStorePaths(got, want []string) (add, remove []string) {
-	gotSet := map[string]bool{}
-	for _, g := range got {
-		gotSet[g] = true
-	}
-	wantSet := map[string]bool{}
-	for _, w := range want {
-		wantSet[w] = true
-	}
-
-	for _, g := range got {
-		if _, ok := wantSet[g]; !ok {
-			remove = append(remove, g)
-		}
-	}
-
-	for _, w := range want {
-		if _, ok := gotSet[w]; !ok {
-			add = append(add, w)
-		}
-	}
-	return add, remove
-}

internal/devbox/packages.go

@@ -249,7 +249,7 @@ const (
 // 1. Skipping certain operations that may not apply.
 // 2. User messaging to explain what operations are happening, because this function may take time to execute.
 //
-// nolint:revive to turn off linter complaining about function complexity
+// linter:revive is turned off due to complaining about function complexity
 func (d *Devbox) ensureStateIsUpToDate(ctx context.Context, mode installMode) error { //nolint:revive
 	defer trace.StartRegion(ctx, "devboxEnsureStateIsUpToDate").End()
 	defer debug.FunctionTimer().End()
@@ -259,10 +259,7 @@ func (d *Devbox) ensureStateIsUpToDate(ctx context.Context, mode installMode) er
 		// then we skip some operations below for speed.
 		// Remove the local.lock file so that we re-compute the project state when
 		// we are in the devbox environment again.
-		fmt.Printf("mode is not ensure and env is not enabled, so will remove local.lock on exit\n")
-
 		defer func() { _ = d.lockfile.RemoveLocal() }()
-		// return nil
 	}
 
 	// if mode is install or uninstall, then we need to update the nix-profile
@@ -275,12 +272,10 @@ func (d *Devbox) ensureStateIsUpToDate(ctx context.Context, mode installMode) er
 	if mode == ensure {
 		// if mode is ensure and we are up to date, then we can skip the rest
 		if upToDate {
-			fmt.Printf("state is up to date. Returning early\n")
 			return nil
 		}
 		fmt.Fprintln(d.stderr, "Ensuring packages are installed.")
 	}
-	fmt.Printf("state is not up to date. Continuing...\n")
 
 	// Validate packages. Must be run up-front and definitely prior to computeEnv
 	// and syncNixProfile below that will evaluate the flake and may give
@@ -300,9 +295,6 @@ func (d *Devbox) ensureStateIsUpToDate(ctx context.Context, mode installMode) er
 		return err
 	}
 
-	fmt.Printf("mode is %s\n", mode)
-	fmt.Printf("env is enabled: %v\n", d.IsEnvEnabled())
-
 	if err := shellgen.GenerateForPrintEnv(ctx, d); err != nil {
 		return err
 	}
@@ -341,7 +333,6 @@ func (d *Devbox) ensureStateIsUpToDate(ctx context.Context, mode installMode) er
 		}
 	}
 
-	fmt.Printf("calling lockfile.tidy\n")
 	// Ensure we clean out packages that are no longer needed.
 	d.lockfile.Tidy()
 
@@ -428,72 +419,46 @@ func (d *Devbox) InstallRunXPackages(ctx context.Context) error {
 
 // installNixPackagesToStore will install all the packages in the nix store, if
 // mode is install or update, and we're not in a devbox environment.
-// This is done by running `nix build` on the flake
+// This is done by running `nix build` on the flake. We do this so that the
+// packages will be available in the nix store when computing the devbox environment
+// and installing in the nix profile (even if offline).
 func (d *Devbox) installNixPackagesToStore(ctx context.Context) error {
-	packages, err := d.AllInstallablePackages()
+	packages, err := d.packagesToInstallInProfile(ctx)
 	if err != nil {
 		return err
 	}
-	packages = lo.Filter(packages, devpkg.IsNix) // Remove non-nix packages from the list
 
+	names := []string{}
 	installables := []string{}
 	for _, pkg := range packages {
 		i, err := pkg.Installable()
 		if err != nil {
 			return err
 		}
 		installables = append(installables, i)
+		names = append(names, pkg.String())
 	}
 
 	if len(installables) == 0 {
 		return nil
 	}
 
+	ux.Finfo(d.stderr, "Installing to the nix store: %s. This may take a brief while.\n", strings.Join(names, " "))
+
 	// --no-link to avoid generating the result objects
 	return nix.Build(ctx, []string{"--no-link"}, installables...)
 }
 
 // validatePackagesToBeInstalled will ensure that packages are available to be installed
 // in the user's current system.
 func (d *Devbox) validatePackagesToBeInstalled(ctx context.Context) error {
-	// First, fetch the profile items from the nix-profile,
-	profileDir, err := d.profilePath()
-	if err != nil {
-		return err
-	}
-	profileItems, err := nixprofile.ProfileListItems(ctx, d.stderr, profileDir)
+	// First, get the packages to install
+	packagesToInstall, err := d.packagesToInstallInProfile(ctx)
 	if err != nil {
 		return err
 	}
 
-	// Second, get and prepare all the packages that must be installed in this project
-	packages, err := d.AllInstallablePackages()
-	if err != nil {
-		return err
-	}
-	packages = lo.Filter(packages, devpkg.IsNix) // Remove non-nix packages from the list
-	if err := devpkg.FillNarInfoCache(ctx, packages...); err != nil {
-		return err
-	}
-
-	// Third, compute which packages need to be installed
-	packagesToInstall := []*devpkg.Package{}
-	// Note: because devpkg.Package uses memoization when normalizing attribute paths (slow operation),
-	// and since we're reusing the Package objects, this O(n*m) loop becomes O(n+m) wrt the slow operation.
-	for _, pkg := range packages {
-		found := false
-		for _, item := range profileItems {
-			if item.Matches(pkg, d.lockfile) {
-				found = true
-				break
-			}
-		}
-		if !found {
-			packagesToInstall = append(packagesToInstall, pkg)
-		}
-	}
-
-	// Last, validate that packages that need to be installed are in fact installable
+	// Then, validate that packages that need to be installed are in fact installable
 	// on the user's current system.
 	for _, pkg := range packagesToInstall {
 		inCache, err := pkg.IsInBinaryCache()
@@ -524,3 +489,43 @@ func (d *Devbox) validatePackagesToBeInstalled(ctx context.Context) error {
 	}
 	return nil
 }
+
+func (d *Devbox) packagesToInstallInProfile(ctx context.Context) ([]*devpkg.Package, error) {
+	// First, fetch the profile items from the nix-profile,
+	profileDir, err := d.profilePath()
+	if err != nil {
+		return nil, err
+	}
+	profileItems, err := nixprofile.ProfileListItems(ctx, d.stderr, profileDir)
+	if err != nil {
+		return nil, err
+	}
+
+	// Second, get and prepare all the packages that must be installed in this project
+	packages, err := d.AllInstallablePackages()
+	if err != nil {
+		return nil, err
+	}
+	packages = lo.Filter(packages, devpkg.IsNix) // Remove non-nix packages from the list
+	if err := devpkg.FillNarInfoCache(ctx, packages...); err != nil {
+		return nil, err
+	}
+
+	// Third, compute which packages need to be installed
+	packagesToInstall := []*devpkg.Package{}
+	// Note: because devpkg.Package uses memoization when normalizing attribute paths (slow operation),
+	// and since we're reusing the Package objects, this O(n*m) loop becomes O(n+m) wrt the slow operation.
+	for _, pkg := range packages {
+		found := false
+		for _, item := range profileItems {
+			if item.Matches(pkg, d.lockfile) {
+				found = true
+				break
+			}
+		}
+		if !found {
+			packagesToInstall = append(packagesToInstall, pkg)
+		}
+	}
+	return packagesToInstall, nil
+}

internal/lock/local.go

@@ -5,7 +5,6 @@ package lock
 
 import (
 	"errors"
-	"fmt"
 	"io/fs"
 	"os"
 	"path/filepath"
@@ -51,8 +50,6 @@ func isLocalUpToDate(project devboxProject) (bool, error) {
 }
 
 func updateLocal(project devboxProject) error {
-	fmt.Printf("updating local.lock\n")
-	// debug.PrintStack()
 	l, err := readLocal(project)
 	if err != nil {
 		return err

internal/lock/lockfile.go

@@ -55,7 +55,6 @@ func (f *File) Add(pkgs ...string) error {
 			return err
 		}
 	}
-	fmt.Printf("calling lockfile.Add with pkgs: %v\n", pkgs)
 	return f.Save()
 }
 

internal/nix/build.go

@@ -2,10 +2,10 @@ package nix
 
 import (
 	"context"
-	"fmt"
 	"os/exec"
 
 	"github.com/pkg/errors"
+	"go.jetpack.io/devbox/internal/debug"
 )
 
 func Build(ctx context.Context, flags []string, installables ...string) error {
@@ -15,15 +15,15 @@ func Build(ctx context.Context, flags []string, installables ...string) error {
 	cmd.Args = append(cmd.Args, installables...)
 	// We need to allow Unfree packages to be installed. We choose to not also add os.Environ() to the environment
 	// to keep the command as pure as possible, even though we must pass --impure to nix build.
-	cmd.Env = allowUnfreeEnv([]string{}) // allowUnfreeEnv(os.Environ())
+	cmd.Env = allowUnfreeEnv([]string{})
 
-	out, err := cmd.Output()
+	debug.Log("Running cmd: %s\n", cmd)
+	_, err := cmd.Output()
 	if err != nil {
 		if exitErr := (&exec.ExitError{}); errors.As(err, &exitErr) {
-			fmt.Printf("Exit code: %d, output: %s\n", exitErr.ExitCode(), exitErr.Stderr)
+			debug.Log("Nix build exit code: %d, output: %s\n", exitErr.ExitCode(), exitErr.Stderr)
 		}
 		return err
 	}
-	fmt.Printf("Ran nix build, output: %s\n", out)
 	return nil
 }