Skip to content

Conversation

mikeland73
Copy link
Contributor

Summary

I was implementing a progress indicator for global install, and the profile install retry logic was making it a bit complicated. I then realized that the order we were assigning to priorities (latter package takes precedence) is actually inverse to what flake buildInputs does (it uses earlier precedence). In practice, this means the profile generated by devbox and the flake generated by devbox would have different behaviors. This is not terrible but also not ideal. So I removed the sorting and added some documentation to ensure we always keep the same (local, global) order.

Finally, I tried out a slightly different approach which doesn't require a profile install with retries. By reading the profile manifest file we're able to see what the highest number existing priority (lowest priority) is and just increment it to achieve the next lowest priority. I tried this up to numbers in the thousands there doesn't seem to be any limits.

Some benefits of this approach:

  • It's easier to implement progress indicators that read stdout/err in a goroutine.
  • Better performance (no retries needed)
  • more deterministic
  • Code is a bit simpler

Drawbacks:

  • Depends on the manifest
  • Needlessly assigns more priorities than are needed

@savil curious your thoughts on this?

How was it tested?

devbox global add php82
devbox add php
devbox run "php -v"

@mikeland73 mikeland73 requested review from gcurtis and savil March 15, 2023 22:23
@mikeland73 mikeland73 marked this pull request as ready for review March 15, 2023 22:23
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.

Oh good catch on the buildInputs behavior. Just to confirm: did you try it out to verify?
Asking because I'd heard the inverse last time and so implemented that order :)

I really like the clever implementation using the priority from manifest.json!

return errors.Wrapf(err, "Command: %s", cmd)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

in line 227 above, lets add:

fmt.Fprintf(args.Writer, "%s: ", stepMsg)
color.New(color.FgRed).Fprintf(args.Writer, "Fail\n")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mind if I push this to the stacked diff? That changes this logic because it no longer uses custom stdout

That way I can test how this will look with the new tty nix command.

}

var m manifest
return m, json.Unmarshal(data, &m)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm glad flakes changed the manifest from .nix to .json so we can do this :)

@mikeland73
Copy link
Contributor Author

@savil yeah, I think I was wrong on precedence before:

image

image

@mikeland73 mikeland73 merged commit bf52b9a into main Mar 16, 2023
@mikeland73 mikeland73 deleted the landau/priority branch March 16, 2023 19:38
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