New issue
Advanced search Search tips

Issue 701617 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Simple Chrome workflow should copy Chrome's D-Bus permission files

Project Member Reported by derat@chromium.org, Mar 15 2017

Issue description

The chromeos-chrome ebuild's src_install step currently installs D-Bus permissions for org.chromium.LibCrosService.conf:

    # Copy org.chromium.LibCrosService.conf, the D-Bus config file for the
    # D-Bus service exported by Chrome.
    insinto /etc/dbus-1/system.d
    DBUS="${CHROME_ROOT}"/src/chromeos/dbus/services
    doins "${DBUS}"/org.chromium.LibCrosService.conf

The Simple Chrome workflow doesn't do this, which is problematic because it means that developers trying to install ToT Chrome on older OS images might see Chrome not have permission to own newly-added D-Bus service names.

chromite's deploy_chrome script (which is used by both Simple Chrome and the ebuild) ought to install *.conf from this directory; then we can remove this from the ebuild (and moving stuff from the ebuild to deploy_chrome is a long-term goal, in any case).

Unfortunately, I don't see any support in deploy_chrome.py or chrome_util.py for installing source files (as opposed to files from the output dir). I'm also not sure that deploy_chrome supports installing to /etc/dbus-1/system.d rather than to /opt/google/chrome.

One approach might be:

a) change the Chrome gn build to copy src/chromeos/dbus/services/*.conf to the output dir
b) update chromite to copy these files to /opt/google/chrome/dbus
c) update /etc/dbus-1/system.conf to read /opt/google/chrome/dbus in addition to /etc/dbus-1/system.d
d) update the ebuild to not copy these files

I don't know if there's trickiness around synchronizing these changes.

Setting this to available in case it catches Justin's interest. :-)
 
Labels: Hotlist-CrOS-Gardener
There is a little trickiness, but it shouldn't be too bad.

Since the files are being copied from the chrome src tree, deploy_chrome should have no problem doing the copy.

The process would be:

1. Make the changes to deploy_chrome in chromite (chromeos/chromite).
2. Copy the changes locally to chrome/third_party/chromite/ and test with Simple Chrome.
3. Once the changes to deploy_chrome land, uprev chromite for chrome (in chrome/DEPS)
4. Sync chromeos, change the chromeos-chrome ebuild locally to remove the copy, and verify that deploy_chrome copies the files successfully.
5. Commit the changes to the ebuild.

It would be great to get this code out of the ebuild and into deploy_chrome!


Comment 2 by derat@chromium.org, Mar 15 2017

Is there already code in deploy_chrome for copying files from the source tree? I didn't see it in a quick pass through the files. I think it's currently geared toward copying files from the output dir.

I suggested adding a new /opt/google/chrome/dbus directory for these since I didn't see support for copying to dests outside of /opt/google/chrome and since we'll end up with stale files when things are removed (but since we mostly just use these config files to add permissions, that may not cause problems for developers).
We might not be copying from src. You may need (or want) to copy the files
to the out/ directory as part of the build step to avoid making assumptions
about the src path.
Moving from ebuild script to chromite python code generally sounds good.
Are we going to move all doins (and dosym/dodir) from chromeos-chrome ebuild to deploy_chrome?
Owner: teravest@chromium.org
Status: Started (was: Available)
I'm happy to take a look at this, it'll make it much easier to test changes for  crbug.com/692246  .

Comment 6 by derat@chromium.org, Mar 15 2017

Thanks, and let me know if you get stuck!

/etc/dbus-1/system.conf looks like it's installed by the sys-apps/dbus package (determined by running "equery-lumpy b /etc/dbus-1/system.conf" in my chroot), so you may need to apply a patch in that package's ebuild to add the additional path.

(I think that we should probably move Chrome's D-Bus config files to /opt/google/chrome since devs play various games with symlinking that directory to another partition to get more free space if they're installing an unstripped version of Chrome. It could get confusing if Simple Chrome additionally installs files to /etc/dbus-1.)
Cc: achuith@chromium.org
Re comment #4 - I would love to move as much as possible to deploy_chrome.

In particular, some of the files used for test should definitely be moved so that when we get VM Tests working in Simple Chrome they use the latest Chrome files and not whatever is on the chromeos image.

+achuith@

Comment 8 by derat@chromium.org, Mar 15 2017

Great! But let's use other bugs to track the other stuff to move. :-)
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/toolchain-utils/+/cdd7d45be9b171835027165ed0474bf8dbc04a9d

commit cdd7d45be9b171835027165ed0474bf8dbc04a9d
Author: Justin TerAvest <teravest@chromium.org>
Date: Fri Mar 17 12:46:46 2017

Remove reference to chromeos.gyp.

chromeos.gyp is used here to detect a path to a valid chrome browser
checkout. I'd like to get rid of chromeos.gyp, so this changes the logic
to use BUILD.gn, which was added to the source in June 2014.

BUG= chromium:701617 
TEST=None

Change-Id: I16f1705874bb8acafca0d3c38211f5e192e00a33
Reviewed-on: https://chromium-review.googlesource.com/455279
Commit-Ready: Justin TerAvest <teravest@chromium.org>
Tested-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Ting-Yuan Huang <laszio@chromium.org>

[modify] https://crrev.com/cdd7d45be9b171835027165ed0474bf8dbc04a9d/build_chrome_browser.py

Project Member

Comment 10 by bugdroid1@chromium.org, Mar 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/54ee225d47dd22fe7d2a2cb75bde7d27d790bf49

commit 54ee225d47dd22fe7d2a2cb75bde7d27d790bf49
Author: teravest <teravest@chromium.org>
Date: Fri Mar 17 14:18:16 2017

Remove chromeos/chromeos.gyp.

This migrates the variables in chromeos.gyp to BUILD.gn, making the
source easier to read. This also lets us remove a use of exec_script, so
chromeos/BUILD.gn no longer has to be whitelisted to use it.

BUG= 701617 

Review-Url: https://codereview.chromium.org/2753883002
Cr-Commit-Position: refs/heads/master@{#457759}

[modify] https://crrev.com/54ee225d47dd22fe7d2a2cb75bde7d27d790bf49/.gn
[modify] https://crrev.com/54ee225d47dd22fe7d2a2cb75bde7d27d790bf49/chromeos/BUILD.gn
[delete] https://crrev.com/3c10af5a884d33dc190ce36474db8e17db2ff51c/chromeos/chromeos.gyp

Project Member

Comment 11 by bugdroid1@chromium.org, Mar 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/822d58e6b22506cbd4787b58d8ccb421409b5b65

commit 822d58e6b22506cbd4787b58d8ccb421409b5b65
Author: teravest <teravest@chromium.org>
Date: Mon Mar 20 16:31:58 2017

Copy D-Bus service files to build output.

This is done so that chromite can copy these files from the build_output
to remove this step from an ebuild in ChromeOS. This also makes "Simple
Chrome" installs more closely match standard Chrome installs on
ChromeOS.

I've confirmed that dbus/org.chromium.LibCrosService.conf and
dbus/org.chromium.LivenessService.conf are created in the output
directory when performing a chrome build for a chromeos board.

BUG= 701617 

Review-Url: https://codereview.chromium.org/2764513002
Cr-Commit-Position: refs/heads/master@{#458083}

[modify] https://crrev.com/822d58e6b22506cbd4787b58d8ccb421409b5b65/chromeos/BUILD.gn
[add] https://crrev.com/822d58e6b22506cbd4787b58d8ccb421409b5b65/chromeos/dbus/services/README.md

Project Member

Comment 12 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/d407f485a58520d7b4259d18ef92786232166386

commit d407f485a58520d7b4259d18ef92786232166386
Author: Justin TerAvest <teravest@chromium.org>
Date: Tue Mar 21 02:30:15 2017

Copy D-Bus service files in deploy_chrome.

This allows D-Bus service file changes to be tested with deploy_chrome,
which lets us make changes without breaking the Simple Chrome workflow.

I've made this optional so that deploy_chrome will still succeed for
older Chrome builds that don't write these service files to the build
output.

BUG= chromium:701617 
TEST=deploy_chrome, files appeared in /opt/google/chrome/dbus

Change-Id: I2f0b9daded88e434006a9397227b1089a56f9c4a
Reviewed-on: https://chromium-review.googlesource.com/457260
Commit-Ready: Justin TerAvest <teravest@chromium.org>
Tested-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/d407f485a58520d7b4259d18ef92786232166386/lib/chrome_util.py

Project Member

Comment 13 by bugdroid1@chromium.org, Mar 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/7c4d0fb591d43e1596c47671e184d806ece90cb8

commit 7c4d0fb591d43e1596c47671e184d806ece90cb8
Author: Justin TerAvest <teravest@chromium.org>
Date: Wed Mar 29 18:42:17 2017

Add /opt/google/chrome/dbus to include dirs.

This allows D-Bus service files to be copied to the output of the Chrome
build and managed by deploy_chrome instead of requiring an update to an
ebuild file for every new D-Bus service file.

This new file must have a ".conf" extension to be included; D-Bus
ignored an attempted chromium.xml.

BUG= chromium:701617 
TEST=Installed package, service files in /opt/google/chrome/dbus work

Change-Id: I0ce8bac696be2a96e1d0ae10d2106bb6918fa47c
Reviewed-on: https://chromium-review.googlesource.com/461330
Commit-Ready: Justin TerAvest <teravest@chromium.org>
Tested-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[add] https://crrev.com/7c4d0fb591d43e1596c47671e184d806ece90cb8/chromeos-base/chromeos-chrome/files/chrome.conf
[modify] https://crrev.com/7c4d0fb591d43e1596c47671e184d806ece90cb8/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/ecb7ceadcc21d0758e9c44ab903c6ec9fb1af9c0

commit ecb7ceadcc21d0758e9c44ab903c6ec9fb1af9c0
Author: Justin TerAvest <teravest@chromium.org>
Date: Thu Mar 30 22:48:16 2017

Remove doins for LibCrosService.

org.chromium.LibCrosService.conf should now be available in
/opt/google/chrome/dbus, which is included by
/etc/dbus-1/system.d/chrome.conf.  This means that we no longer need
this explicit copy step in the ebuild.

BUG= chromium:701617 
TEST=None

Change-Id: I7d81e7d97f7d9033546bef33ff62e25edc835da9
Reviewed-on: https://chromium-review.googlesource.com/463586
Commit-Ready: Justin TerAvest <teravest@chromium.org>
Tested-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/ecb7ceadcc21d0758e9c44ab903c6ec9fb1af9c0/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/0c0d01f3b42c68535d400f0a8b44bd31d7148941

commit 0c0d01f3b42c68535d400f0a8b44bd31d7148941
Author: Dan Erat <derat@chromium.org>
Date: Mon Apr 03 21:39:50 2017

Revert "Remove doins for LibCrosService."

This reverts commit ecb7ceadcc21d0758e9c44ab903c6ec9fb1af9c0.

Reason for revert: It looks like the Chrome-provided version of this config file isn't being loaded, resulting in Chrome aborting at startup after failing to take ownership of the service name:  http://crbug.com/707317 

Original change's description:
> Remove doins for LibCrosService.
> 
> org.chromium.LibCrosService.conf should now be available in
> /opt/google/chrome/dbus, which is included by
> /etc/dbus-1/system.d/chrome.conf.  This means that we no longer need
> this explicit copy step in the ebuild.
> 
> BUG= chromium:701617 
> TEST=None
> 
> Change-Id: I7d81e7d97f7d9033546bef33ff62e25edc835da9
> Reviewed-on: https://chromium-review.googlesource.com/463586
> Commit-Ready: Justin TerAvest <teravest@chromium.org>
> Tested-by: Justin TerAvest <teravest@chromium.org>
> Reviewed-by: Dan Erat <derat@chromium.org>
> Reviewed-by: Mike Frysinger <vapier@chromium.org>
> 

TBR=derat@chromium.org,vapier@chromium.org,teravest@chromium.org
# Not skipping CQ checks because original CL landed > 1 day ago.
BUG= chromium:701617 

Change-Id: I69c02b2c42a211b35cef2d3d7dcdd35d7005b187
Reviewed-on: https://chromium-review.googlesource.com/466510
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Trybot-Ready: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/0c0d01f3b42c68535d400f0a8b44bd31d7148941/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild

Project Member

Comment 16 by bugdroid1@chromium.org, Apr 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f721f93701a353deb099f775cbe5131b2377aa3c

commit f721f93701a353deb099f775cbe5131b2377aa3c
Author: teravest <teravest@chromium.org>
Date: Tue Apr 04 18:19:36 2017

Roll DEPS for chromite.

This is needed to pick up changes to deploy_chrome
(https://chromium-review.googlesource.com/457260).

The chromeos-base/chromeos-chrome ebuild runs deploy_chrome from the
Chrome checkout, so we need to roll deps to use the correct version.

BUG= 701617 

Review-Url: https://codereview.chromium.org/2797703003
Cr-Commit-Position: refs/heads/master@{#461772}

[modify] https://crrev.com/f721f93701a353deb099f775cbe5131b2377aa3c/DEPS

Justin figured out that some of the brokenness devs have been seeing is due to them having correct D-Bus configs on their devices, but dbus-daemon not having been restarted to use them.

deploy_chrome should probably compare the on-device D-Bus config files in /opt/google/chrome/dbus against the ones that it's about to install. If they've changed, it should reboot the device. _DisableRootfsVerification already has code for rebooting.

There are mentions online about sending SIGHUP to dbus-daemon to trigger a "partial reload", but there are also mentions saying that you can't. It's unclear whether it'll do a full permissions reload or not, but that may be a simpler option if it works. All we really need for this is for it to allow new permissions, really.
Project Member

Comment 18 by bugdroid1@chromium.org, Apr 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/fac210ee0821219f5cf57501c9ee4bf6e45b54f0

commit fac210ee0821219f5cf57501c9ee4bf6e45b54f0
Author: Justin TerAvest <teravest@chromium.org>
Date: Fri Apr 14 06:54:07 2017

deploy_chrome: Reboot if D-Bus files changed.

D-Bus service files are read and configured at boot, and can be updated
when deploy_chrome is run. To prevent users from having to deal with
mysterious bugs, we reboot the system after copying files on any changes
to /opt/google/chrome/dbus.

BUG= chromium:701617 
TEST=Manually tested with and without modifying D-Bus files.

Change-Id: I2fd95f6cea4838c2335ef5e56830fb2466f02009
Reviewed-on: https://chromium-review.googlesource.com/477151
Commit-Ready: Justin TerAvest <teravest@chromium.org>
Tested-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/fac210ee0821219f5cf57501c9ee4bf6e45b54f0/scripts/deploy_chrome.py

Status: Fixed (was: Started)
chromite with the deploy_chrome change was rolled into Chrome DEPS today:
https://codereview.chromium.org/2820693003/


Comment 20 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 22 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment