[flakes] Add ability to add custom local flakes to devbox project #866

mikeland73 · 2023-04-05T07:09:26Z

Summary

This allows users to specify a local flake as a package in devbox.json. It can be added by using the following syntax:

path:path/to/flake#output

Where output can be a package that is part of packages or legacyPackages output. (legacyPackages is just a hack that nixpkgs uses for performance)

The path to flake can be relative or absolute. If a relative path is used, it is relative to the project dir.

TODOs: (in this PR)

devbox add doesn't work because we still look for package in nixpkgs
devbox rm doesn't work because NixProfileListItem.AttributePath is broken for these. (see TODO in code)
NixProfileListItem.PackageName and callsites needs to modified to ensure we don't re-install the flake output to the profile every time (cc: @savil for visibility)

TODOs: (in follow up)

Allow remote (github) flakes
Possible improvement: allow multiple outputs? (e.g. path:path/to/flake#output1,output2). This is not nix syntax so not sure we should support. cc: @Lagoja

How was it tested?

see flakes/php example.

devbox run "php -m | grep memcached"

gcurtis

Pretty cool! I like using flakerefs for packages. It would be nice to eventually standardize on that for all packages in devbox.json.

Let me know when this is ready for a full review.

gcurtis · 2023-04-05T14:29:47Z

examples/flakes/README.md

+
+# Local flakes (usually committed to your project)
+
+In devbox.json use "path:/path/to/flake#output" as the package name.


Are these regular flake references? If so, could we call that out in case the user is already familiar with them?

Yep, they are exactly what you use when installing a package that is a flake output. That is why nix profile install works almost as is with this change.

gcurtis · 2023-04-05T14:49:27Z

internal/nix/input.go

Flake references are just URLs, so you should be able to parse them with url.Parse. For example: https://go.dev/play/p/KSyymaLk0CN

mikeland73 · 2023-04-05T16:18:55Z

Let me know when this is ready for a full review.

Will do. Have the 3 TODOs, will get that out today.

mikeland73 · 2023-04-05T22:58:49Z

@gcurtis ready to review.

@savil can you take a look at 0e8e1ea it changes the logic to how we determine if a package is already installed in profile. It's a bit less efficient, but a lot simpler to follow.

I'm gonna measure performance and possibly add some caching to avoid calling nix profile list more than once.

savil · 2023-04-06T18:23:27Z

internal/nix/profile.go

+
+	// TODO(landau/savil): Should we use unlocked reference instead?
+	// I'm not sure we gain anything by using the locked reference. since
+	// the user specifies only the package name when installing


hmm yeah, possibly.

I can't remember why I chose to do the lockedReference, so its possible there was some scenario I ran into. But give it a try, and we can see if some edge-case doesn't work?

would recommend a separate PR

savil

Did a quick pass. Nice work!

There's a lot going on here. So, want to have another look in a bit.

Could we double-check the testscript unit tests have adequate coverage to ensure these changes work for various scenarios: with different forms of flake inputs, adding/removing multiple packages?
Maybe citing them in the Test Plan would be good.

For some of the nix.Input stuff, we could add some unit tests as well.

savil · 2023-04-06T18:34:29Z

examples/flakes/php/devbox.json

+  "nixpkgs": {
+    "commit": "f80ac848e3d6f0c12c52758c0f25c10c97ca3b62"
+  }
+}


could you add a run_test script here so this gets exercised?

savil · 2023-04-06T18:34:50Z

internal/impl/packages.go

 		}

 		// TODO: unify this with nix.ProfileRemove
 		cmd := exec.Command("nix", "profile", "remove",
 			"--profile", profileDir,
-			attrPath,
+			fmt.Sprintf("%d", index),


using index instead of (packageName, attributePath) sounds good

Could you double-check that the index is stable? Or at least stable enough for the purposes of these operations?

For example: what happens if I am installing 3 packages at the same time?

It's stable through adding but not removing. Which is why I added the ability to pass a pre-computed list. When we are removing we recompute the list every time. When adding we compute it once and then add all packages.

Computing the list every time is not very slow, but we don't want it to happen on ensure since that gets called on every shell, run etc.

In a follow up, we could even cache the list in a file which would allow a 20-30ms speed-up of ensure.

I don't know if this is the biggest perf bottleneck. 20-30 ms doesn't seem that much.

gcurtis

Can be done in a followup, but we should make sure to have some testscript + golden file test coverage before we release.

gcurtis · 2023-04-06T18:40:52Z

internal/nix/input.go

+func currentSystem() (string, error) {
+	cmd := exec.Command(
+		"nix", "eval",
+		"--impure", "--raw", "--expr",


Does this need to be impure?

probably not. I copy pasted this from the internet. I'll fix.

[flakes] Add ability to add custom local flakes to devbox project

95b7039

mikeland73 requested review from gcurtis and ipince April 5, 2023 07:09

mikeland73 changed the title ~~[flakes] Add ability to add custom local flakes to devbox project~~ [flakes][WIP] Add ability to add custom local flakes to devbox project Apr 5, 2023

gcurtis reviewed Apr 5, 2023

View reviewed changes

mikeland73 added 3 commits April 5, 2023 15:50

Ensure flakes can be compared to existing profile packages correctly

0e8e1ea

New golden files

416d594

Fix golden

cdb8e5a

mikeland73 requested a review from gcurtis April 5, 2023 22:55

Cache list

93b7876

mikeland73 changed the title ~~[flakes][WIP] Add ability to add custom local flakes to devbox project~~ [flakes] Add ability to add custom local flakes to devbox project Apr 5, 2023

mikeland73 added 2 commits April 5, 2023 16:18

Clean php example flake

d623c59

Merge branch 'main' into landau/flakes

cb9a582

Lagoja mentioned this pull request Apr 6, 2023

[Feature Request] Allow adding a package from file system #668

Closed

savil reviewed Apr 6, 2023

View reviewed changes

gcurtis approved these changes Apr 6, 2023

View reviewed changes

mikeland73 added 4 commits April 7, 2023 09:11

Merge branch 'main' into landau/flakes

0b0b54e

Add unit tests

c76fa6c

Add more tests, small fix

78a2c71

Dedup add/rm

d336a9a

mikeland73 merged commit 7a05ab9 into main Apr 7, 2023

mikeland73 deleted the landau/flakes branch April 7, 2023 21:58


		# Local flakes (usually committed to your project)

		In devbox.json use "path:/path/to/flake#output" as the package name.

[flakes] Add ability to add custom local flakes to devbox project #866

[flakes] Add ability to add custom local flakes to devbox project #866

Uh oh!

Conversation

mikeland73 commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

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!

mikeland73 commented Apr 5, 2023

Uh oh!

mikeland73 commented Apr 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Uh oh!

mikeland73 commented Apr 5, 2023 •

edited

Loading