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

Issue 785137 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 10
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-09-07
OS: Chrome
Pri: 3
Type: Task

Blocked on:
issue 867171
issue 885315

Blocking:
issue 690513



Sign in to add a comment

Move shill from aosp back to chromiumos platform2

Project Member Reported by benchan@chromium.org, Nov 15 2017

Issue description

shill is no longer used in any aosp or brillo context, we should consider moving it back to platform2
 
Dupe or sub-issue of issue #690513
Blocking: 690513
Cc: -cernekee@chromium.org briannorris@chromium.org
Owner: vapier@chromium.org
Status: Started (was: Available)
i've rewritten & posted the branch for review here:
  https://chromium.googlesource.com/chromiumos/platform2/+/sandbox/vapier/shill

i didn't do as much editing on this series as others because of the sheer volume ... over 850 CLs.  what i did edit out of the history though was:
- drop Android specific files (e.g. NOTICE and shill.rc)
- drop gyp changes that changed to ../../platform2/common-mk/ paths from ../common-mk/
- add "shill:" prefix to all commit messages that didn't already have it

that means the code base is still licensed as AOSP.  editing that out was more effort than worth i think.
Blockedon: 867171
Cc: benchan@chromium.org
Brian said he thought it looked sane, so once the current shill fire settles down, i'll push through the migration
NextAction: 2018-09-07
Ben asked i wait until after M70 branches, so i'm going to aim for the afternoon of Sep 7th (Fri).  that'll give me the weekend to chump fixes in case things burn ;).
I'm curious: is it better to do this just before or after a branch? Just before: means that if you have to pick fixes (e.g., for other features) back to M70, you don't have to pick them to different repositories. Just after: means you have the highest chance of having to cherry-pick between repositories.

Or maybe there's other factors I'm not considering. e.g., maybe there's a higher chance of accidentally missing something in the transition, and we'd rather not have to fix that up on 2 branches?
IMHO, what's more important is that we need some bake time before beta promotion, so it seems less ideal to do the transition right before the branch point, which will give us only 2 weeks to sort things out. Between fighting for beta blockers + reverting CLs (worst case) in a branch and cherry-picking/backporting CLs to a branch, the latter seems more tolerable.
Another factor is that we want the sandboxing by itself in R70, then do the path move. If stuff breaks with sandboxing, at least we are fixing on familiar paths. 

M70 branch is supposed to be tomorrow (Aug 30) which is why i was picking a week later (Sep 7).  that'll get the "quick" cherry picks out of the way, but you're right that after we move you'll have to do a bit more effort to backport any changes.

that said, i don't think it's too big a deal.  after we move the code, backporting looks like:
- land CL in platform2 repo
- use `git format-patch -1` on the commit to create a patch
- apply the patch by hand to the standalone shill repo using `git am -p1`
- upload the CL to shill repo for that branch

if that's the only downside, imo having longer bake time in M71 is more important than being able to smoothly backport things to M70/M69.

i strongly don't think we should fork just before branching because of possible unforeseen problems/breakage.  some of the platform2 migrations have gone smoothly (no one noticed and CLs went through the CQ like normal), but others (like libbrillo) caused immediate CQ breakage at build time and required some chumping :/.  the TPMs already have enough to deal with to trying to get a new branch stable to not also have to deal with this.
SGTM, thanks
Cc: mortonm@chromium.org
i'm still planning on doing this migration this coming weekend
thanks for the heads up. ill need to cherry pick these https://chromium-review.googlesource.com/c/aosp/platform/system/connectivity/shill/+/1176166 https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1176106 by hand back into M70 but that shouldn't be too bad per c#10
^that is if I can't get the CLs into ToT and cherry picked before then
The NextAction date has arrived: 2018-09-07
Cc: adityakali@google.com caiz@chromium.org harpreet@chromium.org adityakali@chromium.org
+Harpreet FYI/for testing. *In theory*, this shouldn't cause breakage. In practice, I don't know what is an appropriate test other than "all builders should be green". 

Adding some folks as a heads-up to sister teams: adityakali@ for lakitu, caiz@ for Jetstream. 

Cc: wonderfly@google.com
Thanks for looping us in.
I don't think we use shill anymore though.
Yeah, we've moved away from shill long ago, and `equery-lakitu d chromeos-base/shill` doesn't return anything.
i've pushed this out now.  over 800 CLs!

i'll watch the tree, and if it doesn't burn, run the CLs to update the ebuild/manifests through the CQ.
For the naive and uninitiated among us, could you document here the steps you've taken for the migration? 

1. https://chromium.googlesource.com/chromiumos/platform2/+/sandbox/vapier/shill
Did you do a `git push cros ${VAPIER_BRANCH}:refs/for/master` on this branch? 
FWIW, this branch is no longer accessible for me (404). 

2. https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1147713
https://chromium-review.googlesource.com/c/chromiumos/manifest/+/1149101

I only see these 2 CLs with the search "owner:vapier@chromium.org status:open"
that look relevant. 
Are those the only CLs? 
Are there any steps between 1 and 2? 


> For the naive and uninitiated among us, could you document here the steps you've taken for the migration? 

it was pretty all ad-hoc
(1) rewrite aosp git history
git filter-branch -f --prune-empty --tree-filter 'mkdir -p shill; git ls-tree --name-only ${GIT_COMMIT} | xargs -I %files% mv %files% shill/'
(2) flatten aosp git history with git-rebase until all the merge commits are gone
(3) find the commit that deleted shill in platform2 and revert it (see db66d0fab31649e3d24797648dbc4c0a99494726)
(4) manually stitch/reorder aosp shill history with git-rebase until we can find a commit that matches the tree state in platform2's (now restored but still ancient) shill/
(5) run `git filter-branch` to delete Android-specific files (MODULE_LICENSE_APACHE2/NOTICE/etc...) in the aosp tree
(6) run git rebase and manually rewrite all new commits to have a "shill:" prefix on it
(7) run `git format-patch` in aosp to get all the new CLs
(8) run `git am` in platform2 to apply all those new CLs
(9) use `git log` on the original aosp history to get original commit email/dates into a flat file
(10) write a script (like www/~vapier/rewrite-commit-date.sh) to use with `git rebase --exec` to read that log and reset the commit author/date details
(11) write a CL to update the platform2/README.md and check-readme.py files to make sure presubmit doesn't break
(12) `git push` the new history directly to master

> 1. https://chromium.googlesource.com/chromiumos/platform2/+/sandbox/vapier/shill

no, that's the sandbox namespace.  anyone can run `git push cros HEAD:refs/sandbox/$USER/foo` to create things like this.

since i've merged shill into master, i've cleaned up my sandbox branch as i no longer need it.

> I only see these 2 CLs with the search "owner:vapier@chromium.org status:open"

yes, there are only 3 CLs needed: update the ebuild, update the public manifest, update the internal manifest
Project Member

Comment 23 by bugdroid1@chromium.org, Sep 10

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

commit 9bb00e88895d92c9b02367f7e3b55192d11cb870
Author: Mike Frysinger <vapier@chromium.org>
Date: Mon Sep 10 05:09:07 2018

Project Member

Comment 24 by bugdroid1@chromium.org, Sep 10

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

commit c744e153151beab3abcc45af7977e0d70a29ad5e
Author: Mike Frysinger <vapier@chromium.org>
Date: Mon Sep 10 05:09:08 2018

shill: move back to platform2

BUG= chromium:785137 
TEST=precq passes

Change-Id: Ife0a6dffc1675fda27e86a7d6dc12d0ae1d125aa
Reviewed-on: https://chromium-review.googlesource.com/1149101
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Brian Norris <briannorris@chromium.org>

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

Status: Fixed (was: Started)
Seems that a repo sync after this change will blow away local shill branches for people, without a warning. This just happened to me, although I don't care since my branches were uploaded. Regardless, do you think this is the behavior CrOS developers expect? Just dont want somebody to lose local changes. That being said, I imagine there's probably some way of undoing a repo sync command.
usually repo is supposed to not prune checkouts that have branches/work in them

you should be able to find your old git repo under:
  .repo/project-objects/aosp/platform/system/connectivity/shill.git/
  .repo/projects/src/aosp/system/connectivity/shill.git/

if you `cd` into the latter, you can run `git branch -a` and such to extract your work.
Good to know, thanks!
Project Member

Comment 29 by bugdroid1@chromium.org, Sep 13

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

commit aff7547c93d70a4f0085b26a16e6ba934df9bd39
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Sep 13 06:46:00 2018

shill: add vpn-manager to the subree list

This avoids sandbox errors related to vpn-manager/service_error.h use.

BUG= chromium:785137 
TEST=precq passes

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

[modify] https://crrev.com/aff7547c93d70a4f0085b26a16e6ba934df9bd39/chromeos-base/shill/shill-9999.ebuild

Blockedon: 885315

Sign in to add a comment