Garcon support for x-terminal-emulator |
||||
Issue descriptionGarcon should provide an interface to launch the native Crostini terminal on the host. As part of this, we should add an alternative (similar to x-www-browser).
,
Jun 22 2018
We could just ignore any flags passed to the terminal....that's probably better than nothing.
,
Jun 22 2018
if there are options, then abort. otherwise, execv the commandline. if people want -c behavior, they can invoke /bin/sh -c themselves. that should be a reasonable start w/out backing into a corner.
,
Jun 22 2018
Sounds reasonable.
,
Jun 27 2018
Mike...when you say to execv the command line...do you mean that we shouldn't launch a new terminal in that case, and just execute the command line instead? So I'm thinking a script to do this would work fine then, let me know if this isn't aligned. The script would just do: 1. If there are options passed, abort with an error message 2. If there are other args (non-options), then just exec that as one command 3. If there is no options/args, then we send a message over to Chrome via cicerone to instruct it to launch another crosh terminal (and rate limit this like we do for openurl calls); this would just invoke garcon with a flag to make this happen, just like we do for openurl.
,
Jun 28 2018
For me as a consumer, the _absolute_ bare minimum is "run this command", much like xterm's -e flag (e.g. you can synthesize the "working directory" flag most terminal emulators offer via "-e /bin/sh -c 'cd foo; ./bar'") A -title flag or similar is a definite nice to have, so you can differentiate easily between multiple terminal windows.
,
Jun 28 2018
we should launch a new terminal, and in that terminal exec the specified command instead of spawning a shell and running it through that i specifically do not think we should emulate xterm's -e behavior. that's a bit more complicated: - it tries to execvp() the argv directly - if the execvp fails, falls back to running through a shell with -c (where the shell is obtained via $SHELL or falls back to the user's registered login shell) and the first argv from the -e this leads to confusing behavior for people. if you specify a program that isn't currently installed, xterm will spawn a shell. if your login shell isn't POSIX compatible (like fish), then the string might not work if your login shell was POSIX compatible (like dash or bash). imo this is why it should be explicit at least for now: if you want to run a shell string, you need to run /bin/sh -c "<whatever you want>" yourself. lets hold off on adding support for any options for now. i agree that there might be interesting things here (like setting the title), but not just yet so we can focus on something simple/sane.
,
Jul 14
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/system_api/+/6e71a28cf4a6fc980b0cb3ce55a82204ed20d89a commit 6e71a28cf4a6fc980b0cb3ce55a82204ed20d89a Author: Jeffrey Kardatzke <jkardatzke@google.com> Date: Sat Jul 14 00:28:57 2018 Add D-Bus method and proto for launching a Crostini Terminal This will be used by the container to tell Chrome to launch another terminal. This will then be linked to an override of the x-terminal-emulator alternative in the container. We support extra parameters which will then be passed to the terminal for execution inside of the terminal. BUG= chromium:853560 TEST=Builds Change-Id: I810fc882960c7837f6fd428b2fdfec5a76378ba1 Reviewed-on: https://chromium-review.googlesource.com/1135900 Commit-Ready: Jeffrey Kardatzke <jkardatzke@google.com> Tested-by: Jeffrey Kardatzke <jkardatzke@google.com> Reviewed-by: Nicholas Verne <nverne@chromium.org> Reviewed-by: Stephen Barber <smbarber@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/6e71a28cf4a6fc980b0cb3ce55a82204ed20d89a/dbus/vm_applications/apps.proto [modify] https://crrev.com/6e71a28cf4a6fc980b0cb3ce55a82204ed20d89a/dbus/vm_applications/dbus-constants.h
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/system_api/+/2e61850939976516c75622dc05134bac49cd24e3 commit 2e61850939976516c75622dc05134bac49cd24e3 Author: Jeffrey Kardatzke <jkardatzke@google.com> Date: Wed Jul 25 23:35:10 2018 Add owner/vm/container fields for TerminalParams proto BUG= chromium:853560 TEST=Builds Change-Id: Ifb04f25fe9f63f3293e1b30dd00afc2da3e20001 Reviewed-on: https://chromium-review.googlesource.com/1149183 Commit-Ready: Jeffrey Kardatzke <jkardatzke@google.com> Tested-by: Jeffrey Kardatzke <jkardatzke@google.com> Reviewed-by: Nicholas Verne <nverne@chromium.org> Reviewed-by: Stephen Barber <smbarber@chromium.org> [modify] https://crrev.com/2e61850939976516c75622dc05134bac49cd24e3/dbus/vm_applications/apps.proto
,
Jul 26
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8fa347f197f1e0246d128fc15b1cc3598c326bfa commit 8fa347f197f1e0246d128fc15b1cc3598c326bfa Author: Jeffrey Kardatzke <jkardatzke@google.com> Date: Fri Jul 27 02:40:44 2018 Add D-Bus call for launching terminal and support params This adds support for sending a D-Bus message to Chrome so it launches a terminal connected to a Crostini container. Arguments to be executed in that terminal can also be passed as well. Bug: chromium:853560 Test: Verified with manual testing Change-Id: Ib0126e6fba94e210d9376d57c0f6edb85e667484 Reviewed-on: https://chromium-review.googlesource.com/1149188 Commit-Queue: Jeffrey Kardatzke <jkardatzke@google.com> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Nicholas Verne <nverne@chromium.org> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#578524} [modify] https://crrev.com/8fa347f197f1e0246d128fc15b1cc3598c326bfa/chrome/browser/chromeos/crostini/crostini_manager.cc [modify] https://crrev.com/8fa347f197f1e0246d128fc15b1cc3598c326bfa/chrome/browser/chromeos/crostini/crostini_manager.h [modify] https://crrev.com/8fa347f197f1e0246d128fc15b1cc3598c326bfa/chrome/browser/chromeos/crostini/crostini_util.cc [modify] https://crrev.com/8fa347f197f1e0246d128fc15b1cc3598c326bfa/chrome/browser/chromeos/dbus/vm_applications_service_provider_delegate.cc [modify] https://crrev.com/8fa347f197f1e0246d128fc15b1cc3598c326bfa/chrome/browser/chromeos/dbus/vm_applications_service_provider_delegate.h [modify] https://crrev.com/8fa347f197f1e0246d128fc15b1cc3598c326bfa/chrome/browser/ui/views/crostini/crostini_installer_view.cc [modify] https://crrev.com/8fa347f197f1e0246d128fc15b1cc3598c326bfa/chromeos/dbus/services/vm_applications_service_provider.cc [modify] https://crrev.com/8fa347f197f1e0246d128fc15b1cc3598c326bfa/chromeos/dbus/services/vm_applications_service_provider.h
,
Aug 3
,
Aug 6
Yeah, I've got commits sitting in my local repo for our deb packages for that as well as for the config file for disabling automatic updates.
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/886b547a031159f9cf6f7c8246cf004ff3b47a3d commit 886b547a031159f9cf6f7c8246cf004ff3b47a3d Author: Jeffrey Kardatzke <jkardatzke@google.com> Date: Wed Aug 08 04:45:00 2018 vm_tools: garcon/cicerone: Add opening a terminal from container This adds the ability to execute garcon so that it sends a message to cicerone and then to Chrome to open a new terminal window. Additional parameters can be passed which will then be executed in the terminal. BUG= chromium:853560 TEST=tast run eve vm.LaunchTerminal Change-Id: I4baf06f015ec8085d7637d247d57e80547946173 Reviewed-on: https://chromium-review.googlesource.com/1149146 Commit-Ready: Jeffrey Kardatzke <jkardatzke@google.com> Tested-by: Jeffrey Kardatzke <jkardatzke@google.com> Reviewed-by: Stephen Barber <smbarber@chromium.org> [modify] https://crrev.com/886b547a031159f9cf6f7c8246cf004ff3b47a3d/vm_tools/cicerone/container_listener_impl.cc [modify] https://crrev.com/886b547a031159f9cf6f7c8246cf004ff3b47a3d/vm_tools/proto/container_host.proto [modify] https://crrev.com/886b547a031159f9cf6f7c8246cf004ff3b47a3d/vm_tools/garcon/host_notifier.h [modify] https://crrev.com/886b547a031159f9cf6f7c8246cf004ff3b47a3d/vm_tools/garcon/main.cc [modify] https://crrev.com/886b547a031159f9cf6f7c8246cf004ff3b47a3d/vm_tools/cicerone/service.cc [modify] https://crrev.com/886b547a031159f9cf6f7c8246cf004ff3b47a3d/vm_tools/cicerone/service.h [modify] https://crrev.com/886b547a031159f9cf6f7c8246cf004ff3b47a3d/vm_tools/cicerone/container_listener_impl.h [modify] https://crrev.com/886b547a031159f9cf6f7c8246cf004ff3b47a3d/vm_tools/garcon/host_notifier.cc
,
Aug 31
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/containers/cros-container-guest-tools/+/4919e78c3bbdf2a9e5ffec2d6ad2114bccb721dc commit 4919e78c3bbdf2a9e5ffec2d6ad2114bccb721dc Author: Jeffrey Kardatzke <jkardatzke@google.com> Date: Fri Aug 31 02:33:59 2018 cros-garcon: Add alternative for x-terminal-emulator BUG= chromium:853560 TEST=Verified .deb file installs proper files Change-Id: I22bf2673e0de3d4aaaa4bc2b6b62d81855131ee0 Reviewed-on: https://chromium-review.googlesource.com/1191865 Commit-Ready: Jeffrey Kardatzke <jkardatzke@google.com> Tested-by: kokoro <noreply+kokoro@google.com> Tested-by: Jeffrey Kardatzke <jkardatzke@google.com> Reviewed-by: Stephen Barber <smbarber@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> [modify] https://crrev.com/4919e78c3bbdf2a9e5ffec2d6ad2114bccb721dc/cros-garcon/prerm [modify] https://crrev.com/4919e78c3bbdf2a9e5ffec2d6ad2114bccb721dc/cros-garcon/garcon-url-handler [modify] https://crrev.com/4919e78c3bbdf2a9e5ffec2d6ad2114bccb721dc/cros-garcon/BUILD [add] https://crrev.com/4919e78c3bbdf2a9e5ffec2d6ad2114bccb721dc/cros-garcon/garcon-terminal-handler [modify] https://crrev.com/4919e78c3bbdf2a9e5ffec2d6ad2114bccb721dc/cros-garcon/postinst
,
Aug 31
|
||||
►
Sign in to add a comment |
||||
Comment 1 by hexxing...@googlemail.com
, Jun 18 2018