New issue
Advanced search Search tips

Issue 853560 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Feature

Blocked on:
issue 870529



Sign in to add a comment

Garcon support for x-terminal-emulator

Project Member Reported by smbar...@chromium.org, Jun 17 2018

Issue description

Garcon 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).
 
The problem with the x-terminal-emulator alternative is there is zero standardization of command-line flags between terminal emulators - so whilst you can expect `some-browser http://google.com` to work, there's no standard way to say `some-terminal foo.sh` or `some-terminal -c ./foo`
We could just ignore any flags passed to the terminal....that's probably better than nothing.

Comment 3 by vapier@chromium.org, 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.
Cc: vapier@chromium.org
Status: Assigned (was: Untriaged)
Sounds reasonable.
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.
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.

Comment 7 by vapier@chromium.org, 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.
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Status: Started (was: Assigned)
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Blockedon: 870529
We can't actually set up x-terminal-emulator until  issue 870529  is resolved.
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.
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment