Allow web server device to use extension if started with --start-paused #44263
Conversation
DanTup
added
the
tool
fluttergithubbot
added
the
c: contributor-productivity
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| @@ -1115,6 +1115,7 @@ class _AppRunLogger extends Logger { | |||
| final int id = _nextProgressId++; | |||
| _sendProgressEvent(<String, dynamic>{ | |||
| 'id': id.toString(), | |||
| 'message': message, | |||
There was a problem hiding this comment.
@jonahwilliams It feels a bit weird having this here, since this is an app.progress notification - however it seems like this method doesn't otherwise work as intended (the message gets dropped, and the only code calling this seems to exist specifically to pass the url back). It was added in #40627.
The daemon spec says message will not be included for a finished event, but this change will make that not true.
Should we change this to something else (eg. not app.progress) that may allow passing arbitrary data back to the daemon client (I think we're going to need more than this one URL - for ex. both the web server URL and the debug service URL - I'lL CC you on another issue about this shortly)?
There was a problem hiding this comment.
Actually, I'm going to take this back out for now - I think I have a better idea for how this can work that might solve the other issues I was talking about. I'll put a PoC together and if it works we can discuss it.
There was a problem hiding this comment.
I think this is a more sensible fix for this -> #44268
packages/flutter_tools/lib/src/build_runner/resident_web_runner.dart
Outdated
Show resolved
Hide resolved
|
For testing, you might just verify that a heavily mocked resident runner prints the right messages and calls the right API on dwds |
|
@jonahwilliams I've removed the launchBrowser switch and related code, and also added tests. Please check the tests seem to cover what you think they need to, since I'm not all that familiar with this code. Thanks! 👍 |
| dartDefines: dartDefines, | ||
| ); | ||
| // When connecting to a browser, update the message with a seemsSlow notification | ||
| // to handle the case where we fail to connect. | ||
| buildStatus.stop(); | ||
| statusActive = false; | ||
| if (debuggingOptions.browserLaunch && supportsServiceProtocol) { | ||
| if (supportsServiceProtocol) { |
There was a problem hiding this comment.
This condition should be the same as skipDwds, since we can only exit from here if dwds is started.
There was a problem hiding this comment.
supportsServiceProtocol is defined as isRunningDebug && deviceIsDebuggable, so I think these are the same? Would it be better to change skipDwds to use !supportsServiceProtocol to make it clearer?
There was a problem hiding this comment.
Yeah - I think you could either make them the same or pull it into a single getter _enableDwds so they don't accidentally get out of sync
There was a problem hiding this comment.
Added a new getter, but also made its implementation use debuggingEnabled since it was the same condition and seems like they should match.
| @@ -741,7 +743,7 @@ class _DwdsResidentWebRunner extends ResidentWebRunner { | |||
| } | |||
| } | |||
| // Allows browser refresh hot restart on non-debug builds. | |||
| if (device is ChromeDevice && debuggingOptions.browserLaunch) { | |||
| if (device is ChromeDevice) { | |||
There was a problem hiding this comment.
This should only be hit if we're running release/profile on chrome
There was a problem hiding this comment.
Is device is ChromeDevice && !isRunningDebug correct? Should it also be device.device is ChromeDevice now?
|
@jonahwilliams pushed some tweaks, though not sure why Cirrus doesn't seem to be running (cc @fkorotkov) |
|
@DanTup according to logs GitHub API returned 404 when Cirrus tried to load |
|
@fkorotkov looks good this time 🙂🤷♂️ I don't know if it happens enough to bother working around, but if it was only a 404 temporarily, maybe it could be retried a few times over a few seconds to catch this. |
|
This is the first known report of such issue. If there will be more then I will definitely contact GitHub and will workaround in the meantime. 404 is a legit response since if you have enabled Cirrus on all repositories but inly have it configured on a few then the webhook events are still coming to Cirrus from these repositories that don't have configs. |
|
There were some conflicts after landing #44268 (it changed the notification to a real event, and this PR had moved the original notification around) so I've rebased it. I reviewed the diff before/after and they're the same except for the notification (I thought GH would make that easy, but the comparison includes every other changeset that was came in with the rebase). (builds currently broken because I rebased over a broken master - I'll re-rebase once the build is fixed 😄) |
|
@fkorotkov I'm a bit confused about the test results. On GitHub here it shows that the build failed (tool_tests-commands-linux) but when I look on Cirrus it looks like it failed OOM, then was automatically re-run, and the re-run passed (so the overall status looks green): https://cirrus-ci.com/build/5823148376719360 Should the status here have been updated to green? |
|
It seems two reruns happend when only one should've. And the last one failed again which led to overriding success status of the check from the first successful re-run. I'll investigate why two re-runs happened and will make sure to not flip the status check once it set to success. Sorry for the confusion! |
|
@fkorotkov am I reading it right that both of them failed with OOMKilled? I'm trying to figure out if I broke it. If two of them went OOM and other PRs aren't, it doesn't look good for my change 🤔 |
|
As far as I can see the initial task Task |
That's what concerned me - if 2/3 runs failed OOM, I think something might be wrong. I did force it again, but now Windows failed inexplicably (looks like it didn't run any tests) 😔 I think think |
Co-Authored-By: Jonah Williams <jonahwilliams@google.com>
…ing Chrome The web-server device should now be used if you don't want to launch a browser.
|
Can't reproduce any issues locally, nor see why these changes should cause OOM (or a hang on Windows). Rebased on latest master again and will see what happens this time 🤷♂️ |
|
The OOM issues are likely due to the shards running with less memory in an attempt to improve scheduling. I've opened a PR to bump that one from 8GB to 10GB at #45178. |
|
@DanTup I now spent my whole Saturday trying to get this working over ssh port forwarding. I can use start-paused from terminal and attach from chrome but when I try to use a chrome instance running in a machine that is not where flutter app is running, it does not work. there is definitely the issue of a random debugging port which flutter does not always print to command line as in So I'd be eternally grateful if you could comment on the feasibility of using ssh port mapping, or point me to an example etc. |
|
@serefarikan are you using the VS Code Remoting - SSH extension? The supported way to do this is use VS Code Remoting to run the extension host on the remote SSH machine. Then you just use the normal "Start Debugging" command in VS Code to run on the web-server device and everything should work (it will spawn a local browser instance for you that will have been tunnelled by the VS Code Remote - SSH extension). If you're seeing random disconnects when doing this, please file an issue at https://github.com/Dart-Code/Dart-Code with enough info for me to try and repro the same thing. Thanks! |
|
@DanTup thanks. I'm actually using Intellij Idea, not Vs Code. I don't know how vs code deals with the problem I'm having but I overcame it by using dynamic proxy via ssh. The dynamic proxy ssh connection is complaining about some missing port mappings but things are working as they should. So I start a dynamic proxy over ssh, then start an instance of chrome using that as socks proxy and everything works. My point re some port mapping being required is rather generic. I'm still thinking there is a port (used for the debug service as the printed out console log suggests) which as chosen randomly and the only way to deal with it is use a dynamic proxy which handles all ports. I suspect Vs Code has a way of setting that port to a particular value, but frankly I'm reluctant to dive into source code of the plugin to figure it out :) Thanks for getting back to me on this one. |
|
@serefarikan VS Code runs Flutter using a flag I don't know if it's possible to specify each of the ports manually that would be needed (I think there are a few - the one the Flutter server listens on, the one for the DWDS backend, the one for the injected client backend). We don't try to do that from VS code because using static ports makes it harder to run multiple instances and often the hostnames also need mapping (this functionality is designed to support cloud-based IDEs in addition to SSH/Containers). Unfortunately I'm not sure if there's an easy way to do what you need if not using VS Code. |
|
@DanTup that's fascinating, thanks a lot for explaining the mechanics of it. My development setup is admittedly unusual :) I think I may simply give vs code a try and save myself some trouble. I could not possibly find enough time to figure out what you described above, so thanks a lot for that, once again. |
|
No worries. Sorry it wasn't as simple as you hoped, but supporting cloude IDEs made it a little more than just tunnelling ports (for a cloud IDE, localhost:1234 will often map to 1234-some-hostname without a port). If you do try VS Code using the remoting extensions and it doesn't "just work" then do give me a shout at https://github.com/Dart-Code/Dart-Code and we can debug there. |
|
It's never as simple as we hope it'd be, is it? :) Thanks for the hint, will certainly keep it in mind. |
@jonahwilliams This enables using the Dart debug extension using the web-server device when
--start-pausedis used. This is to support running in remote environments where there's no Chrome installed (and the Chrome device is not available) but you still want to be able to debug.Tests
@jonahwilliams do we have any ability to test with the Debug extension? I looked but couldn't find anything. I tested manually from the terminal by:
flutter run -d web-server --start-pausedrin the terminal to confirm hot reload worked (counter reset - since it's a restart right now)I also tested from VS Code with a fork of DWDS that does not spawn the DevTools and confirmed I could launch the app and hot-reload-on-save worked:
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?