-
-
Notifications
You must be signed in to change notification settings - Fork 397
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
Print message when Wait Bar is started non-interactively #1649
Print message when Wait Bar is started non-interactively #1649
Conversation
If we're amenable to this change, I'll have to fix a lot of tests. I was also thinking it may be a good idea to augment the initial message with For instance:
|
Definitely amenable to the broad concept being described here. A user in a non-interactive session shouldn't have less information about what is going on. The |
hmm...I like this idea as well....but my initial thought is this would require setting up a thread to print while Briefcase continues on? ...hmm Looks like |
2fe3b4b
to
f81e0af
Compare
I implemented support and tests for printing static messages in lieu of the Wait Bar in non-interactive environments. I also found that we were evaluating "interactivity" based on Rich's Additionally, I updated the static message that's printed after a Wait Bar in the event of an exception. In this error condition, the status is now Finally, I adjusted how these static messages are printed so they are always contained in the log file. I'm still debating the periodic "...still working" messages; I'm somewhat concerned of the depth of the rabbit hole to implement a "threaded non-interactive wait bar"... Also, thinking about the user experience, it might be a bit disruptive; for instance, some long Wait Bars have a lot of output from the underlying task....so, injecting a "...still working" message might actually be confusing... I wonder if there's even another case where we'd want this message that isn't starting the Android emulator..? |
f81e0af
to
1db98e0
Compare
I implemented a cooperative keep alive that callers of the Wait Bar can use to signify that Briefcase is still alive and working via a Seen in CI: https://github.com/beeware/briefcase/actions/runs/7944249382/job/21689654386?pr=1649#step:25:677 There's probably room here to debate if 10 seconds is too short and whether [edit] i was a wee bit whimsical in the naming....we can revise if too much |
7397481
to
25a51a2
Compare
As a note, unlike the new Wait Bar's "started" message, the new Let me know what you think about this. I looked at how the startup for Apple's iOS simulator is implemented but it would need to be re-worked to leverage the keep-alive messages....probably using |
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.
Not much to fault here; it looks like a good abstraction (with just the right amount of YAGNI and whimsy 😄), and I agree it makes sense to rely on a little cooperation with the worker tasks, rather than try and build some multithreaded monitoring monster.
My initial impression was actually whether 10s was too long rather than too short... so I guess we can live with 10s initially, and see how living with that feels.
Marking as approved; I'm happy for this to land as is, or if you want to roll in the iOS simulator bootup change as part of this work, I'm happy to do another review pass.
I've been looking at the iOS simulator startup:
Steps 1 and 2 are mostly asynchronous, in that they defer control elsewhere and just return. So then, we're stuck at step 3 to uninstall the app which blocks until the simulator is ready to start accepting commands. And on my virtualized macOS, this takes some time. I was hoping to find a way to wait on the simulator to be ready without waiting on the command to uninstall the app. So far, I found Otherwise, maybe there's innocuous command that'll block like the uninstall command until the simulator is ready. |
Maybe an easier approach is just to lie about what Briefcase is doing. In that, while the command to uninstall the app is waiting for the simulator to be ready, we just have the Wait Bar say |
3d48758
to
6eb9de8
Compare
The simulator's operation and status is just too opaque; so, i simply implemented the keep alive spinner for uninstalling and installing the app. All the other calls are basically async and if they are blocking, then there's bigger problems. https://github.com/beeware/briefcase/actions/runs/8009151207/job/21877475631?pr=1649#step:26:128 |
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.
A minor quibble inline; plus the CI issue.
In terms of the approach itself - that seems fine. We can only cook with the food we're given, and if this approach is only obviously viable on some steps, I'm happy to go with that.
src/briefcase/platforms/iOS/xcode.py
Outdated
uninstall_popen = self.tools.subprocess.Popen( | ||
["xcrun", "simctl", "uninstall", udid, app.bundle_identifier] | ||
) | ||
with uninstall_popen: |
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 reason to no use a nested or multi-clause with statement here? e.g.,
with self.input.wait_bar(
"Uninstalling any existing app version..."
) as keep_alive, self.tools.subprocess.Popen(
["xcrun", "simctl", "uninstall", udid, app.bundle_identifier]
) as uninstall_popen:
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.
Because it's harder to mock... 🥲
But let me take a look at exactly what'll it take...should just be a few copy/pastes once I get it
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.
Re-implemented using multi-with blocks.
src/briefcase/platforms/iOS/xcode.py
Outdated
["xcrun", "simctl", "install", udid, self.binary_path(app)], | ||
check=True, | ||
with self.input.wait_bar(f"Installing new {label} version..."): | ||
install_popen = self.tools.subprocess.Popen( |
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.
Same again here with the nested context manager
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.
Implementation looks good; only issue now is the collection of CI failures
One solution for the CI failures has been implemented in #1661...but there is at least one other option; I outlined them in #1661 (comment). |
My apologies - I missed that comment in my overnight catchup. I guess we resolve/merge that one, then re-run CI here. Marking the PR accepted on the basis there's no code changes needed. |
- When Briefcase runs non-interactively, such as in CI, the Wait Bar is not rendered; therefore, it may not be clear which task is currently running. For instance, starting the Android emulator requires several Wait Bars but there is only output when the Wait Bar finishes. - This prints the Wait Bar's message when the Wait Bar starts if Briefcase is running non-interactively. - Additionally, this new Wait Bar start message as well as the outcome message is always included in the log file now. This is an example of non-interactive output: ``` [helloworld] Starting emulator beePhone... Starting emulator... started Starting emulator... done [helloworld] Starting app on @beephone (running emulator) (device ID emulator-5554) [helloworld] Installing app... Stopping old versions of the app... started Stopping old versions of the app... done Installing new app version... started Installing new app version... done Launching app... started Launching app... done [helloworld] Following device log output (type CTRL-C to stop log)... =========================================================================== ```
e857a20
to
7b4cf13
Compare
Changes
... still waiting
while a background runs that doesn't include any console output.This is an example of non-interactive output:
This is akin to what
pip
does when run non-interactively:PR Checklist: