Skip to content

Commit 617b0a4

Browse files
authored
[bugfix] Write back lockfile after devbox update (#1291)
## Summary After calling `devbox update`, we were not writing back to `devbox.lock`. `ensurePkgsAreInstalled` would then return early because the local lock file hadn't changed (since it re-reads `devbox.lock` from disk, rather than using the updated in-memory lockfile), so it would skip writing the lockfile as well. This also makes the lockfile a bit more self-healing, since it will overwrite the current user's sysInfo entirely, rather than just adding the CAStorePath. This ensures that the StorePath and CAStorePath are written together. ## How was it tested? Manually, but plan to add testscripts in separate PR. I tested a few different scenarios, notably: 1. `devbox update` adds system info for current user without changing system info for other systems. 2. If current system info is missing something (either CAStorePath or Path), `devbox update` will restore it. 3. If, say, CAStorePath is missing for the current system, and the user calls `devbox shell`, then we'll print a warning instructing the user to run `devbox update`. Calling it will then actually fix the lockfile and the warning will not be reprinted. It's worth mentioning that the warning will only be shown once to the user, because then they'll cache print-dev-env, so we won't re-exercise the path that computes CAStorePath. Not super ideal, but good enough I think. 4. If current system info is missing entirely, it is added. 5. If current system info exists and matches new one, nothing is done and we print "Already up-to-date"
1 parent 8ddaa38 commit 617b0a4

File tree

3 files changed

+22
-23
lines changed

3 files changed

+22
-23
lines changed

internal/devpkg/package.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -518,8 +518,8 @@ func (p *Package) ContentAddressedPath() (string, error) {
518518

519519
ux.Fwarning(
520520
os.Stderr,
521-
"calculating ca_store_path. This may be slow. "+
522-
"Run `devbox update` to speed this up for next time.\n",
521+
fmt.Sprintf("calculating ca_store_path for %s. This may be slow. "+
522+
"Run `devbox update` to speed this up for next time.\n", sysInfo.StorePath),
523523
)
524524
localPath, err := nix.ContentAddressedStorePath(sysInfo.StorePath)
525525
if err != nil {

internal/impl/update.go

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,10 @@ func (d *Devbox) Update(ctx context.Context, pkgs ...string) error {
5454
}
5555
}
5656

57+
if err := d.lockfile.Save(); err != nil {
58+
return err
59+
}
60+
5761
if err := d.ensurePackagesAreInstalled(ctx, ensure); err != nil {
5862
return err
5963
}
@@ -107,34 +111,22 @@ func (d *Devbox) updateDevboxPackage(
107111
return nil
108112
}
109113

110-
// Check if the package's system info is missing, or not complete.
114+
// Add any missing system infos for packages whose versions did not change.
111115
if featureflag.RemoveNixpkgs.Enabled() {
112116
userSystem, err := nix.System()
113117
if err != nil {
114118
return err
115119
}
116120

117-
// If the newEntry has a system info for the user's system,
118-
// then check if we need to update system info
119-
if newEntry.Systems[userSystem] != nil {
120-
121-
// Check if the system info is missing for the user's system.
122-
sysInfo := d.lockfile.Packages[pkg.Raw].Systems[userSystem]
123-
if sysInfo == nil {
121+
// If the newEntry has a system info for the user's system, then add/overwrite it. We don't overwrite
122+
// other system infos because we don't want to clobber system-dependent CAStorePaths.
123+
if newSysInfo, ok := newEntry.Systems[userSystem]; ok {
124+
if !newSysInfo.Equals(existing.Systems[userSystem]) {
125+
// We only guard this so that the ux messaging is accurate. We could overwrite every time.
124126
if d.lockfile.Packages[pkg.Raw].Systems == nil {
125127
d.lockfile.Packages[pkg.Raw].Systems = map[string]*lock.SystemInfo{}
126128
}
127-
d.lockfile.Packages[pkg.Raw].Systems[userSystem] = newEntry.Systems[userSystem]
128-
ux.Finfo(d.writer, "Updated system information for %s\n", pkg)
129-
return nil
130-
}
131-
132-
// Check if the CAStorePath is missing for the user's system.
133-
// Since any one user cannot add this field for all systems,
134-
// we'll need to progressively add it to a project's lockfile.
135-
if sysInfo.CAStorePath == "" {
136-
// Update the CAStorePath for the user's system
137-
d.lockfile.Packages[pkg.Raw].Systems[userSystem].CAStorePath = newEntry.Systems[userSystem].CAStorePath
129+
d.lockfile.Packages[pkg.Raw].Systems[userSystem] = newSysInfo
138130
ux.Finfo(d.writer, "Updated system information for %s\n", pkg)
139131
return nil
140132
}

internal/lock/package.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,11 @@ type Package struct {
2222
type SystemInfo struct {
2323
// StorePath is the input-addressed path for the nix package in /nix/store
2424
// It is the cache key in the Binary Cache Store (cache.nixos.org)
25-
// It is of the form <hash>-<name>-<version>
25+
// It is of the form /nix/store/<hash>-<name>-<version>
2626
// <name> may be different from the canonicalName so we store the full store path.
2727
StorePath string `json:"store_path,omitempty"`
2828
// CAStorePath is the content-addressed path for the nix package in /nix/store
29-
// It is of the form <hash>-<name>-<version>
29+
// It is of the form /nix/store/<hash>-<name>-<version>
3030
CAStorePath string `json:"ca_store_path,omitempty"`
3131
}
3232

@@ -43,3 +43,10 @@ func (p *Package) IsAllowInsecure() bool {
4343
}
4444
return p.AllowInsecure
4545
}
46+
47+
func (i *SystemInfo) Equals(other *SystemInfo) bool {
48+
if i == nil || other == nil {
49+
return i == other
50+
}
51+
return *i == *other
52+
}

0 commit comments

Comments
 (0)