New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 830940 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 29
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , Chrome
Pri: 2
Type: Bug

Blocked on:
issue 877152



Sign in to add a comment

libbrillo has forked. Expected?

Project Member Reported by gwendal@chromium.org, Apr 9 2018

Issue description

I noticed libbrillo has forked between Android and Chromeos around 7/16.

26 android changes are not in chromeos
39 chromeos changes are not in aosp.

Is it intentional? 

Trying to merge, few ASOP CLs have conflicts:

3b926ae: Use registered users in libbrillo/OWNERS. (migrate from @chromium to @google)

7a4643f: Convert most libbrillo targets to soong.
f85de06: Convert libbrillo-minijail target to Soong

also some conflicts in:
826000b libbrillo: Update libchrome APIS to r456626.
0cd09dc libbrillo: policy: Increase resilience for policy read

Gwendal.

 
tree_libbrillo.txt
7.9 KB View Download
Cc: -avakulenko@chromium.org -sosa@chromium.org ahass...@chromium.org benchan@chromium.org
libchrome and libbrillo need reconciliation. at this point, UE is the only project left in aosp that uses these libs (apmanager and shill are in process of moving back).

can we can get a summary of how much these libs are ingrained in UE? if we could move the libs back to Chromium, it'd make our lives easier.

Comment 2 Deleted

These are some of the things that UE uses in brillo:
bind_lambda.h
data_encoding.h
error_codes.h
message_loop.h -> this one might be tricky since it is very basic in UE. 
key_store_value.h -> Another important one.
string_utils.h
deamons.h
flag_helper.h
bind_watcher.h

for libchrome we use mainly use dbus.

We do syncing between UE in CrOS and AOSP a lot. We try to keep them in sync as much as possible since good features that land in one gets merge into another one and we would like to keep it this way unless there is opposition.

If we move back these libs to chromium, does that mean that it will be a permanent fork and we won't uprev to AOSP changes anymore?
Cc: senj@chromium.org
Adding UE owner in AOSP: senj@

Comment 5 by vapier@chromium.org, Jun 11 2018

we're just bouncing around ideas atm

if we move the main libbrillo/libchrome dev back to Chromium, the changes can be merged back to AOSP as needed on the UE side

Comment 6 by senj@chromium.org, Jun 11 2018

Cc: dpursell@chromium.org stevefung@chromium.org avakulenko@chromium.org
update_engine is not the only one using libbrillo/libchrome
http://cs/search/?q=lib(chrome%7Cbrillo)+f:android%5C.(bp%7Cmk)&sq=package:%5Eandroid$&type=cs
There's metricsd, peripheralmanager, bluetooth, nfc and some others using them.
So I don't think it's feasible to remove libchrome from aosp at this point, but I know that there aren't much development in libchrome/libbrillo going on from android side, so if it make you guys happier I'm fine with treating the repo in chromium as upstream.
Adding other android owners.
I remember, the decision to separate libchrome in aosp and chromeos has been made a couple years ago. At the time, it was a constant source of problems for anybody doing uprevving (need to make sure both Android and Chrome OS code is compiling correctly, at the same time and with the branch layout [aosp, vs internal googleplex repos] it became even a bigger problem of trying to keep automerger happy and working, since some AOSP changes are tried by TreeHugger on top of googleplex changes).

In the end, the Brillo team at the time decided to maintain AOSP copy and Chrome OS team dealt with its own version. AT the same time "shared" engines such as Update Engines have parted their way and were no longer single-sourced.

So, I'd say the fork is expected and has happened like two years ago.

Comment 8 by vapier@chromium.org, Jun 12 2018

i'm not talking about removing libs from AOSP.  they can stay there.  the question is how to handle the projects that are still being actively used in both CrOS & AOSP wrt these libs.

clearly there's a lot more development happening on the CrOS side with these projects while the AOSP side tends to be quiet.  it's unreasonable to have CrOS devs doing libchrome uprevs on the CrOS side to be responsible for all the AOSP projects.  if UE is the only thing we care about keeping in sync, we can probably just do what we had in the past ... when the code diverges, uses #if BASE_VER CPP logic to pick between the two.

libchrome today is at 395517 in AOSP and CrOS.  so when we move to the next version (e.g. 413419), when do it on the CrOS side w/out checking anything on the AOSP side.  but when Amin goes to do an uprev of AOSP UE into CrOS, he'll have to make sure it still works with the newer version of libchrome.  if it doesn't, he can make a change on the AOSP side like:
#if BASE_CPP <= 395517
  // Do older libchrome stuff that AOSP needs
#else
  // Do newer libchrome stuff that CrOS needs
#endif

then if/when AOSP ever merges libchrome/libbrillo changes back, the code in UE should still work, and get pruned.

UE & minijail are still single sourced.  everything else has gone back to platform2/ or is in process.
so what i'm thinking is:
- move libbrillo back into platform2/
  - create a gsubtree copy on GoB at chromiumos/platform2/libbrillo
- leave the AOSP copy alone (and thus nothing changes on the Android side)
  - if people want to merge changes, the gsubtree copy can be used
- all projects in CrOS will use the new libbrillo that we maintain
- any changes in libbrillo will run through the CrOS CQ like normal and makes sure everything continues to build/work
  - AOSP is a fork that does not get auto-updated
- if any changes to libbrillo are made that break UE, the person making that change will have to update UE to include some #if logic so it works with the new code on the CrOS side
  - this change will go through AOSP/treehugger to make sure Android doesn't break
- a manual uprev of UE happens on the CrOS side like it does today
- the CrOS libbrillo change lands via the CrOS CQ

this basically means libbrillo on the AOSP side won't change until someone on the Android side cares.  if the #if logic in UE gets too onerous, then someone on the UE team will have to take care of rolling things forward (as it cascades into all the other packages that Android has that might be using libbrillo still).

but this should allow the CrOS side to move forward w/out being beholden to the Android side, and Android doesn't have to worry about changes rolling in automatically that breaks the Android tree.

to keep things simple, we can keep the Android.bp and related files in the platform2/libbrillo/ source tree.  or if it makes no difference to Android, they can keep those files in the Android libbrillo fork and just do normal git merges.  i guess they're going to anyways right?
#9: Thanks for writing that up. No objections here.
Thanks :)
So regarding libbrillo, we are only moving it to platform2 because AOSP and CrOS where technically on different repos. right? That doesn't fundamentally change anything much. Did I get it wrong? 
LGTM with me although, upreving the AOSP version of libbrillo may not be an easy task as it might need cherry picking hundreds of CLs among platform2 repo (instead of a possible git merge).

How about libchrome?
#9: sgtm. Should the same logic apply to libchrome as well?
we want to move libbrillo back into platform2 because having a single repo for all our platform projects increases visibility and makes it easier to make changes together (especially with breaking changes).  the fact that it's hosted in AOSP makes this even more painful to update -- it's a sep CQ and the ebuild is manually uprevved.

by setting up a gsubtree of libbrillo, Chrome infra will create a git repo of just libbrillo CLs as if they were rooted at libbrillo/.  that should make it easier to cherry pick or even do a `git merge` directly.  you can see an example of this here:
  https://chromium.googlesource.com/chromium/src/base/
those are commits made to src.git but only to files under base/.
actually, the AOSP/blacklist note doesn't apply to libbrillo, so nm that.

wrt libchrome, i'm not sure we'd ever merge it into platform2 unrelated to AOSP.  i'd like to brainstrom that in a sep bug.
Sounds good to me, I think I just need to change aosp/upstream-master branch to replicate the new gsubtree repo, and merge should still work.
Cool, That explains it better. But just a question, I see that libbrillo has a 9999 ebuild and is not blacklisted, so why does it have a separate CQ? Changes in CrOS do not land in AOSP automatically and neither the otherwise. And AFAIK the changes to libbrillo goes through Chromium gerrit not Android gerrit. I'm just a bit confused.

SGTM overall.
Cc: lhchavez@chromium.org
+1 to #9.

One question. What should we do if we/UE team want to change libbrillo in AOSP?
Change in CrOS and then uprev in AOSP? Or, just making local change (so that libbrillo is (more) diverged)?

As for libchrome, filed a separate bug. crbug.com/873906
tl;dr; I'd propose to move libchrome to CrOS, too, and make AOSP's mirror.
Let's discuss it in that bug.

ignoring Android-specific files (e.g. Android.bp), ideally there should be no differences between any source files between Android and CrOS.  however, i think that'd be up to Android devs to make sure.

Comment 19 Deleted

i've rewritten & merged the history here:
  https://chromium.googlesource.com/chromiumos/platform2/+log/sandbox/vapier/libbrillo

i've opted to drop the Android-specific files on the floor:
 .clang-format                    |   1 
 Android.mk                       | 389 
 MODULE_LICENSE_BSD               |   0
 NOTICE                           |  27 
 testrunner_android.cc            |  26 

astute reviewers might notice that AOSP has backoff_entry files but CrOS does not.  this is because we deleted/renamed them in CrOS after AOSP had forked, but those changes weren't merged into AOSP.  nothing uses them there though afaik so i've opted to keep that drop in CrOS.  i expect AOSP will also drop them when they sync up with CrOS again.
  https://chromium-review.googlesource.com/293421

since AOSP will want to take a merge style to the repo anyways like it has in the past, it should be easy to add those files on the AOSP side.  this also allows AOSP to merge any changes they want independently of platform2 (although conflicts will obviously make it harder to pull things back into sync).

i'll add a README.md describing the process for libbrillo contributors and the relationship with AOSP.

i'm going to wait for Ben's CL to land, and once that does, i'll pull the trigger on this stuff and nerf all open libbrillo CL's on our side.  no other libbrillo CL looks like it'll be landing any time soon.
  https://chromium-review.googlesource.com/1181529
now that Ben's CL has landed, i've pulled that in, rebased onto the latest, and pushed it into platform2.  i made sure that the diff between platform2.git:a06bc4708453 and libbrillo.git:84a40fb6c08e was just the Android content i noted in comment #20.

ebuild/manifest CLs are in flight now.
Blockedon: 877152
Project Member

Comment 23 by bugdroid1@chromium.org, Aug 23

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

commit e825fcfb0bc08f40941bba1d281d3285c7a002f1
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Aug 23 20:47:38 2018

libbrillo: restore backoff_entry

While nothing in CrOS has used this, some Jetstream code has started to.
Restore the code until we can update those codebases.

BUG= chromium:830940 
TEST=build still passes

Change-Id: I3488f9293c38640577f75ad1c134dd494aafd0f3
Reviewed-on: https://chromium-review.googlesource.com/1187383
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>

[add] https://crrev.com/e825fcfb0bc08f40941bba1d281d3285c7a002f1/libbrillo/brillo/backoff_entry_unittest.cc
[modify] https://crrev.com/e825fcfb0bc08f40941bba1d281d3285c7a002f1/libbrillo/libbrillo.gypi
[add] https://crrev.com/e825fcfb0bc08f40941bba1d281d3285c7a002f1/libbrillo/brillo/backoff_entry.cc
[add] https://crrev.com/e825fcfb0bc08f40941bba1d281d3285c7a002f1/libbrillo/brillo/backoff_entry.h

Project Member

Comment 24 by bugdroid1@chromium.org, Aug 23

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

commit b3b2cd035802fb3346ce051b09a0810abdcbf048
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Aug 23 20:47:58 2018

libbrillo: build out of platform2

BUG= chromium:830940 
TEST=precq passes

Change-Id: I9af0cc629a6912fd8dd0188137bd8d0b815f7e00
Reviewed-on: https://chromium-review.googlesource.com/1173694
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/b3b2cd035802fb3346ce051b09a0810abdcbf048/chromeos-base/libbrillo/libbrillo-9999.ebuild

Project Member

Comment 25 by bugdroid1@chromium.org, Aug 29

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/manifest-internal/+/8a3465b6049b83211712229930892f9b416b12a7

commit 8a3465b6049b83211712229930892f9b416b12a7
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Aug 29 18:15:45 2018

Status: Fixed (was: Started)
i've got one more CL in the CQ (update public manifest), so just going to close this out now
Project Member

Comment 27 by bugdroid1@chromium.org, Aug 29

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

commit c61929d8263fa9f07fe2f13272a603e04d5d79f8
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Aug 29 23:09:10 2018

libbrillo: move back to platform2

BUG= chromium:830940 
TEST=precq passes

Change-Id: I15ef9697c324fa69cf89ca2cfc92ff557cc1fef3
Reviewed-on: https://chromium-review.googlesource.com/1183997
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>

[modify] https://crrev.com/c61929d8263fa9f07fe2f13272a603e04d5d79f8/full.xml

Sign in to add a comment