[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 := "hello@1.2.3"
@@ -67,7 +65,6 @@ func TestUpdateNewCurrentSysInfoIsAdded(t *testing.T) {
 }
 
 func TestUpdateNewSysInfoIsAdded(t *testing.T) {
-featureflag.RemoveNixpkgs.EnableForTest(t)
 devbox := devboxForTesting(t)
 
 raw := "hello@1.2.3"
@@ -127,7 +124,6 @@ func TestUpdateNewSysInfoIsAdded(t *testing.T) {
 }
 
 func TestUpdateOtherSysInfoIsReplaced(t *testing.T) {
-featureflag.RemoveNixpkgs.EnableForTest(t)
 devbox := devboxForTesting(t)
 
 raw := "hello@1.2.3"

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 = [
  ];
  };
- }
- );
-}
+ };
+ }