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

livecheck: progress bar for JSON output #8577

Merged
merged 2 commits into from Sep 18, 2020
Merged

livecheck: progress bar for JSON output #8577

merged 2 commits into from Sep 18, 2020

Conversation

nandahkrishna
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This PR adds a progress bar for livecheck's JSON output. We often use the --json flag when we have too many formulae to check and would like to redirect the livecheck output to a file, maybe to use later in a script or to compare livecheck outputs after making changes to livecheckables.

Currently, no feedback or progress information is provided, and we're left wondering how long it's going to take until livecheck is done. The addition of the progress bar keeps us updated on the status and helps us anticipate how much longer the process would take. I used the ruby-progressbar gem as I was unable to find any other command or module which had a progress bar (curl uses its own --progress-bar option), so do let me know if I should be doing it some other way.

This progress bar is temporary – it stays on-screen until livecheck is done preparing the JSON output, following which it is erased (for a TTY stdout). This 'feature' required the addition of some additional SPECIAL_CODES to the Tty module (to clear a line and move up a line, I added a few more to make sure we have a 'complete' set).

I think I've covered all the details here. Looking forward to your comments/suggestions!

CC @samford

@issyl0
Copy link
Member

issyl0 commented Sep 2, 2020

I was going to ask how long a livecheck run usually takes, but I decided to try it myself:

issyl0 on ophiuchus in /usr/local/Homebrew on livecheck-json-progress via 💎 v2.6.3
➜ brew livecheck --all --json
==> Generating JSON
############## 398/5530

And it's still going. I can see why this might be good to have!

Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

This is useful for when you're doing a run where you're not redirecting the output to a file and does deal with the issue where we have to wait in silence until livecheck spits out all the JSON at the end. I added a suggestion that provides a different progress bar format which may be a little more pleasant in some respects.

Unfortunately, the progress bar doesn't seem to be compatible with redirecting the output to a file. I do something like brew livecheck --json --tap homebrew/core > some_directory/filename.json very frequently and this ends up producing a file that isn't valid JSON since the progress bar output appears at the beginning of the file.

One way of dealing with this would be to add a CLI option like --output-file (or something like that), where livecheck would write the JSON (making sure we can create the file at the destination before doing the work, of course). I imagine we would only allow this option when the --json flag is also present (until/unless someone comes along asking to do this for the normal output). This would allow us to have the best of both worlds, where we could have the progress bar while also outputting JSON to a file. This particular setup is exactly what I'm looking for.

The other issue is that the progress bar output would also appear in the output if you were to run the command in a script and capture the output (as happens in the utils/livecheck_formula.rb file that brew bump uses), so this would also result in invalid JSON. I added a suggestion of one potential way to address this but we may end up having to put the progress bar output behind a flag (e.g., --progress) to make it opt-in rather than opt-out. I imagine users would expect the --json flag to only output JSON under any circumstances and this breaks that contract.

Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
@nandahkrishna
Copy link
Member Author

With the latest push:

  • Fixed a situation in which the head downloader output wouldn't be suppressed for a non-HEAD-only formula (but the installed version is head) – replaced formula.head.downloader.shutup! if formula.head? with formula.head&.downloader&.shutup!.
  • Added an --output-file option.
  • Added the $stdout.tty? condition to print the progress bar.
  • Using --quiet suppresses the progress bar, and the help string for the flag includes this information.
  • The ruby-progressbar dependency is now vendored (removed from .gitignore).
  • Updated the command completions and manpage.

I think I've covered most (if not all) changes made. Let me know if anything else should be changed or improved. Thanks!

Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

I can only comment on the changes within livecheck (and I'll rely on someone else to say whether the other parts are correct) but I think this should be good from my perspective after these final comments are resolved.

I did a run with the progress bar and --output-file= and it's quite nice 👍

Library/Homebrew/dev-cmd/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
@dtrodrigues
Copy link
Member

dtrodrigues commented Sep 4, 2020

This isn't strictly needed given the --output-file option, but what about also/instead having the progress bar write to stderr instead of stdout which would allow you to do brew livecheck --json > output.json while still getting the progress bar on the screen and not polluting the output file?

@nandahkrishna
Copy link
Member Author

nandahkrishna commented Sep 4, 2020

Now that we have a solution to the redirection problem by writing the progress bar to stderr, do we still need --output-file?

@dtrodrigues
Copy link
Member

I personally think there would be no need for --output-file and it would make the codebase simpler without it, but there may be a use case I'm not thinking of.

@MikeMcQuaid
Copy link
Member

I personally think there would be no need for --output-file and it would make the codebase simpler without it, but there may be a use case I'm not thinking of.

Agreed. I don't think we have any other commands that take it but many that are designed to work differently if brew $COMMAND > file (which should be handled by the TTY check here).

Library/Homebrew/livecheck/livecheck.rb Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/utils/tty.rb Outdated Show resolved Hide resolved
@nandahkrishna
Copy link
Member Author

nandahkrishna commented Sep 4, 2020

With the current utils/tty implementation, redirection of stdout is handled (we make sure we check if stdout is a TTY in order to print the special characters and colours).

However the problems are:

  • Redirection of only stderr to a file: if there are coloured outputs in stderr, the special characters feature in the file. For example, without a watchlist file, brew livecheck 2> test.txt results in text.txt containing colour-code characters.
  • Redirection of only stdout: stderr loses colour output.

I tried adding an @output variable to utils/tty, which would be $stdout by default – so we could check for only one of stderr or stdout depending on the situation. However, this would require a large number of changes which I believe should be another PR.

Any thoughts on how we should move forward? To me, it seems like we should revert to the --output-file setup for now, get the tty changes done (if they can/should be) and then fix this again.

@samford
Copy link
Member

samford commented Sep 5, 2020

Any thoughts on how we should move forward?

The general consensus is that we shouldn't have an --output-file flag and should just rely on output redirection, so we shouldn't reintroduce that. It seems like the options are:

  1. Go back to printing the progress bar on stdout for the time being and temporarily live with the progress bar not displaying when stdout is redirected to a file (e.g., brew livecheck --json > file.json).
  2. Continue printing the progress bar on stderr but don't modify the $stdout conditions in utils/tty.rb. The progress bar wouldn't have color and wouldn't be cleared when stdout is redirected to a file but this would at least allow us to have some form of the progress bar in this situation for the interim time.
  3. Handle the Tty changes in a separate PR and simply return to this PR (rebasing and merging) after they're done.

I think how we proceed here depends on any Tty changes and how much work would be involved in a PR for that. I'll defer to @MikeMcQuaid on whether your Tty suggestion (i.e., an @output instance variable) is the right way to go or if there's another option.

@nandahkrishna
Copy link
Member Author

I'll go ahead with option 3 and get the Tty PR done, so we can make some progress (no pun intended, haha). I'll return to this once the other issues have been sorted out.

@nandahkrishna nandahkrishna added the in progress Maintainers are working on this label Sep 5, 2020
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Fine with me once @samford is happy. Thanks @nandahkrishna!

@samford
Copy link
Member

samford commented Sep 17, 2020

I tested this branch locally by rebasing on the latest main branch and making some Tty-related changes (with respect to #8624). With the changes in place, brew livecheck --json > some_file.json properly displays color in the $stderr output and clears the output (i.e., ==> Running checks and the progress bar) when the run finishes. I did a full JSON run across the homebrew/core formulae with no problems.

I think this should be good after the related changes have been incorporated into this PR.

nandahkrishna and others added 2 commits September 18, 2020 02:40
Co-authored-by: Sam Ford <1584702+samford@users.noreply.github.com>
Co-authored-by: Dustin Rodrigues <dust.rod@gmail.com>
@nandahkrishna
Copy link
Member Author

@samford: I've rebased and incorporated the changes, let me know if this is good to merge, thanks.

Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Thanks, Nanda!

@nandahkrishna nandahkrishna merged commit 65e51da into Homebrew:master Sep 18, 2020
@nandahkrishna nandahkrishna deleted the livecheck-json-progress branch September 18, 2020 02:01
@nandahkrishna nandahkrishna removed the in progress Maintainers are working on this label Sep 18, 2020
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 11, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants