Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX] Better dedup packages we nix build, and improve UX printouts #1827

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

savil
Copy link
Collaborator

@savil savil commented Feb 21, 2024

Summary

We have two separate steps for "installing packages".

  1. We use nix build to add to the nix store.
  2. We later ensure the nix profile is updated to have items pointing to the nix store packages.

For the first step, we should only run nix build if the package's outputs are not in the nix store. For this, we check nix store ls <path>. We get <path> from nix path-info <installable>.

Previously, we were checking if the packages were in the nix profile, rather than directly querying the nix store. This would lead to re-running nix build for when we've done devbox add outside a devbox-environment, and then started a devbox-environment (example: doing devbox add followed by devbox shell).

For the second step, we can remove the [1/3] <package> being removed print outs. This was already removed for packages being added. Reason: this step should be quick! We've already added the packages to the nix store via nix build. It also confuses users who are likely not aware of a nix profile concept, nor should we make them aware of it.

Alternatives:
Also tried nix build --profile <path> <installable> but this doesn't seem to return the installed package in nix profile list. Not sure why, but it doesmean its not useful for de-duplicating packages that remain to be installed.

Upside:
Ths store-path deduping is great. Works much faster. Re-doing devbox add <package> for a package already in /nix/store is much faster since we skip the nix build step entirely. As it should be.

Downside:
A user adding or removing packages directly from editing the devbox.json will not see any nice per-package-steps installation output. (Note, this was changed when adding nix-proflile-from-flake, but mentioning for completeness of analyzing the UX)

How was it tested?

devbox add a package already in store no longer shows the per-package installation steps
devbox shell subsequently does not again show we are adding the package.

devbox add a package not in store does show the per-package-steps installation output.

Need to do more rigorous testing:

  • add a local flake
  • add a remote flake

Copy link
Collaborator Author

savil commented Feb 21, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@savil savil force-pushed the savil/check-store-before-install branch from 1db57d9 to 2902a80 Compare February 21, 2024 20:15
@savil savil force-pushed the savil/check-store-before-install branch from 2902a80 to c842714 Compare February 21, 2024 20:19
@gcurtis
Copy link
Collaborator

gcurtis commented Feb 21, 2024

You might want to try out nix build --log-format internal-json to get a structured log of which packages Nix is building (or downloading). There's also a --dry-run flag if it helps to know what needs to be build ahead of time.

Maybe I'm misunderstanding the downside, but it sounds like not seeing any output is okay if it's fast. Ideally things would be fast enough that we wouldn't need to output anything at all!

@savil
Copy link
Collaborator Author

savil commented Feb 21, 2024

@gcurtis to clarify, prior to this PR the de-duplication was happening based on the nix profile even for the nix build. Hence this PR.

I added this to the Summary to clarify:

Previously, we were doing this de-duplication by checking for missing package from the nix profile, rather than directly querying the nix store.

Regarding:

You might want to try out nix build --log-format internal-json to get a structured log of which packages Nix is building (or downloading). There's also a --dry-run flag if it helps to know what needs to be build ahead of time.

IIUC I think this would be useful to show the incremental progress being made while installing. @mikeland73 yesterday made a change to enable this output to be shown. See #1826

@savil savil force-pushed the savil/check-store-before-install branch from 5d663fc to 2415de2 Compare February 22, 2024 04:52
@savil
Copy link
Collaborator Author

savil commented Feb 22, 2024

For completeness, I'll share my findings.

nix build --log-format internal-json doesn't seem to have useful output:

❯ nix build --log-format internal-json  github:nixos/nixpkgs/5233fd2ba76a3accb5aaa999c00509a11fd0793c#cowsay --extra-experimental-features 'nix-command flakes' --
json
@nix {"action":"start","id":406196532019200,"level":4,"parent":0,"text":"evaluating derivation 'github:nixos/nixpkgs/5233fd2ba76a3accb5aaa999c00509a11fd0793c#cowsay'","type":0}
@nix {"action":"stop","id":406196532019200}
@nix {"action":"start","id":406213711888384,"level":6,"parent":0,"text":"querying info about missing paths","type":0}
@nix {"action":"stop","id":406213711888384}
@nix {"action":"start","id":406213711888385,"level":0,"parent":0,"text":"","type":102}
@nix {"action":"start","id":406213711888386,"level":0,"parent":0,"text":"","type":104}
@nix {"action":"start","id":406213711888387,"level":0,"parent":0,"text":"","type":103}
@nix {"action":"result","fields":[0,1,0,0],"id":406213711888386,"type":105}
@nix {"action":"result","fields":[0,0,0,0],"id":406213711888387,"type":105}
@nix {"action":"result","fields":[101,0],"id":406213711888385,"type":106}
@nix {"action":"result","fields":[100,0],"id":406213711888385,"type":106}
@nix {"action":"start","id":406213711888388,"level":6,"parent":0,"text":"querying info about missing paths","type":0}
@nix {"action":"stop","id":406213711888388}
@nix {"action":"result","fields":[0,0,0,0],"id":406213711888386,"type":105}
@nix {"action":"result","fields":[0,0,0,0],"id":406213711888387,"type":105}
@nix {"action":"result","fields":[101,0],"id":406213711888385,"type":106}
@nix {"action":"result","fields":[100,0],"id":406213711888385,"type":106}
@nix {"action":"stop","id":406213711888387}
@nix {"action":"stop","id":406213711888386}
@nix {"action":"stop","id":406213711888385}

nix build --json --dry-run is more promising. We could pick out the store-paths from the outputs below:

❯ nix build github:nixos/nixpkgs/5233fd2ba76a3accb5aaa999c00509a11fd0793c#cowsay --extra-experimental-features 'nix-command flakes' --json --dry-run
[{"drvPath":"/nix/store/wp73jzcp45x22a54jmb3x5hz9fhaf6y4-cowsay-3.04.drv","outputs":{"man":"/nix/store/njfycpgk5ky9q53h6wmh2z131gh64wfn-cowsay-3.04-man","out":"/nix/store/awaw8fix4bcjxsyg86srbdr3pxkw8wqg-cowsay-3.04"}}]

but the format can change based on the <installable> kind:

❯ nix build --dry-run --json /nix/store/awaw8fix4bcjxsyg86srbdr3pxkw8wqg-cowsay-3.04 --extra-experimental-features nix-command
["/nix/store/awaw8fix4bcjxsyg86srbdr3pxkw8wqg-cowsay-3.04"]

In contrast, nix path-info output is more predictable for the <installable> variants I've seen, so feels better to continue with that.

@savil
Copy link
Collaborator Author

savil commented Feb 22, 2024

Putting up for review. The stacks_lapp and stacks_lepp tests are being flaky. Will look into separately.

@savil savil marked this pull request as ready for review February 22, 2024 16:53
Copy link
Contributor

@mikeland73 mikeland73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Can you confirm that doing devbox install on a fresh project with a few packages shows output.

Should we rename packagesToInstallInProfile to packagesToInstallInStore ?

@savil savil force-pushed the savil/check-store-before-install branch from 2415de2 to c23bdcb Compare February 23, 2024 00:58
@savil
Copy link
Collaborator Author

savil commented Feb 23, 2024

LGTM!

Can you confirm that doing devbox install on a fresh project with a few packages shows output.

confirmed!

Should we rename packagesToInstallInProfile to packagesToInstallInStore ?

yes, done!

@savil savil merged commit 10c436f into main Feb 23, 2024
22 of 25 checks passed
@savil savil deleted the savil/check-store-before-install branch February 23, 2024 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants