[lockfile] add lockfile.Tidy and call from ensurePackagesAreInstalled #1109

savil · 2023-06-07T01:07:28Z

Summary

Problem:
if I change devbox.json from [email protected] to [email protected], and do devbox install, I still see the [email protected] in the devbox.lock (in addition to the new [email protected])

This PR ensures that when we update installed packages, we also update the lockfile
to have the set of packages that match those of the devbox.json config.

implementation note: I needed to rename lockfile.Packages to a different name
since devboxProject is embedded in the lockfile.File struct and we have a name
conflict.

Fixes #1095

How was it tested?

will add a testscript unit test that matches the steps below:

# initial devbox.lock file
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/devbox (savil/update-lockfile-v2)> cat devbox.lock
{
  "lockfile_version": "1",
  "packages": {
    "[email protected]": {
      "last_modified": "2023-03-31T22:52:29Z",
      "resolved": "github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#actionlint",
      "version": "1.6.23"
    },
    "[email protected]": {
      "last_modified": "2023-05-01T16:53:22Z",
      "resolved": "github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go",
      "version": "1.20.3"
    },
    "[email protected]": {
      "last_modified": "2023-05-01T16:53:22Z",
      "resolved": "github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#golangci-lint",
      "version": "1.52.2"
    }
  }
}

# edit the [email protected] to be [email protected]⏎
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/devbox (savil/update-lockfile-v2)> vim devbox.json
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/devbox (savil/update-lockfile-v2)> devbox install
Ensuring packages are installed.
Finished installing packages.


(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/devbox (savil/update-lockfile-v2)> cat devbox.lock
{
  "lockfile_version": "1",
  "packages": {
    "[email protected]": {
      "last_modified": "2023-03-31T22:52:29Z",
      "resolved": "github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#actionlint",
      "version": "1.6.23"
    },
    "[email protected]": {
      "last_modified": "2023-05-01T16:53:22Z",
      "resolved": "github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go_1_19",
      "version": "1.19.8"
    },
    "[email protected]": {
      "last_modified": "2023-05-01T16:53:22Z",
      "resolved": "github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#golangci-lint",
      "version": "1.52.2"
    }
  }
}⏎
# Notice that the older [email protected] is now gone

savil · 2023-06-07T01:07:41Z

Current dependencies on/for this PR:

main
- PR [lockfile] add lockfile.Tidy and call from ensurePackagesAreInstalled #1109 👈

This comment was auto-generated by Graphite.

savil · 2023-06-07T01:08:16Z

internal/lock/lockfile.go

@@ -127,6 +128,10 @@ func IsLegacyPackage(pkg string) bool {
 		!strings.HasPrefix(pkg, "/")
 }

+func (l *File) Tidy(project devboxProject) {
+	l.PackageMap = lo.PickByKeys(l.PackageMap, project.Packages())


@mikeland86 not 100% confident that the keys in the lockfile are intended to match the package-names in devbox.json. Can you confirm?

Yeah they should always match. Right now nix.Input uses Input.String() instead of Input.Raw as a key (raw is the package name in devbox.json) but they always match for devbox packages. I think we should probably change those callsites to use input.Raw just to be sure (since for flakes Raw and String() might not be the same)

But for this use case this is safe.

oh I see. We should probably change that sooner than later...

mikeland73

LGTM, pending revert of Packages name change. You can also remove the Packages() function

mikeland73 · 2023-06-07T05:22:31Z

internal/lock/lockfile.go

+func (l *File) Tidy(project devboxProject) {
+	l.PackageMap = lo.PickByKeys(l.PackageMap, project.Packages())


This doesn't need a parameter. The lockFile struct has access to devbox struct (i.e. l.devboxProject.Packages())

oh good point

mikeland73 · 2023-06-07T05:32:32Z

internal/lock/lockfile.go

@@ -127,6 +128,10 @@ func IsLegacyPackage(pkg string) bool {
 		!strings.HasPrefix(pkg, "/")
 }

+func (l *File) Tidy(project devboxProject) {
+	l.PackageMap = lo.PickByKeys(l.PackageMap, project.Packages())


Yeah they should always match. Right now nix.Input uses Input.String() instead of Input.Raw as a key (raw is the package name in devbox.json) but they always match for devbox packages. I think we should probably change those callsites to use input.Raw just to be sure (since for flakes Raw and String() might not be the same)

But for this use case this is safe.

mikeland73 · 2023-06-07T05:32:37Z

internal/lock/lockfile.go

@@ -22,7 +23,7 @@ type File struct {
 	resolver

 	LockFileVersion string              `json:"lockfile_version"`
-	Packages        map[string]*Package `json:"packages"`
+	PackageMap      map[string]*Package `json:"packages"`


You don't need to rename this. See this draft #1113

You can access conflicting functions on structs by specifying the embedded struct explicitly (e.g. lock.devboxProject.Packages())

That said, when embedded structs get this complicated it's probably better to get rid of them (and just make them explicit fields) (not something that needs to happen in this PR though).

that draft was useful, thanks!

That said, when embedded structs get this complicated it's probably better to get rid of them (and just make them explicit fields) (not something that needs to happen in this PR though).

yeah, that's what savil/update-lockfile-v1 was about. But I thought it may receive pushback so took this more conservative approach. See #1120

…types work

[lockfile] add lockfile.Tidy and call from ensurePackagesAreInstalled

0083f2c

savil commented Jun 7, 2023

View reviewed changes

savil requested a review from mikeland73 June 7, 2023 01:08

fix lint

0760824

mikeland73 approved these changes Jun 7, 2023

View reviewed changes

savil added 4 commits June 6, 2023 22:35

add testscript unit test

de90917

rename Packages back to PackageMap, and do function juggling to make …

eac925f

…types work

lintfix

d6f1a03

clean up input_test

7da88b7

savil mentioned this pull request Jun 7, 2023

[lockfile] add lockfile.Tidy and remove devboxProject embed #1120

Closed

savil merged commit dcce2b1 into main Jun 7, 2023

savil deleted the savil/update-lockfile-v2 branch June 7, 2023 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lockfile] add lockfile.Tidy and call from ensurePackagesAreInstalled #1109

[lockfile] add lockfile.Tidy and call from ensurePackagesAreInstalled #1109

Uh oh!

savil commented Jun 7, 2023 •

edited

Loading

Uh oh!

savil commented Jun 7, 2023

Uh oh!

savil Jun 7, 2023

Uh oh!

mikeland73 Jun 7, 2023

Uh oh!

savil Jun 7, 2023

Uh oh!

mikeland73 Jun 7, 2023

Uh oh!

mikeland73 left a comment

Uh oh!

mikeland73 Jun 7, 2023

Uh oh!

savil Jun 7, 2023

Uh oh!

mikeland73 Jun 7, 2023

Uh oh!

mikeland73 Jun 7, 2023

Uh oh!

savil Jun 7, 2023

Uh oh!

Uh oh!

		func (l *File) Tidy(project devboxProject) {
		l.PackageMap = lo.PickByKeys(l.PackageMap, project.Packages())

[lockfile] add lockfile.Tidy and call from ensurePackagesAreInstalled #1109

[lockfile] add lockfile.Tidy and call from ensurePackagesAreInstalled #1109

Uh oh!

Conversation

savil commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Uh oh!

savil commented Jun 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeland73 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savil commented Jun 7, 2023 •

edited

Loading