[Outputs] Add store-paths of package outputs to devbox.lock #1814

savil · 2024-02-14T23:31:01Z

Summary

This PR updates the devbox.lock file to incorporate the expanded response from /v2/resolve api. This /v2/response incorporates store-paths for each output of the nix package.

To do so, we update lock.SystemInfo to expect []Output instead of the previous StorePath string. This SystemInfo struct needs to be backwards-compatible to read existing devbox.lock files that have the StorePath string. So, the read-path in lock.GetFile has some fallback code to handle this scenario, where the StorePath is added as Output{ Name: "out", path: <path>, Default: true}.

The main challenge with writing this PR was that we don't want to update devbox.lock unless there's been an explicit user action of the form devbox update or devbox add or devbox remove. Unfortunately, the lock.File gets internally updated by various code paths using its file.Resolve() function, and so it was (previously) hard for the lockfile
to track when an add/remove/update action happened.

To work around this, I have more explicitly invoked lockfile.Add and lockfile.Remove in the "add package" and "remove package" code paths. I think this is safe to do, but let me know if you feel a better path exists. This lets us track whether to write the lockfile with the legacy format of lock.SystemInfo (i.e. Store Path) or the modern format (i.e. []Output).

How was it tested?

devbox update in devbox-repo doesn't have changes so the lock file wasn't changed.
rm -rf .devbox && devbox shell. The lockfile wasn't changed.
devbox add prometheus added the package in the modern []Output format of SystemInfo, and also updated the other packages to use the modern SystemInfo format.

savil · 2024-02-14T23:31:14Z

Current dependencies on/for this PR:

[Outputs] Add store-paths of package outputs to devbox.lock #1814 👈
main

This stack of pull requests is managed by Graphite.

mikeland73 · 2024-02-15T02:39:56Z

internal/devbox/packages.go

+	if err := d.lockfile.Add(addedPackageNames...); err != nil {
+		return err
+	}


Main issue with this is that this saves the lock file right away. This will leave it in a bad state if something fails later on. It's also not 100% correct (see newFlakePlan calls Add)

A few alternatives:

Instead of doing lockfile.Add just do lockfile.UpdateSystemsFormat() in the 3 places you need it (add/remove/update). This would update the internal boolean. This is simplest. We can keep this function around whenever we want to update lock file formats.

In lockfile.Save() only update the format is stuff is dirty (i.e if reloading the lockfile doesn't match what is in memory). (though it may be a bit complex to tell if dirty). This is cleanest solution because it doesn't require a new bool we may forget to add.

Remove Save from Add (this is probably OK, but it feels risky). I don't love this one.

FWIW, currently newFlakePlan calls lockfile.Add on includes every time the state needs to be recomputed, so I think any project with includes will automatically update.

ugh, I thought I had checked for existing callers, but I think I missed this one...good catch

mikeland73

Added some comments with alternative approach. There's a minor bug because of previous use of Add

I think the cleanest solution that would not require boolean:

If lockfile contains store paths it is legacy.
If lockfile is dirty (i.e. loaded version doesn't match file) we modernize. (small perf hit for re-reading file, but we should rarely write to lock file and when we do, it's a slower operation anyway)

But if you run into issues implementing this, I think using a boolean is fine.

internal/lock/lockfile.go

mikeland73 · 2024-02-15T02:48:55Z

internal/lock/package.go

+	if o == nil || other == nil {
+		return o == other
+	}
+	return o.Name == other.Name && o.Path == other.Path && o.Default == other.Default


This is normal equality in golang. You can do *o == *other

mikeland73 · 2024-02-15T02:49:24Z

internal/lock/package.go

-	// It is the cache key in the Binary Cache Store (cache.nixos.org)
-	// It is of the form /nix/store/<hash>-<name>-<version>
-	// <name> may be different from the canonicalName so we store the full store path.
+	Outputs []*Output `json:"outputs,omitempty"`


Why pointers? If you make them structs you don't need the Equals function below you can just compare

a == b

mikeland73 · 2024-02-15T02:52:20Z

internal/lock/package.go

+	isEqual := true
+	if len(i.Outputs) != len(other.Outputs) {
+		return false
+	}
+	for i, o := range i.Outputs {
+		if !o.Equals(other.Outputs[i]) {
+			return false
+		}
+	}
+	return isEqual
+}


slices.Compare == 0 (assuming you don't use pointers, see comment above)

savil · 2024-02-15T19:46:06Z

If lockfile contains store paths it is legacy.

This doesn't work in the following scenario:

user reads legacy devbox.lock so lockfile has non-empty StorePath
user does devbox remove
During lockfile.Save, the lockfile will have the legacy StorePath so we'll save it in legacy format
BUT since the user did an explicit action that updates devbox.lock, we want to actually also write it in modern format.

If lockfile is dirty (i.e. loaded version doesn't match file) we modernize. (small perf hit for re-reading file, but we should rarely write to lock file and when we do, it's a slower operation anyway)

yay! this works :)
Updated code.

gcurtis

I think using "out" for legacy data is okay. I'm pretty sure the original store_path JSON field mapped to .outPath attribute of the package.

gcurtis · 2024-02-15T19:57:09Z

internal/lock/package.go

@@ -43,9 +64,50 @@ func (p *Package) IsAllowInsecure() bool {
 	return p.AllowInsecure
 }

+// Useful for debugging when we print the struct
+func (i *SystemInfo) String() string {
+	return fmt.Sprintf("SystemInfo{Outputs:%v, StorePath:%s}", i.Outputs, i.StorePath)


Suggested change

return fmt.Sprintf("SystemInfo{Outputs:%v, StorePath:%s}", i.Outputs, i.StorePath)

return fmt.Sprintf("%+v", *i)

gcurtis · 2024-02-15T20:04:37Z

internal/lock/package.go

 func (i *SystemInfo) Equals(other *SystemInfo) bool {
 	if i == nil || other == nil {
 		return i == other
 	}
-	return *i == *other
+
+	return slices.Equal(i.Outputs, other.Outputs)


Does this need to take i.StorePath into account?

I think this is OK? at least looking at the callsite https://github.com/jetpack-io/devbox/blob/main/internal/devbox/update.go#L142-L146

Only outputs matter moving forward and storePath is just kept for legacy reasons (to avoid updating everyone's lockfile)

mikeland73 · 2024-02-15T22:07:55Z

internal/lock/lockfile.go

 func (f *File) Save() error {
+	isDirty, err := f.isDirty()


sweet! forgot we had this.

mikeland73 · 2024-02-15T22:12:54Z

internal/lock/package.go

 func (i *SystemInfo) Equals(other *SystemInfo) bool {
 	if i == nil || other == nil {
 		return i == other
 	}
-	return *i == *other
+
+	return slices.Equal(i.Outputs, other.Outputs)


I think this is OK? at least looking at the callsite https://github.com/jetpack-io/devbox/blob/main/internal/devbox/update.go#L142-L146

Only outputs matter moving forward and storePath is just kept for legacy reasons (to avoid updating everyone's lockfile)

savil force-pushed the savil/outputs-lockfile branch 6 times, most recently from bdba596 to 7718ef5 Compare February 15, 2024 00:57

savil requested review from mikeland73 and gcurtis and removed request for mikeland73 February 15, 2024 02:00

savil marked this pull request as ready for review February 15, 2024 02:00

mikeland73 reviewed Feb 15, 2024

View reviewed changes

mikeland73 requested changes Feb 15, 2024

View reviewed changes

savil force-pushed the savil/outputs-lockfile branch from 52da4cf to c0f982d Compare February 15, 2024 19:45

savil requested a review from mikeland73 February 15, 2024 19:46

gcurtis approved these changes Feb 15, 2024

View reviewed changes

mikeland73 approved these changes Feb 15, 2024

View reviewed changes

savil added 3 commits February 20, 2024 16:48

[Outputs] Add store-paths of package outputs to devbox.lock

216bd7e

use isDirty to determine when to write Outputs versus StorePath

6985fbd

Write Outputs back in lockfile.Save, after the file is written to disk

e1cbd9d

savil force-pushed the savil/outputs-lockfile branch from 2ab4625 to e1cbd9d Compare February 21, 2024 00:48

requested change

fe14e45

savil merged commit 37de8f0 into main Feb 21, 2024
25 checks passed

savil deleted the savil/outputs-lockfile branch February 21, 2024 01:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Outputs] Add store-paths of package outputs to devbox.lock #1814

[Outputs] Add store-paths of package outputs to devbox.lock #1814

savil commented Feb 14, 2024 •

edited

savil commented Feb 14, 2024 •

edited

mikeland73 Feb 15, 2024 •

edited

savil Feb 15, 2024

mikeland73 left a comment

mikeland73 Feb 15, 2024

mikeland73 Feb 15, 2024

mikeland73 Feb 15, 2024

savil commented Feb 15, 2024

gcurtis left a comment

gcurtis Feb 15, 2024

gcurtis Feb 15, 2024

mikeland73 Feb 15, 2024

mikeland73 Feb 15, 2024

mikeland73 Feb 15, 2024

	return fmt.Sprintf("SystemInfo{Outputs:%v, StorePath:%s}", i.Outputs, i.StorePath)
	return fmt.Sprintf("%+v", *i)

[Outputs] Add store-paths of package outputs to devbox.lock #1814

[Outputs] Add store-paths of package outputs to devbox.lock #1814

Conversation

savil commented Feb 14, 2024 • edited

Summary

How was it tested?

savil commented Feb 14, 2024 • edited

mikeland73 Feb 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeland73 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savil commented Feb 15, 2024

gcurtis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savil commented Feb 14, 2024 •

edited

savil commented Feb 14, 2024 •

edited

mikeland73 Feb 15, 2024 •

edited