[Remove Nixpkgs] remove feature flag (#2142)

## Summary

We've had this feature launched for quite a while. Initially, I was
reluctant to remove the feature-flag so that we could debug code paths
for users on old versions of nix (which cannot benefit from the binary
cache. IIRC nix < 1.17). However, in the time since I relied on this
exactly once for some perf-related task, and not otherwise.

At this point, I think we should remove it, and even print messages to
users about updating their nix version if needed.

## How was it tested?

It compiles. I could do `devbox shell` in this project.

Author: savil

internal/boxcli/featureflag/remove_nixpkgs.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 
 	"github.com/pkg/errors"
-	"go.jetpack.io/devbox/internal/boxcli/featureflag"
 	"go.jetpack.io/devbox/internal/devbox/devopt"
 	"go.jetpack.io/devbox/internal/devpkg"
 	"go.jetpack.io/devbox/internal/lock"
@@ -143,39 +142,36 @@ func (d *Devbox) mergeResolvedPackageToLockfile(
 	}
 
 	// Add any missing system infos for packages whose versions did not change.
-	if featureflag.RemoveNixpkgs.Enabled() {
-
-		if lockfile.Packages[pkg.Raw].Systems == nil {
-			lockfile.Packages[pkg.Raw].Systems = map[string]*lock.SystemInfo{}
-		}
+	if lockfile.Packages[pkg.Raw].Systems == nil {
+		lockfile.Packages[pkg.Raw].Systems = map[string]*lock.SystemInfo{}
+	}
 
-		userSystem := nix.System()
-		updated := false
-		for sysName, newSysInfo := range resolved.Systems {
-			// Check whether we are actually updating any system info.
-			if sysName == userSystem {
-				// The resolved pkg has a system info for the user's system, so add/overwrite it.
-				if !newSysInfo.Equals(existing.Systems[userSystem]) {
-					// We only guard this so that the ux messaging is accurate. We could overwrite every time.
-					updated = true
-				}
-			} else {
-				// Add other system infos if they don't exist, or if we have a different StorePath. This may
-				// 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.Equals(newSysInfo) {
-					updated = true
-				}
+	userSystem := nix.System()
+	updated := false
+	for sysName, newSysInfo := range resolved.Systems {
+		// Check whether we are actually updating any system info.
+		if sysName == userSystem {
+			// The resolved pkg has a system info for the user's system, so add/overwrite it.
+			if !newSysInfo.Equals(existing.Systems[userSystem]) {
+				// We only guard this so that the ux messaging is accurate. We could overwrite every time.
+				updated = true
+			}
+		} else {
+			// Add other system infos if they don't exist, or if we have a different StorePath. This may
+			// 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.Equals(newSysInfo) {
+				updated = true
 			}
 		}
-		if updated {
-			// if we are updating the system info, then we should also update the other fields
-			useResolvedPackageInLockfile(lockfile, pkg, resolved, existing)
+	}
+	if updated {
+		// if we are updating the system info, then we should also update the other fields
+		useResolvedPackageInLockfile(lockfile, pkg, resolved, existing)
 
-			ux.Finfo(d.stderr, "Updated system information for %s\n", pkg)
-			return nil
-		}
+		ux.Finfo(d.stderr, "Updated system information for %s\n", pkg)
+		return nil
 	}
 
 	ux.Finfo(d.stderr, "Already up-to-date %s %s\n", pkg, existing.Version)

internal/devbox/update.go

@@ -4,7 +4,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"go.jetpack.io/devbox/internal/boxcli/featureflag"
 	"go.jetpack.io/devbox/internal/devpkg"
 	"go.jetpack.io/devbox/internal/lock"
 	"go.jetpack.io/devbox/internal/nix"
@@ -29,7 +28,6 @@ func TestUpdateNewPackageIsAdded(t *testing.T) {
 }
 
 func TestUpdateNewCurrentSysInfoIsAdded(t *testing.T) {
-	featureflag.RemoveNixpkgs.EnableForTest(t)
 	devbox := devboxForTesting(t)
 
 	raw := "[email protected]"
@@ -67,7 +65,6 @@ func TestUpdateNewCurrentSysInfoIsAdded(t *testing.T) {
 }
 
 func TestUpdateNewSysInfoIsAdded(t *testing.T) {
-	featureflag.RemoveNixpkgs.EnableForTest(t)
 	devbox := devboxForTesting(t)
 
 	raw := "[email protected]"
@@ -127,7 +124,6 @@ func TestUpdateNewSysInfoIsAdded(t *testing.T) {
 }
 
 func TestUpdateOtherSysInfoIsReplaced(t *testing.T) {
-	featureflag.RemoveNixpkgs.EnableForTest(t)
 	devbox := devboxForTesting(t)
 
 	raw := "[email protected]"

internal/devbox/update_test.go

@@ -13,7 +13,6 @@ import (
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/s3"
 	"github.com/pkg/errors"
-	"go.jetpack.io/devbox/internal/boxcli/featureflag"
 	"go.jetpack.io/devbox/internal/debug"
 	"go.jetpack.io/devbox/internal/devbox/providers/nixcache"
 	"go.jetpack.io/devbox/internal/goutil"
@@ -59,9 +58,6 @@ func (p *Package) IsInBinaryCache() (bool, error) {
 // Callers of IsInBinaryCache may call this function first as a perf-optimization.
 func FillNarInfoCache(ctx context.Context, packages ...*Package) error {
 	defer debug.FunctionTimer().End()
-	if !featureflag.RemoveNixpkgs.Enabled() {
-		return nil
-	}
 
 	eligiblePackages := []*Package{}
 	for _, p := range packages {
@@ -240,10 +236,6 @@ func (p *Package) isEligibleForBinaryCache() (bool, error) {
 // Hence, we compute nix.Version, nix.System and lockfile.Resolve prior to calling this
 // function from within a goroutine.
 func (p *Package) sysInfoIfExists() (*lock.SystemInfo, error) {
-	if !featureflag.RemoveNixpkgs.Enabled() {
-		return nil, nil
-	}
-
 	if !p.isVersioned() {
 		return nil, nil
 	}

internal/devpkg/narinfo_cache.go

@@ -56,12 +56,9 @@ func (f *File) FetchResolvedPackage(pkg string) (*Package, error) {
 		return nil, errors.Wrapf(nix.ErrPackageNotFound, "%s@%s", name, version)
 	}
 
-	sysInfos := map[string]*SystemInfo{}
-	if featureflag.RemoveNixpkgs.Enabled() {
-		sysInfos, err = buildLockSystemInfos(packageVersion)
-		if err != nil {
-			return nil, err
-		}
+	sysInfos, err := buildLockSystemInfos(packageVersion)
+	if err != nil {
+		return nil, err
 	}
 	packageInfo, err := selectForSystem(packageVersion.Systems)
 	if err != nil {

internal/lock/resolve.go

@@ -20,7 +20,6 @@ import (
 	"time"
 
 	"github.com/pkg/errors"
-	"go.jetpack.io/devbox/internal/boxcli/featureflag"
 	"go.jetpack.io/devbox/internal/boxcli/usererr"
 	"go.jetpack.io/devbox/internal/redact"
 	"golang.org/x/mod/semver"
@@ -117,10 +116,7 @@ func FlakeNixpkgs(commit string) string {
 }
 
 func ExperimentalFlags() []string {
-	options := []string{"nix-command", "flakes"}
-	if featureflag.RemoveNixpkgs.Enabled() {
-		options = append(options, "fetch-closure")
-	}
+	options := []string{"nix-command", "flakes", "fetch-closure"}
 	return []string{
 		"--extra-experimental-features", "ca-derivations",
 		"--option", "experimental-features", strings.Join(options, " "),

internal/nix/nix.go

@@ -9,7 +9,6 @@ import (
 	"strings"
 
 	"github.com/samber/lo"
-	"go.jetpack.io/devbox/internal/boxcli/featureflag"
 	"go.jetpack.io/devbox/internal/devpkg"
 	"go.jetpack.io/devbox/internal/nix"
 )
@@ -168,15 +167,13 @@ func flakeInputs(ctx context.Context, packages []*devpkg.Package) []flakeInput {
 
 		// Don't include cached packages (like local or remote flakes)
 		// that can be fetched from a Binary Cache Store.
-		if featureflag.RemoveNixpkgs.Enabled() {
-			// TODO(savil): return error?
-			cached, err := pkg.IsInBinaryCache()
-			if err != nil {
-				slog.Error("error checking if package is in binary cache", "err", err)
-			}
-			if err == nil && cached {
-				continue
-			}
+		// TODO(savil): return error?
+		cached, err := pkg.IsInBinaryCache()
+		if err != nil {
+			slog.Error("error checking if package is in binary cache", "err", err)
+		}
+		if err == nil && cached {
+			continue
 		}
 
 		// Packages that need a glibc patch are assigned to the special

internal/shellgen/flake_input.go

@@ -16,7 +16,6 @@ import (
 	"text/template"
 
 	"github.com/pkg/errors"
-	"go.jetpack.io/devbox/internal/boxcli/featureflag"
 	"go.jetpack.io/devbox/internal/cuecfg"
 	"go.jetpack.io/devbox/internal/debug"
 	"go.jetpack.io/devbox/internal/redact"
@@ -184,9 +183,5 @@ var templateFuncs = template.FuncMap{
 
 func makeFlakeFile(d devboxer, plan *flakePlan) error {
 	flakeDir := FlakePath(d)
-	templateName := "flake.nix"
-	if featureflag.RemoveNixpkgs.Enabled() {
-		templateName = "flake_remove_nixpkgs.nix"
-	}
-	return writeFromTemplate(flakeDir, plan, templateName, "flake.nix")
+	return writeFromTemplate(flakeDir, plan, "flake.nix", "flake.nix")
 }

internal/shellgen/generate.go

@@ -19,6 +19,9 @@ import (
 // update overwrites golden files with the new test results.
 var update = flag.Bool("update", false, "update the golden files with the test results")
 
+// TestWriteFromTemplate will verify that the flake.nix code generation works as expected.
+// Note: this test was derived from an older flake.nix, prior to having the builtins.FetchClosures
+// and so may be a bit out of date. It could be updated to be better and more exhaustive.
 func TestWriteFromTemplate(t *testing.T) {
 	t.Setenv("__DEVBOX_NIX_SYSTEM", "x86_64-linux")
 	dir := filepath.Join(t.TempDir(), "makeme")
@@ -37,12 +40,15 @@ func TestWriteFromTemplate(t *testing.T) {
 		cmpGoldenFile(t, outPath, "testdata/flake.nix.golden")
 	})
 	t.Run("WriteModifiedSmaller", func(t *testing.T) {
-		emptyPlan := struct {
-			NixpkgsInfo struct {
-				URL string
-			}
-			FlakeInputs []flakeInput
-		}{}
+		emptyPlan := &flakePlan{
+			NixpkgsInfo: &NixpkgsInfo{
+				URL:    "",
+				TarURL: "",
+			},
+			Packages:    []*devpkg.Package{},
+			FlakeInputs: []flakeInput{},
+			System:      "x86_64-linux",
+		}
 		err = writeFromTemplate(dir, emptyPlan, "flake.nix", "flake.nix")
 		if err != nil {
 			t.Fatal("got error writing flake template:", err)
@@ -82,17 +88,12 @@ If the new file is correct, you can update the golden file with:
 
 var (
 	locker            = &lockmock{}
-	testFlakeTmplPlan = &struct {
-		NixpkgsInfo struct {
-			URL string
-		}
-		FlakeInputs []flakeInput
-	}{
-		NixpkgsInfo: struct {
-			URL string
-		}{
-			URL: "https://github.com/nixos/nixpkgs/archive/b9c00c1d41ccd6385da243415299b39aa73357be.tar.gz",
+	testFlakeTmplPlan = &flakePlan{
+		NixpkgsInfo: &NixpkgsInfo{
+			URL:    "https://github.com/nixos/nixpkgs/archive/b9c00c1d41ccd6385da243415299b39aa73357be.tar.gz",
+			TarURL: "", // TODO savil
 		},
+		Packages: []*devpkg.Package{}, // TODO savil
 		FlakeInputs: []flakeInput{
 			{
 				Name: "nixpkgs",
@@ -123,6 +124,7 @@ var (
 				},
 			},
 		},
+		System: "x86_64-linux",
 	}
 )
 

internal/shellgen/generate_test.go

@@ -1,28 +1,21 @@
 {
-  description = "A devbox shell";
+   description = "A devbox shell";
 
-  inputs = {
-    nixpkgs.url = "";
-    flake-utils.url = "github:numtide/flake-utils";
-  };
+   inputs = {
+     nixpkgs.url = "";
+   };
 
-  outputs = {
-    self,
-    nixpkgs,
-    flake-utils
-  }:
-    flake-utils.lib.eachDefaultSystem (system:
+   outputs = {
+     self,
+     nixpkgs,
+   }:
       let
-        pkgs = (import nixpkgs {
-          inherit system;
-          config.allowUnfree = true;
-        });
+        pkgs = nixpkgs.legacyPackages.x86_64-linux;
       in
       {
-        devShell = pkgs.mkShell {
-          buildInputs = with pkgs; [
+        devShells.x86_64-linux.default = pkgs.mkShell {
+          buildInputs = [
           ];
         };
-      }
-    );
-}
+      };
+ }