-
Notifications
You must be signed in to change notification settings - Fork 1.4k
WIP: New and user selectable progress bars #7897
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
base: main
Are you sure you want to change the base?
Conversation
I wonder if it would make sense for items that end up rounded as "0s" to drop down to milliseconds instead? Otherwise 0s doesn't seem believable on its own, and a lot of "0s" can accumulate to a substantial number, which also would be confusing. |
Probably! I'd love feedback and suggestions :D |
I wouldn't mind some of this now like the number of sources and compile durations. Is it easy to separate into a smaller PR? |
Also can we show whether something is compiling as a tool (host) or destination (target)? (update: never mind, we build the plug-in tools before we do the production build, sigh). |
Side note: should the "Write sources" tasks display name be more informational? |
I'd consider that a "full note"! |
It still useful because we build swift-syntax for the host (for macros) as part of the real build. |
d058b30
to
3e19adc
Compare
@dschaefer2 @MaxDesiatov ![]() |
oops I meant to ping @neonichu not Doug 😓 |
This looks nice. Would it be possible to add some automated tests to validate the expected behaviour? This way, we will ensure the feature does not regress. |
Rauhul and I chatted about this today. We acknowledged the need but also the difficulty writing tests for it. Maybe we can get some help with that. Do we have tests for the current progress messages? |
@rauhul nice! Progress bar for building and progress bar for Swift Testing could/should most likely share implementation - a generic implementation of the CLI based GUI for displaying and updating some progress bars - with the difference being the event stream of updates. Swift Testing does not have SPM as dependency in Package.swift though, hmm, perhaps a new Swift package for displaying progress in CLI could be created and added as a dependency to both Swift Testing and SPM? |
This is very cool! I am a big fan of making progress related output more informative and fun, since I think we all spend a good amount of our time looking at it. That said, I think there are some use cases and nuances to keep in mind. @stmontgomery and @grynspan raised some interesting comments on @Sajjon's swift-testing progress bar patch that I think apply here as well: The relevant bullet points from their comments:
|
With @plemarquand's notes in mind, I'd love to get this all packaged up somewhere that Swift Testing could link to it and use it too. :) |
There are other use cases I know that could use good terminal progress animation in a Swift package such as swift-testing. I'd like to see turning this into that as a long term goal. Also this is an optional view of progress. I think we still need the old one around for tools that depend on it. vscode-swift currently is one of those tools. But talking with @award999, we'd probably be better off with that interface being a streaming machine interface rather than trying to parse human consumable output. We need to be free to make changes to improve human consumption, like we are here. |
e974a4d
to
f3891a5
Compare
I'd definitely also would be interested in consider using it for the Benchmark package if it's broken out to a separate dependency in the future. |
f3891a5
to
1f9da2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Any recordings of the various animations? I'm curious about Ninja and Blast :)
} | ||
} | ||
|
||
class BlastProgressAnimation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this class can be final
?
} | ||
progressAnimation.update(step: step, total: 100, text: message) | ||
progressAnimation.complete("Build complete!") | ||
// case .didUpdateProgress(let progressInfo): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code
@@ -260,12 +263,14 @@ public final class SwiftBuildSystem: SPMBuildCore.BuildSystem { | |||
let parameters = try self.makeBuildParameters() | |||
let derivedDataPath = self.buildParameters.dataPath.pathString | |||
|
|||
let progressAnimation = ProgressAnimation.percent( | |||
// Figure out which progress bar we have to use during the build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing TODO? Or is comment obsolete?
self.onTaskProgressUpdateText?(text, targetName) | ||
if let task = self.progressText(of: message, targetName: targetName) { | ||
self.swiftTaskProgressTexts[info.pid] = task | ||
// self.progressAnimation.update(id: info.pid, name: task, event: .discovered, at: time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code
self.swiftTaskProgressTexts[info.pid] = nil | ||
if let task = self.swiftTaskProgressTexts[info.pid] { | ||
swiftTaskProgressTexts[info.pid] = nil | ||
// self.progressAnimation.update(id: info.pid, name: task, event: .completed(.succeeded), at: time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code
progressTracker.buildStart(configuration: configuration) | ||
progressTracker.buildStart( | ||
configuration: configuration, | ||
action: "Building") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we might benefit from a:
extension String {
static let building: Self = "Building"
}
and use that instead of "Building"
String literal in many places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would want to use an enum
conforming to CustomStringConvertible
rather than extending a currency type like String
.
if env.runningInXcode { return .noColors } | ||
// Disabled colors because output stream is not a tty, CI == nil, | ||
// and CLICOLOR_FORCE == nil. | ||
// FIXME: dont use stream.isTTY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any resolution for this FIXME? Any suggestions for improvement?
//===----------------------------------------------------------------------===// | ||
|
||
// FIXME: use task tree | ||
//struct ProgressTask { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code
this is very much a work in progress. It works enough to be helpful locally for myself.
Screen.Recording.2024-08-20.at.9.52.03.AM.mov
TODO:
Larger example output, the new progress bar makes it really easy to see duplicate work:
