Skip to content

Conversation

mikeland73
Copy link
Contributor

Summary

This shows inline progress when installing new packages (both global and local). nix profile install does this automatically but only in tty mode. @gcurtis curious if we can somehow make nix believe we're in tty mode without having to specify stdin and stdout?

Removed 2 blockers that are no longer needed:

  • PackageInstallWriter is no longer needed I don't think. It could not see the lines it was meant to supress.
  • Returning package not found is a bit redundant because all callsites of ProfileInstall already defensivelly check if package exists. If the user somehow does end up trying to install a package that doesn't exist, we still show a decent error

Fixed 2 semi-related errors:

  • global add would print success message even if nothing was installed because of errors
  • Removing a package from global devbox is impossible if the nix package was not installed

How was it tested?

devbox add curl hello bazel
devbox add global curl hello bazel

@mikeland73 mikeland73 requested review from gcurtis and savil March 16, 2023 01:03
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.

Could you share a screenshot or video of what this looks like now? Not sure I am picturing it correctly. IIRC we needed the custom writer to indent the lines...

Removing a package from global devbox is impossible if the nix package was not installed

what could add a package to devbox.json but have it be not installed?

@@ -216,14 +216,15 @@ func ProfileInstall(args *ProfileInstallArgs) error {
cmd.Env = AllowUnfreeEnv()
cmd.Args = append(cmd.Args, ExperimentalFlags()...)
cmd.Args = append(cmd.Args, args.ExtraFlags...)
cmd.Stdout = &PackageInstallWriter{args.Writer}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aside from ignoring some lines (which are no longer needed), I think we also needed this writer to indent the output:

_, err = io.WriteString(fw.Writer, "\t"+line+"\n")

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.

We would need to specify stdout/stdin anyway because they default to /dev/null. The convention is to check if the stderr file descriptor is a tty, which is what I'm guessing Nix does. As long as the command's stderr is an *os.File pointing to a tty then it should work.

Base automatically changed from landau/priority to main March 16, 2023 19:38
@mikeland73
Copy link
Contributor Author

@savil it has to be a video or a gif because the output is dynamic inline. Which also means I don't think we need the indent to make it look good. Will share.

@mikeland73
Copy link
Contributor Author

I'm a bit too lazy to make a video, this is what it looks like:

image

The lower line is dynamic and shows progress.

this is what it looks like after installing multiple packages

image

I went ahead and removed the redundant "package is now installed" and now it looks like

image

Two weird behavior nits:

  • If package is already installed, no output.
  • When installing local packages we also install the global ones which is weird.

Will address this in follow up

@mikeland73 mikeland73 merged commit f757795 into main Mar 16, 2023
@mikeland73 mikeland73 deleted the landau/progress branch March 16, 2023 21:27
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