diff --git a/internal/devbox/packages.go b/internal/devbox/packages.go index 917835a0099..5184682cb1e 100644 --- a/internal/devbox/packages.go +++ b/internal/devbox/packages.go @@ -60,7 +60,7 @@ func (d *Devbox) Outdated(ctx context.Context) (map[string]UpdateVersion, error) continue } - lockPackage, err := lockfile.FetchResolvedPackage(pkg.Versioned()) + lockPackage, err := lockfile.FetchResolvedPackage(pkg.Versioned(), false) if err != nil { warnings = append(warnings, fmt.Sprintf("Note: unable to check updates for %s", pkg.CanonicalName())) continue diff --git a/internal/devbox/update.go b/internal/devbox/update.go index ad51657b21b..acd6592a529 100644 --- a/internal/devbox/update.go +++ b/internal/devbox/update.go @@ -7,17 +7,20 @@ import ( "context" "fmt" "slices" + "strings" + "time" "github.com/pkg/errors" "go.jetify.com/devbox/internal/devbox/devopt" "go.jetify.com/devbox/internal/devpkg" + "go.jetify.com/devbox/internal/devpkg/pkgtype" "go.jetify.com/devbox/internal/lock" "go.jetify.com/devbox/internal/nix" - "go.jetify.com/devbox/internal/nix/nixprofile" "go.jetify.com/devbox/internal/plugin" "go.jetify.com/devbox/internal/searcher" "go.jetify.com/devbox/internal/shellgen" "go.jetify.com/devbox/internal/ux" + "go.jetify.com/devbox/nix/flake" ) func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { @@ -68,16 +71,8 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { } } - for _, pkg := range pendingPackagesToUpdate { - if _, _, isVersioned := searcher.ParseVersionedPackage(pkg.Raw); !isVersioned { - if err = d.attemptToUpgradeFlake(pkg); err != nil { - return err - } - } else { - if err = d.updateDevboxPackage(pkg); err != nil { - return err - } - } + if err := d.updatePendingPackages(pendingPackagesToUpdate); err != nil { + return err } d.packagesBeingUpdated = inputs @@ -124,8 +119,32 @@ func (d *Devbox) inputsToUpdate( return pkgsToUpdate, nil } +// updatePendingPackages updates the lockfile entries for each package, using +// the right strategy per package kind. Flake refs warn-and-continue on +// failure (see #1180 / #1840); versioned nixpkgs packages abort the update on +// failure. Unversioned non-flake entries are left alone. +func (d *Devbox) updatePendingPackages(pkgs []*devpkg.Package) error { + for _, pkg := range pkgs { + if pkgtype.IsFlake(pkg.Raw) { + if err := d.updateDevboxPackage(pkg); err != nil { + ux.Fwarningf(d.stderr, "Failed to update %s: %s\n", pkg.Raw, err) + } + continue + } + if _, _, isVersioned := searcher.ParseVersionedPackage(pkg.Raw); isVersioned { + if err := d.updateDevboxPackage(pkg); err != nil { + return err + } + } + } + return nil +} + func (d *Devbox) updateDevboxPackage(pkg *devpkg.Package) error { - resolved, err := d.lockfile.FetchResolvedPackage(pkg.Raw) + // refresh=true so flake refs bypass nix's own metadata cache and re-query + // upstream. Without this, `devbox update` on a github: ref can return a + // stale commit that nix had cached from an earlier call. + resolved, err := d.lockfile.FetchResolvedPackage(pkg.Raw, true) if err != nil { return err } @@ -148,6 +167,14 @@ func (d *Devbox) mergeResolvedPackageToLockfile( return nil } + // Flake refs have no Version, so the Version-based comparison below would + // always report "Already up-to-date" even when the locked rev changed. + // Handle them via their Resolved field (which embeds the locked rev) and + // LastModified. + if pkgtype.IsFlake(pkg.Raw) { + return d.mergeResolvedFlakeToLockfile(pkg, resolved, existing, lockfile) + } + if existing.Version != resolved.Version { if existing.LastModified > resolved.LastModified { ux.Fwarningf( @@ -199,33 +226,76 @@ func (d *Devbox) mergeResolvedPackageToLockfile( return nil } -// attemptToUpgradeFlake attempts to upgrade a flake using `nix profile upgrade` -// and prints an error if it fails, but does not propagate upgrade errors. -func (d *Devbox) attemptToUpgradeFlake(pkg *devpkg.Package) error { - profilePath, err := d.profilePath() - if err != nil { - return err +// mergeResolvedFlakeToLockfile updates the lockfile entry for a flake ref. It +// compares on Resolved (which embeds the locked rev) rather than Version since +// flake refs don't carry a semver. It honors the same LastModified staleness +// guard as the nixpkgs path. +func (d *Devbox) mergeResolvedFlakeToLockfile( + pkg *devpkg.Package, + resolved *lock.Package, + existing *lock.Package, + lockfile *lock.File, +) error { + if existing.Resolved == resolved.Resolved { + ux.Finfof(d.stderr, "Already up-to-date %s\n", pkg) + return nil } - ux.Finfof( - d.stderr, - "Attempting to upgrade %s using `nix profile upgrade`\n", - pkg.Raw, - ) - - err = nixprofile.ProfileUpgrade(profilePath, pkg, d.lockfile) - if err != nil { + // Skip the guard if either side is missing a timestamp — treat unknown as + // not-older so we don't block a legit update. + if existing.LastModified != "" && resolved.LastModified != "" && + existing.LastModified > resolved.LastModified { ux.Fwarningf( d.stderr, - "Failed to upgrade %s using `nix profile upgrade`: %s\n", - pkg.Raw, - err, + "Resolved ref for %s has older last_modified time. Not updating\n", + pkg, ) + return nil } + ux.Finfof(d.stderr, "Updating %s %s\n", pkg, describeFlakeUpdate(existing, resolved)) + useResolvedPackageInLockfile(lockfile, pkg, resolved, existing) return nil } +// describeFlakeUpdate renders a short human-readable diff between two flake +// lockfile entries. It prefers short revs when both sides have them, falls +// back to a date range when not, and omits either piece cleanly if missing. +func describeFlakeUpdate(existing, resolved *lock.Package) string { + var parts []string + if oldRev, newRev := shortRev(existing.Resolved), shortRev(resolved.Resolved); oldRev != "" && newRev != "" { + parts = append(parts, fmt.Sprintf("%s -> %s", oldRev, newRev)) + } + if oldDate, newDate := shortDate(existing.LastModified), shortDate(resolved.LastModified); oldDate != "" && newDate != "" { + parts = append(parts, fmt.Sprintf("(%s → %s)", oldDate, newDate)) + } + return strings.Join(parts, " ") +} + +// shortRev returns the first 7 chars of the locked git rev, or "" for refs +// without one (path:, tarball:, unlocked refs). +func shortRev(resolved string) string { + installable, err := flake.ParseInstallable(resolved) + if err != nil || installable.Ref.Rev == "" { + return "" + } + if len(installable.Ref.Rev) < 7 { + return installable.Ref.Rev + } + return installable.Ref.Rev[:7] +} + +func shortDate(rfc3339 string) string { + if rfc3339 == "" { + return "" + } + t, err := time.Parse(time.RFC3339, rfc3339) + if err != nil { + return "" + } + return t.Format("2006-01-02") +} + func useResolvedPackageInLockfile( lockfile *lock.File, pkg *devpkg.Package, diff --git a/internal/devbox/update_test.go b/internal/devbox/update_test.go index 30508e2fdd5..11fa986c106 100644 --- a/internal/devbox/update_test.go +++ b/internal/devbox/update_test.go @@ -232,3 +232,155 @@ func currentSystem(*testing.T) string { sys := nix.System() // NOTE: we could mock this too, if it helps. return sys } + +func TestFlakeUpdateRewritesLockEntry(t *testing.T) { + devbox := devboxForTesting(t) + + raw := "github:numtide/flake-utils" + devPkg := devpkg.PackageFromStringWithDefaults(raw, devbox.lockfile) + oldRev := "1111111111111111111111111111111111111111" + newRev := "2222222222222222222222222222222222222222" + existing := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + oldRev, + LastModified: "2024-01-01T00:00:00Z", + } + resolved := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + newRev, + LastModified: "2025-04-22T00:00:00Z", + } + lockfile := &lock.File{ + Packages: map[string]*lock.Package{raw: existing}, + } + + err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile) + require.NoError(t, err) + require.Equal(t, "github:numtide/flake-utils/"+newRev, lockfile.Packages[raw].Resolved) + require.Equal(t, "2025-04-22T00:00:00Z", lockfile.Packages[raw].LastModified) +} + +func TestFlakeUpdateStalenessGuardRejectsOlder(t *testing.T) { + devbox := devboxForTesting(t) + + raw := "github:numtide/flake-utils" + devPkg := devpkg.PackageFromStringWithDefaults(raw, devbox.lockfile) + newerRev := "2222222222222222222222222222222222222222" + olderRev := "1111111111111111111111111111111111111111" + existing := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + newerRev, + LastModified: "2025-04-22T00:00:00Z", + } + resolved := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + olderRev, + LastModified: "2024-01-01T00:00:00Z", + } + lockfile := &lock.File{ + Packages: map[string]*lock.Package{raw: existing}, + } + + err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile) + require.NoError(t, err) + // Entry must remain on the newer rev. + require.Equal(t, "github:numtide/flake-utils/"+newerRev, lockfile.Packages[raw].Resolved) +} + +// Regression: the staleness guard must not trigger when resolved.LastModified +// is empty (some nix error paths omit it). Missing == unknown, not older. +func TestFlakeUpdateAllowsMissingResolvedLastModified(t *testing.T) { + devbox := devboxForTesting(t) + + raw := "github:numtide/flake-utils" + devPkg := devpkg.PackageFromStringWithDefaults(raw, devbox.lockfile) + oldRev := "1111111111111111111111111111111111111111" + newRev := "2222222222222222222222222222222222222222" + existing := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + oldRev, + LastModified: "2025-04-22T00:00:00Z", + } + resolved := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + newRev, + // LastModified deliberately empty. + } + lockfile := &lock.File{ + Packages: map[string]*lock.Package{raw: existing}, + } + + err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile) + require.NoError(t, err) + require.Equal(t, "github:numtide/flake-utils/"+newRev, lockfile.Packages[raw].Resolved) +} + +func TestFlakeUpdateNoOpWhenResolvedUnchanged(t *testing.T) { + devbox := devboxForTesting(t) + + raw := "github:numtide/flake-utils" + devPkg := devpkg.PackageFromStringWithDefaults(raw, devbox.lockfile) + rev := "1111111111111111111111111111111111111111" + existing := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + rev, + LastModified: "2024-01-01T00:00:00Z", + } + resolved := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + rev, + LastModified: "2024-01-01T00:00:00Z", + } + lockfile := &lock.File{ + Packages: map[string]*lock.Package{raw: existing}, + } + + err := devbox.mergeResolvedPackageToLockfile(devPkg, resolved, lockfile) + require.NoError(t, err) + require.Same(t, existing, lockfile.Packages[raw], "entry should not be replaced on no-op update") +} + +func TestShortRev(t *testing.T) { + // GitHub refs only parse as "locked" (with a Rev) if the third path + // component is a 40-char hex SHA. Anything shorter is treated as a ref + // name, not a revision. + longRev := "abc1234def56789012345678901234567890abcd" + cases := []struct { + in, want string + }{ + {"github:numtide/flake-utils/" + longRev + "#pkg", "abc1234"}, + {"path:./local", ""}, + {"", ""}, + {"not a flake ref", ""}, + } + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + require.Equal(t, tc.want, shortRev(tc.in)) + }) + } +} + +func TestShortDate(t *testing.T) { + require.Equal(t, "2025-04-22", shortDate("2025-04-22T14:30:00Z")) + require.Equal(t, "", shortDate("")) + require.Equal(t, "", shortDate("not a date")) +} + +func TestDescribeFlakeUpdateFormats(t *testing.T) { + oldRev := "abc1234def56789012345678901234567890abcd" + newRev := "f4567890123456789abcdef012345678901234ab" + oldPkg := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + oldRev + "#pkg", + LastModified: "2024-11-01T00:00:00Z", + } + newPkg := &lock.Package{ + Resolved: "github:numtide/flake-utils/" + newRev + "#pkg", + LastModified: "2025-04-22T00:00:00Z", + } + require.Equal(t, + "abc1234 -> f456789 (2024-11-01 → 2025-04-22)", + describeFlakeUpdate(oldPkg, newPkg), + ) + + // Fallback to date-only when refs have no rev (e.g. path:). + oldPath := &lock.Package{Resolved: "path:./x", LastModified: "2024-11-01T00:00:00Z"} + newPath := &lock.Package{Resolved: "path:./x", LastModified: "2025-04-22T00:00:00Z"} + require.Equal(t, "(2024-11-01 → 2025-04-22)", describeFlakeUpdate(oldPath, newPath)) + + // Fallback to rev-only when dates missing. + oldNoDate := &lock.Package{Resolved: "github:numtide/flake-utils/" + oldRev + "#pkg"} + newNoDate := &lock.Package{Resolved: "github:numtide/flake-utils/" + newRev + "#pkg"} + require.Equal(t, "abc1234 -> f456789", describeFlakeUpdate(oldNoDate, newNoDate)) +} diff --git a/internal/lock/lockfile.go b/internal/lock/lockfile.go index a3fd3070bbe..c1f7cd34f01 100644 --- a/internal/lock/lockfile.go +++ b/internal/lock/lockfile.go @@ -82,7 +82,7 @@ func (f *File) Resolve(pkg string) (*Package, error) { locked := &Package{} _, _, versioned := searcher.ParseVersionedPackage(pkg) if pkgtype.IsRunX(pkg) || versioned || pkgtype.IsFlake(pkg) { - resolved, err := f.FetchResolvedPackage(pkg) + resolved, err := f.FetchResolvedPackage(pkg, false) if err != nil { return nil, err } diff --git a/internal/lock/resolve.go b/internal/lock/resolve.go index 38873d5d127..2183d7c6024 100644 --- a/internal/lock/resolve.go +++ b/internal/lock/resolve.go @@ -28,13 +28,17 @@ import ( // not changed. This can happen when doing `devbox update` and search has // a newer hash than the lock file but same version. In that case we don't want // to update because it would be slow and wasteful. -func (f *File) FetchResolvedPackage(pkg string) (*Package, error) { +// +// When refresh is true, flake ref resolution bypasses nix's own cache via +// `--refresh`. This should only be set on the `devbox update` path; other +// callers (Add, install, outdated checks) prefer the cache. +func (f *File) FetchResolvedPackage(pkg string, refresh bool) (*Package, error) { if pkgtype.IsFlake(pkg) { installable, err := flake.ParseInstallable(pkg) if err != nil { return nil, fmt.Errorf("package %q: %v", pkg, err) } - installable.Ref, err = lockFlake(context.TODO(), installable.Ref) + installable.Ref, err = lockFlake(context.TODO(), installable.Ref, refresh) if err != nil { return nil, err } @@ -207,7 +211,7 @@ func buildLockSystemInfos(pkg *searcher.PackageVersion) (map[string]*SystemInfo, return sysInfos, nil } -func lockFlake(ctx context.Context, ref flake.Ref) (flake.Ref, error) { +func lockFlake(ctx context.Context, ref flake.Ref, refresh bool) (flake.Ref, error) { if ref.Locked() { return ref, nil } @@ -216,8 +220,9 @@ func lockFlake(ctx context.Context, ref flake.Ref) (flake.Ref, error) { // file is a bit more lenient and only requires a revision so that we // don't need to download the nixpkgs source for cached packages. If the // search index is ever able to return the NAR hash then we can remove - // this check. - if ref.Type == flake.TypeGitHub && (ref.Rev != "") { + // this check. Skip this shortcut when refresh is requested — the caller + // is asking us to re-query upstream for a newer rev. + if !refresh && ref.Type == flake.TypeGitHub && (ref.Rev != "") { return ref, nil } @@ -234,10 +239,10 @@ func lockFlake(ctx context.Context, ref flake.Ref) (flake.Ref, error) { // // That said, the logic for caching resolved versions and non-locked flake references would not // be the same. - if ref.IsNixpkgs() { + if ref.IsNixpkgs() && !refresh { meta, err = nix.ResolveCachedFlake(ctx, ref) } else { - meta, err = nix.ResolveFlake(ctx, ref) + meta, err = nix.ResolveFlake(ctx, ref, refresh) } if err != nil { diff --git a/internal/nix/flake.go b/internal/nix/flake.go index 822a91a5c3b..efdc2641cd6 100644 --- a/internal/nix/flake.go +++ b/internal/nix/flake.go @@ -22,8 +22,17 @@ type FlakeMetadata struct { Resolved flake.Ref `json:"resolved"` } -func ResolveFlake(ctx context.Context, ref flake.Ref) (FlakeMetadata, error) { - cmd := Command("flake", "metadata", "--json", ref) +// ResolveFlake runs `nix flake metadata` for the given ref. When refresh is +// true, `--refresh` is passed so nix bypasses its own eval/tarball cache and +// re-queries the upstream (e.g. GitHub) — use this on `devbox update`, not on +// paths like Add where stale-but-cached results are fine. +func ResolveFlake(ctx context.Context, ref flake.Ref, refresh bool) (FlakeMetadata, error) { + args := []any{"flake", "metadata", "--json"} + if refresh { + args = append(args, "--refresh") + } + args = append(args, ref) + cmd := Command(args...) out, err := cmd.Output(ctx) if err != nil { return FlakeMetadata{}, err @@ -38,7 +47,7 @@ func ResolveFlake(ctx context.Context, ref flake.Ref) (FlakeMetadata, error) { func ResolveCachedFlake(ctx context.Context, ref flake.Ref) (FlakeMetadata, error) { return flakeFileCache.GetOrSet(ref.String(), func() (FlakeMetadata, time.Duration, error) { - meta, err := ResolveFlake(ctx, ref) + meta, err := ResolveFlake(ctx, ref, false) if err != nil { return FlakeMetadata{}, 0, err } diff --git a/internal/nix/upgrade.go b/internal/nix/flake_update.go similarity index 76% rename from internal/nix/upgrade.go rename to internal/nix/flake_update.go index 201579ae1b3..6eb576c2903 100644 --- a/internal/nix/upgrade.go +++ b/internal/nix/flake_update.go @@ -11,14 +11,6 @@ import ( "go.jetify.com/devbox/nix" ) -func ProfileUpgrade(ProfileDir, indexOrName string) error { - return Command( - "profile", "upgrade", - "--profile", ProfileDir, - indexOrName, - ).Run(context.TODO()) -} - func FlakeUpdate(ProfileDir string) error { ux.Finfof(os.Stderr, "Running \"nix flake update\"\n") cmd := Command("flake", "update") diff --git a/internal/nix/nixprofile/upgrade.go b/internal/nix/nixprofile/upgrade.go deleted file mode 100644 index 3ed16126f95..00000000000 --- a/internal/nix/nixprofile/upgrade.go +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright 2024 Jetify Inc. and contributors. All rights reserved. -// Use of this source code is governed by the license in the LICENSE file. - -package nixprofile - -import ( - "os" - - "go.jetify.com/devbox/internal/devpkg" - "go.jetify.com/devbox/internal/lock" - "go.jetify.com/devbox/internal/nix" -) - -func ProfileUpgrade(ProfileDir string, pkg *devpkg.Package, lock *lock.File) error { - nameOrIndex, err := ProfileListNameOrIndex( - &ProfileListNameOrIndexArgs{ - Lockfile: lock, - Writer: os.Stderr, - Package: pkg, - ProfileDir: ProfileDir, - }, - ) - if err != nil { - return err - } - - return nix.ProfileUpgrade(ProfileDir, nameOrIndex) -} diff --git a/testscripts/update/update_flake_local.test.txt b/testscripts/update/update_flake_local.test.txt new file mode 100644 index 00000000000..44e9970b532 --- /dev/null +++ b/testscripts/update/update_flake_local.test.txt @@ -0,0 +1,38 @@ +# Regression guard: `devbox update` on a project that includes a local path: +# flake must not error out or emit the "Failed to update" warning. Path flakes +# are one of the cases the forgiving-error branch in the update loop has to +# handle cleanly. + +exec devbox install + +# Update with no source changes. +exec devbox update + +# No warnings about the path flake. +! stderr 'Failed to update path:' + +-- devbox.json -- +{ + "packages": ["path:my-flake"] +} + +-- my-flake/flake.nix -- +{ + description = "Test local flake"; + + inputs = { + nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable"; + flake-utils.url = "github:numtide/flake-utils"; + }; + + outputs = { self, nixpkgs, flake-utils }: + flake-utils.lib.eachDefaultSystem (system: + let + pkgs = nixpkgs.legacyPackages.${system}; + in + { + packages = { + default = pkgs.hello; + }; + }); +}