[remove nixpkgs] add toPath, and edit devbox.lock only on update command #1256

savil · 2023-07-07T00:11:44Z

Summary

This PR rolls in a few fixes:

Add toPath to builtins.fetchClosure
- lets us remove --impure flag in nix print-dev-env
Add ca_store_path to devbox.lock
- Because this is system-dependent, and is calculated locally, a devbox update will only update the user's system's ca_store_path.
- This is good for team-mates on the same system, and to avoid the slow nix store make-content-addressed <path> call on each devbox shell/run/shellenv.
Only update devbox.lock during devbox update.
- Previously, it would update system info on devbox shell/run/shellenv.
Simplify devbox.lock schema's SystemInfo to have StorePath and CAStorePath. These are used as fromPath and toPath in builtins.fetchClosure.

Miscellaneous changes:

I remove actionlint from the devbox.json. It is not core to our workflows, and I am working with very minimal space on devbox cloud VM. This reduces "out of space errors" marginally.

How was it tested?

In examples/development/go/hello-world:
devbox update and devbox shell

The devbox.lock diff is now

❯ git diff
diff --git a/examples/development/go/hello-world/devbox.lock b/examples/development/go/hello-world/
devbox.lock
index 34556d3f..168874db 100644
--- a/examples/development/go/hello-world/devbox.lock
+++ b/examples/development/go/hello-world/devbox.lock
@@ -2,9 +2,18 @@
   "lockfile_version": "1",
   "packages": {
     "[email protected]": {
-      "last_modified": "2023-05-01T16:53:22Z",
-      "resolved": "github:NixOS/nixpkgs/8670e496ffd093b60e74e7fa53526aa5920d09eb#go_1_19",
-      "version": "1.19.8"
+      "last_modified": "2023-05-03T02:55:54Z",
+      "resolved": "github:NixOS/nixpkgs/0d373d5af960504dd60c3d06c65e553b36ef29d8#go_1_19",

+      "version": "1.19.8",
+      "systems": {
+        "aarch64-darwin": {
+          "store_path": "/nix/store/7m99ip3616l3z670y83p34r3plwd4iq1-go-1.19.8"
+        },
+        "x86_64-linux": {
+          "store_path": "/nix/store/r0x0agq0vwn0p6z99vkkvn8l8a8idzsb-go-1.19.8",
+          "ca_store_path": "/nix/store/ns557l24yf1f16f9jvbmxv57qdkkn2mj-go-1.19.8"
+        }
+      }
     }
   }
-}
\ No newline at end of file
+}

LucilleH · 2023-07-07T01:00:29Z

internal/impl/update.go

+			// we'll need to progressively add it to a project's lockfile.
+			needsLocalStorePath = userSysInfo.LocalStorePath == ""
+		}
+	}


line 119 to line 128 the logic is a bit hard to follow, since you have if !needsSysInfo, needsLocalStorePath, if needsSysInfo...

Can we flatten this a bit? Something like:

var userSysInfo var localStorePath if featureflag.RemoveNixpkgs.Enabled() { userSysInfo = d.lockfile.Packages[pkg.Raw].Systems[userSystem] if userSysInfo != nil { localStorePath = userSysInfo.LocalStorePath } } if userSysInfo == nil { ... } if localStorePath == "" { ... }

gcurtis

I'm finding some of the new terms a bit confusing. Could we make them match what Nix calls them?

BinaryStorePath - this can just be StorePath. There isn't any difference between a path in a remote binary cache and a local store.
LocalStorePath - it looks like this is just the content-addressed version of the store path. Could we call it CAStorePath (or ContentAddressedStorePath if CA sounds like certificate)?
BinaryStore → BinaryCache

gcurtis · 2023-07-07T01:29:04Z

internal/nix/store.go

+// Example:
+// > nix store make-content-addressed  /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1
+// rewrote '/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1' to '/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1'
+var contentAddressedRegex = regexp.MustCompile(`rewrote\s'[\/a-z0-9-\.]+'\sto\s'([a-z0-9-\/\.]+)'`)


Does nix store make-content-addressed --json work?

oh yeah, good call. Will try that...

gcurtis · 2023-07-07T01:32:26Z

internal/nix/store_test.go

+			"/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1",
+			"/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1",


Does Nix automatically download these if they're not in the store yet?

gcurtis · 2023-07-07T01:44:05Z

internal/nix/store_test.go

+	"testing"
+)
+
+func TestContentAddressedPath(t *testing.T) {


savil · 2023-07-07T04:48:13Z

I'm finding some of the new terms a bit confusing. Could we make them match what Nix calls them?

BinaryStorePath - this can just be StorePath. There isn't any difference between a path in a remote binary cache and a local store.

LocalStorePath - it looks like this is just the content-addressed version of the store path. Could we call it CAStorePath (or ContentAddressedStorePath if CA sounds like certificate)?

BinaryStore → BinaryCache

Sounds good to me. Don't feel strongly about the current names (except I didn't want it to be "fromPath" and "toPath", since those don't make sense outside the context of fetchClosure)

savil

All CICD has passed. Going to make a small "rename" I missed out and then land...

savil · 2023-07-07T06:02:45Z

internal/lock/lockfile.go

+	StorePath string `json:"store_path,omitempty"`
+	// CAStorePath is the content-addressed path for the nix package in /nix/store
+	// It is of the form <hash>-<name>-<version>
+	CAStorePath string `json:"local_store_path,omitempty"`


oops, forgot to rename the key to ca_store_path

mikeland73 · 2023-07-07T20:10:50Z

internal/impl/update.go

+		// Check if the system info is missing for the user's system.
+		sysInfo := d.lockfile.Packages[pkg.Raw].Systems[userSystem]
+		if sysInfo == nil {
+			d.lockfile.Packages[pkg.Raw] = newEntry


Wait, could this override an existing (different) system that already has a CA path?

You are right! Much obliged 🙏🏾

sending fix: #1259

mikeland73 · 2023-07-07T20:14:07Z

internal/impl/update.go

+	}
+
+	// Check if the package's system info is missing, or not complete.
+	if featureflag.RemoveNixpkgs.Enabled() {


Most of the logic in this block could be encapsulated in lock file. Maybe:

if existing.CanBeUpdated(newEntry) { d.lockfile.Update(existing, newEntry) }

or something like that. It could deal with version changes and also missing system stuff.

That way we could make Package.Systems semi-private. (It still needs to be exported because it is part of the JSON)

Also, is this logic block missing profile install/uninstall? e.g. if a package previously required nixpkgs and we replace it with one that does not require it, do we want to update the profile?

yes, I agree this could likely be moved into the lock package. In general, I think quite a bit of the update code (beyond this block) could be better encapsulated. I may not have time to get to it though...

Also, is this logic block missing profile install/uninstall? e.g. if a package previously required nixpkgs and we replace it with one that does not require it, do we want to update the profile?

🤔 yeah, this one I am unsure about. In theory, the package's binary should be the exact same.

Its also why I think it was okay and/or put a question mark after using InputAddressedPath in the Package.Installable here.

@gcurtis would you have an opinion?

@gcurtis

… and ContentAddressedPath, and s/BinaryStore/BinaryCache (#1257) ## Summary This PR is a follow up to this request from @gcurtis: #1256 (review) BUT having `NixStorePath` and `ContentAddressedPath` seemed odd since content-addressed paths are also nix store paths. So, in this PR, I'm explicit about `NixStorePath` being `InputAddressedPath`. Also - changed `BinaryStore` to `BinaryCache`. **RFC**: I didn't change the `lock.SystemInfo` struct, I've left it as: ``` type SystemInfo struct { // StorePath is the input-addressed path for the nix package in /nix/store // It is the cache key in the Binary Cache Store (cache.nixos.org) // It is of the form <hash>-<name>-<version> // <name> may be different from the canonicalName so we store the full store path. StorePath string `json:"store_path,omitempty"` // CAStorePath is the content-addressed path for the nix package in /nix/store // It is of the form <hash>-<name>-<version> CAStorePath string `json:"ca_store_path,omitempty"` } ``` I think this could be okay, because the default `store_path` for nix is input-addressed, for now at least. If you'd like I can change `StorePath` to be explicitly: ``` InputAddrStorePath string `json:"input_addr_path, omitempty" ContentAddrStorePath string `json:"content_addr_path, omitempty" ``` Let me know! ## How was it tested? compiles

savil force-pushed the savil/to-path branch from 9a249d3 to 37ed187 Compare July 7, 2023 00:34

[remove nixpkgs] add toPath, and edit devbox.lock only on update command

85b2c95

savil force-pushed the savil/to-path branch from 37ed187 to 85b2c95 Compare July 7, 2023 00:55

LucilleH approved these changes Jul 7, 2023

View reviewed changes

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

savil marked this pull request as ready for review July 7, 2023 01:09

gcurtis reviewed Jul 7, 2023

View reviewed changes

lint fix

a735e64

savil force-pushed the savil/to-path branch from 587359d to fc414f7 Compare July 7, 2023 05:06

lucille: rewrite for clarity

aeaa38e

savil force-pushed the savil/to-path branch from fc414f7 to aeaa38e Compare July 7, 2023 05:09

savil added 2 commits July 6, 2023 22:21

gcurtis: renames, as requested. Some TODOs added to maintain scope

e00b5c6

use --json for nix store make-content-addressed

bfd8e59

savil commented Jul 7, 2023

View reviewed changes

rename lockfile key to ca_store_path

39d01b3

savil merged commit ad6725d into main Jul 7, 2023

savil deleted the savil/to-path branch July 7, 2023 06:13

savil mentioned this pull request Jul 7, 2023

[remove nixpkgs] Naming: update Package methods to InputAddressedPath and ContentAddressedPath, and s/BinaryStore/BinaryCache #1257

Merged

mikeland73 reviewed Jul 7, 2023

View reviewed changes

		"/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1",
		"/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1",

[remove nixpkgs] add toPath, and edit devbox.lock only on update command #1256

[remove nixpkgs] add toPath, and edit devbox.lock only on update command #1256

Uh oh!

Conversation

savil commented Jul 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!

Choose a reason for hiding this comment

Uh oh!

gcurtis 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!

savil commented Jul 7, 2023

Uh oh!

savil 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savil commented Jul 7, 2023 •

edited

Loading