[bugfix] Write back lockfile after devbox update #1291
Conversation
internal/lock/package.go
Outdated
| if i == nil { | ||
| return other == nil | ||
| } | ||
| if other == nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
if i == nil || other == nil {
return i == other
}
return *i == *other| @@ -54,6 +54,10 @@ func (d *Devbox) Update(ctx context.Context, pkgs ...string) error { | |||
| } | |||
| } | |||
|
|
|||
| if err := d.lockfile.Save(); err != nil { | |||
There was a problem hiding this comment.
One downside to this approach is that if anything breaks while installing packages (which is fairly common due to stuff like insecure packages) then we leave the lockfile in a bad state that doesn't represent reality. This would have happened while updating nodejs@16
An alternative approach is to have local.IsUpToDate take a lock file instead of reading from disk. This would guarantee not running into this issue again. Similarly, localLock.Update() would take a lock file.
Not for this PR, but I think we could improve the local lock file API. Specifically I would make it private and just have the lock struct interact with it. e.g.
d.lockfile.LocalIsUpToDate()
And privatize local.Update (just have it be called by lock.Save() since they are both called together anyway.
This would make all the local stuff private.
There was a problem hiding this comment.
An alternative approach is to have local.IsUpToDate take a lock file instead of reading from disk.
Yeah.. I have an implementation of this, but I prefer to get this in and later think about how to refactor the local lock stuff, since it may have larger implications.
## Summary Follow up to #1291. This simplified the lockfile interface and mostly removes "local" lock from public view. It addresses this comment: #1291 (comment) ## How was it tested? `devbox update` on a package that needs system info downloaded.
Summary
After calling
devbox update, we were not writing back todevbox.lock.ensurePkgsAreInstalledwould then return early because the local lock file hadn't changed (since it re-readsdevbox.lockfrom 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:
devbox updateadds system info for current user without changing system info for other systems.devbox updatewill restore it.devbox shell, then we'll print a warning instructing the user to rundevbox 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.