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

Print message when Wait Bar is started non-interactively #1649

Merged
merged 4 commits into from Feb 23, 2024

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Feb 14, 2024

Changes

  • 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.
  • This new Wait Bar start message as well as the outcome message is always included in the log file now.
  • Additionally, a new keep-alive spinner is available as a return from the Wait Bar.
    • This spinner was be used to periodically print ... still waiting while a background runs that doesn't include any console output.
    • The is currently implemented for the start up process of the Android emulator.

This is an example of non-interactive output:

[helloworld] Starting emulator beePhone...
Starting emulator... started
Starting emulator... done

[helloworld] Starting app on https://github.com/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)...
===========================================================================

This is akin to what pip does when run non-interactively:

❯ python -m pip install pygobject --no-cache | cat
Collecting pygobject
  Downloading PyGObject-3.46.0.tar.gz (723 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 723.4/723.4 kB 3.7 MB/s eta 0:00:00
  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
  Getting requirements to build wheel: started
  Getting requirements to build wheel: finished with status 'done'
  Preparing metadata (pyproject.toml): started
  Preparing metadata (pyproject.toml): finished with status 'done'
Collecting pycairo>=1.16.0 (from pygobject)
  Downloading pycairo-1.26.0.tar.gz (346 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 346.9/346.9 kB 39.3 MB/s eta 0:00:00
  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
  Getting requirements to build wheel: started
  Getting requirements to build wheel: finished with status 'done'
  Installing backend dependencies: started
  Installing backend dependencies: finished with status 'done'
  Preparing metadata (pyproject.toml): started
  Preparing metadata (pyproject.toml): finished with status 'done'
Building wheels for collected packages: pygobject, pycairo
  Building wheel for pygobject (pyproject.toml): started
  Building wheel for pygobject (pyproject.toml): finished with status 'done'
  Created wheel for pygobject: filename=PyGObject-3.46.0-cp310-cp310-linux_x86_64.whl size=780934 sha256=adaf291713960cd7c5020defbb5294742c95a2cc03240157053fd801b60eefc3
  Stored in directory: /tmp/pip-ephem-wheel-cache-8d96807b/wheels/2b/d7/f7/1f4c5631b6d31c5a0f377754294700bf3d7df1ceee00617404
  Building wheel for pycairo (pyproject.toml): started
  Building wheel for pycairo (pyproject.toml): finished with status 'done'
  Created wheel for pycairo: filename=pycairo-1.26.0-cp310-cp310-linux_x86_64.whl size=320989 sha256=07a26626db22ee48e253c5669be0d2227091599db2abe5f695391a697033e90d
  Stored in directory: /tmp/pip-ephem-wheel-cache-8d96807b/wheels/e3/46/83/453eb7915b034ce1a9fee5a6023def2030633f6a73dc6d2de8
Successfully built pygobject pycairo
Installing collected packages: pycairo, pygobject
Successfully installed pycairo-1.26.0 pygobject-3.46.0

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16
Copy link
Member Author

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 started or something.

For instance:

[helloworld] Starting emulator beePhone...
Starting emulator... started
Starting emulator... done

@freakboy3742
Copy link
Member

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 starting message might be a good addition (although I think I'd rather not see that in interactive mode); another would be to replace the "throbber" with a periodic (say, every 10 seconds) "... still working" message on a line by its own.

@rmartin16
Copy link
Member Author

another would be to replace the "throbber" with a periodic (say, every 10 seconds) "... still working" message on a line by its own

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 pip accomplishes this with a context manager....but that requires a lot of cooperation with the callers.

@rmartin16 rmartin16 force-pushed the non-interactive-wait-bar branch 2 times, most recently from 2fe3b4b to f81e0af Compare February 15, 2024 22:07
@rmartin16
Copy link
Member Author

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 FileProxy for stdout; so, I updated the check to use os.__stdout__ instead.

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 aborted for KeyboardInterrupt and errored otherwise. I think this is an improvement over the Wait Bar message just becoming a static message with any sort of status.

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..?

@rmartin16
Copy link
Member Author

rmartin16 commented Feb 17, 2024

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 ... still waiting message. I've only implemented it for the start up of the Android emulator.

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 ... still waiting is the right message. I didn't parameterize these since it kinda felt like YAGNI.

[edit] i was a wee bit whimsical in the naming....we can revise if too much

@rmartin16 rmartin16 force-pushed the non-interactive-wait-bar branch 2 times, most recently from 7397481 to 25a51a2 Compare February 18, 2024 18:40
@rmartin16
Copy link
Member Author

As a note, unlike the new Wait Bar's "started" message, the new ... still waiting message prints even in an interactive session where the Wait Bar is being rendered.

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 Popen() instead of run().

@rmartin16 rmartin16 marked this pull request as ready for review February 18, 2024 19:01
Copy link
Member

@freakboy3742 freakboy3742 left a 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.

@rmartin16
Copy link
Member Author

I've been looking at the iOS simulator startup:

  1. call to boot simulator
  2. open window for simulator
  3. uninstall app
  4. install app
  5. launch app

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 xcrun simctl bootstatus <UUID>....but this command is semi-undocumented. It isn't in the list of commands for xcrun simctl....but it has help text via xcrun simctl help bootstatus. I've also found an SO answer from 2019 mentioning it....so, its been around for a minute.

Otherwise, maybe there's innocuous command that'll block like the uninstall command until the simulator is ready.

@rmartin16
Copy link
Member Author

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 Booting... and if the uninstall fails, we can error as such. Otherwise, the Wait Bar moves on to Installing app....

@rmartin16 rmartin16 force-pushed the non-interactive-wait-bar branch 4 times, most recently from 3d48758 to 6eb9de8 Compare February 22, 2024 18:17
@rmartin16
Copy link
Member Author

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

Copy link
Member

@freakboy3742 freakboy3742 left a 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.

uninstall_popen = self.tools.subprocess.Popen(
["xcrun", "simctl", "uninstall", udid, app.bundle_identifier]
)
with uninstall_popen:
Copy link
Member

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:

Copy link
Member Author

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

Copy link
Member Author

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.

["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(
Copy link
Member

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

Copy link
Member

@freakboy3742 freakboy3742 left a 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

@rmartin16
Copy link
Member Author

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).

@freakboy3742
Copy link
Member

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)...
===========================================================================
```
@freakboy3742 freakboy3742 merged commit e141754 into beeware:main Feb 23, 2024
51 checks passed
@rmartin16 rmartin16 deleted the non-interactive-wait-bar branch February 23, 2024 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants