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

[perf] Install all packages to profile in a single nix command #2130

Merged
merged 7 commits into from
Jun 10, 2024

Conversation

mikeland73
Copy link
Contributor

Summary

This change installs all packages into profile with same command. If we fail due to priority conflict, we fallback to the previous one by one installation.

This significantly improves performance on projects with many packages when all packages are already in store. This aims to solve a common CICD case where nothing has changed, all packages are in store, but we need to create the .devbox directory and nix profile from scratch.

cc: @Lagoja this helps address some reported issues.

How was it tested?

Happy path:
On project with 30+ packages, delete .devbox directory and ran devbox install. This change reduced total time from 10s to 2s.

Sad path:
On new project added curl, ruby, bundler, go. ruby and bundler conflict. I ran devbox install and it correctly fell back to one by one. I inspected the profile to ensure nix did not install anything twice.

Copy link
Contributor

@josh-d2 josh-d2 left a comment

Choose a reason for hiding this comment

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

Nice, lgtm!

Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

Can you test to make sure the insecure-package scenario is properly handled?

@mikeland73 mikeland73 merged commit 5326d5b into main Jun 10, 2024
24 checks passed
@mikeland73 mikeland73 deleted the landau/perf-profile-install branch June 10, 2024 18:22
mohsenari pushed a commit that referenced this pull request Jun 10, 2024
## Summary

This change installs all packages into profile with same command. If we
fail due to priority conflict, we fallback to the previous one by one
installation.

This significantly improves performance on projects with many packages
when all packages are already in store. This aims to solve a common CICD
case where nothing has changed, all packages are in store, but we need
to create the .devbox directory and nix profile from scratch.

cc: @Lagoja this helps address some reported issues.

## How was it tested?

Happy path:
On project with 30+ packages, delete `.devbox` directory and ran `devbox
install`. This change reduced total time from 10s to 2s.

Sad path:
On new project added `curl`, `ruby`, `bundler`, `go`. `ruby` and
`bundler` conflict. I ran `devbox install` and it correctly fell back to
one by one. I inspected the profile to ensure nix did not install
anything twice.
Copy link

sentry-io bot commented Jun 19, 2024

Suspect Issues

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

  • ‼️ **Generic Error: error running "nix profile install": <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.

4 participants