Skip to content

Conversation

mikeland73
Copy link
Contributor

@mikeland73 mikeland73 commented Apr 30, 2024

Summary

This changes how installNixPackagesToStore calls nix.Build so that it installs all installables in same nix command.

It does this by grouping all installables into two groups, one for allow-insecure and other for default and runs build on both groups (if not empty)

I was able to remove packageInstallErrorHandler because it is already called in packagesToInstallInStore so we will never actually try to build something that can't build on the system, or if it requires --allow-insecure we will notice it then.

This does change our segment logging to group packages together for install times, so we will need to do some more work on the metrics side to determine slow packages.

How was it tested?

  • Installed multiple packages not present in my nix store at once, including a combination of secure and insecure ones.
  • Tried to install something that doesn't build on my system, saw correct error message

@mikeland73 mikeland73 requested review from gcurtis and savil April 30, 2024 21:18
@savil
Copy link
Collaborator

savil commented Apr 30, 2024

For the scenario where a user clones a repo and does devbox install, this PR's change will mean they have to wait a long time without much UI information about what is being installed. What does the UX look like?

Alternatively, we could preserve the old-behavior that prints output for each package, while having a --parallel flag or DEVBOX_INSTALL_PARALLEL env-var setting that runs it in parallel. EDIT: "parallel" is incorrect. Maybe "--bulk" or something? not sure.

Copy link
Collaborator

@gcurtis gcurtis left a comment

Choose a reason for hiding this comment

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

LGTM.

@savil I think the user will see progress via the nix build command because it's hooked up to stderr.

@mikeland73
Copy link
Contributor Author

@savil like @gcurtis mentioned, this does show output the entire time.

One thing this removed is the package by package manual output we used to do, specifically fmt.Sprintf("[%d/%d] %s", stepNum, total, pkg)

in it's place we now do:

ux.Finfo(
		d.stderr,
		"Installing the following packages to the nix store: %s\n",
		strings.Join(packageNames, ", "),
	)

@mikeland73 mikeland73 merged commit bc719ca into main May 1, 2024
@mikeland73 mikeland73 deleted the landau/build-all-at-once branch May 1, 2024 18:00
Copy link

sentry-io bot commented May 2, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue

Did you find this useful? React with a 👍 or 👎

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.

3 participants