Concretely, the steps here are:

1. Make //google_apis/drive its own target, rather than part of the monolithic //google_apis target.
2. Have AuthService take in IdentityManager and interact with it rather than interacting with PO2TS. This will eliminate the usage of PO2TS in //c/b/chromeos/extensions/file_manager as well as //google_apis/drive.

Making these changes will also eliminate the usage of PO2TS in //c/b/chromeos/drive.

Will eliminate the usage of O2TS in //components/drive as well.

Taking this one, as I need to resolve this now I started working on 883655.

@blundell, about your comment here:

> Concretely, the steps here are:
> 
> 1. Make //google_apis/drive its own target, rather than part of the monolithic //google_apis target.
> 2. Have AuthService take in IdentityManager and interact with it rather than interacting with PO2TS. This will eliminate the usage of PO2TS in //c/b/chromeos/extensions/file_manager as well as //google_apis/drive.

It looks to me that the patches for these steps are related to whatever gets proposed for  crbug.com/883655 . Would it be ok for you to simply list `Bug:  809440 ,  883655 ` on the related CLs, or do you want to handle it some other way?

Let's list this bug (809440) for these CLs. Thanks!

BTW, I haven't looked into depth about potential issues involved in splitting the targets: if it gets complex or ambiguous in any way, please feel free to reach out early rather than banging your head against the wall! Also please feel free to share a WIP CL early.

Thanks Colin. I've started by creating a new BUILD.gn file under google_apis/drive/BUILD.gn and offloading parts of google_apis/BUILD.gn there + changing places where //google_apis was included in favour of //google_apis/drive & friends... and I'm seeing lots of linking failures now, as expected.

My plan is to now go through such failures to learn what needs to be changed and add / fix as needed until I get everything building again.

In the meantime, if you have some advice on how to do this, or if you think the approach I'm following is not the right one, please let me know. Thanks!

Hi Mario,

I think that you'll have an easier time if rather than replacing //google_apis deps you just add //google_apis/drive deps everywhere that seems needed. Of course some of those //google_apis deps might now be unnecessary but we can look at that in a second pass. Since //components/signin doesn't depend on //google_apis/drive you naturally won't add the dep there, which should solve the circular dependency problem. Make sense?

If this ends up being too much of a nuisance just let me know and you can tag out to me to look at it; I've spent an unfortunately large amount of time dealing with Chromium's build system :P.

Actually, simply adding google_apis/drive (instead of replacing) it's what I'm doing now, I was leaving the removal for a second pass as you comment. Sorry my comment was not clear on that.

As for dealing this being a bit of a nuisance, I'll let you know if I get stuck but for now I think I'm making progress and I think it's useful for me to deal with GN at this level too, so I'd rather continue if that's ok.

Go for it!

FWIW, I have an experimental CL uploaded to https://chromium-review.googlesource.com/c/chromium/src/+/1273074 where I'm trying to get all the bots green while doing the split.

It's not looking very greenish yet, though... WIP!

Hi Mario,

AWesome! Suggestion that can cut down on your iteration: the generation_build_files step failures are due to "gn check". Those you can find and fix locally for at least Linux and ChromeOS (assuming you're developing on Linux):

gn check path/to/linux/out/dir
gn check path/to/chromeos/out/dir

If you get those working and then get those platforms building locally you would be left with a lot less round-tripping through trybots for debugging. Wrt getting things building, I would suggest working up the food chain of build targets. So something like:
services_unittests
components_unittests
content_shell
chrome
unit_tests
browser_tests

Thanks Colin. I was actually using `gn check` already, but for some reason I still haven't figured out I couldn't reproduce the failures in the bots, not even on Linux. It's weird, perhaps some of the build options I had specified made a difference... will try with the same options than the bots.

Quick heads-up: I managed to put some more bots on green and I'm now fighting against the failures on some of the android bots, that seem to be the last offenders.

@Colin: Since it's fairly convoluted to use the try bots for quick code + test cycles, I've setup a local environment to build for Android following [1], but so far I couldn't reproduce the failure (still building, though), so I figured I'd ask in case you had some early feedback?

PS: If possible, I'd love to leave this in a reviewable state before the end of the day since I'm off tomorrow (public holiday), but if that's not possible I'd like to at least leave it in a state that can be resumed by someone else if needed. Otherwise I'd continue on Monday of course.

[1] https://chromium.googlesource.com/chromium/src/+/master/docs/android_build_instructions.md

Wrt not being able to reproduce "gn check" failures locally, my guess would be that the difference is whether it's a component build or not ("is_component_build = <bool>" in args.gn). The set and hierarchy of targets change in component build, so it seems entirely plausible that "gn check" could fail in one mode but not the other (in either direction).

I'm happy to take an early look at the CL!

The effort did pay off and I'm now able to reproduce the same error locally, so I have something to work on without endless round trips to the bots.

As per the difference, I think I was using the same value of is_component_build, but I was also enabling jumbo builds locally, so maybe that made a difference.

Anyway, now looking at your early review...

Cool! jumbo build seems like it would make a difference if gn does the checking after the jumbo modifications are made, as it impacts which headers are included by which other headers.

All good and building now it seems, so I moved https://chromium-review.googlesource.com/c/chromium/src/+/1273074 into review state now.

As mentioned before, I'll be away until next Monday, so apologies in advance if I'm slow replying or can't follow up properly until then.

@blundell: A quick heads-up on the progress here:

As you know, there are a couple of CLs pending to land related this bug (splitting //google_apis/drive into its own target, see [1] & [2]) and then a third one related to  crbug.com/883655  that adds the new constructor required for AccessTokenFetcher (see [3]).

As soon as those three CLs are landed, we should be able to do the rest required here as a few more CLs on top, for which I have a WIP-and-squashed branch here already, in case you want to take a look:

  https://chromium-review.googlesource.com/c/chromium/src/+/1283039

Warning: It's not meant for review yet, but thought I'd share it to provide you with some feedback, in case you want to comment on that. Need to leave now for the day, but tomorrow I'll continue working on that, making sure the bots are green and starting to split things into logical CLs that can actually be reviewed.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1273074
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1280275
[3] https://chromium-review.googlesource.com/c/chromium/src/+/1280553

Thanks for the update! I'll wait for the split-up CLs to take a look :).

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

commit fdda9f64969342df4a958995b7f360e18e74c23c
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Thu Oct 18 13:50:45 2018

Move //google_apis/drive into its own target and update clients

The reason we're now splitting this module into its own target is
to avoid dependency cycles when migrating certain bits to the
Identity service: since AuthService lives in google_apis/drive,
we can't port it to the IdentityManager without incurring in a
cycle caused by the identity service depending on the signin
component which, in turn, depends again on google_apis.

Note that we now need to make some GN targets under chrome/browser
directly depend on //google_apis/drive as well in order to prevent
build failures caused from happening because of such targets including
headers files from //google_apis/drive. This was not required before
doing this split since those targets would implicitly pull //google_apis
through some of their direct dependencies' public_deps anyway. However,
the new //google_apis/drive target is not listed as a public_dep anywhere,
and so explicitly depending on it from those places is now required.

Making this change now allows us to break the cycle since the signin
component does not depend on anything in google_apis/drive, allowing
us to reimplement AuthService in terms of the IdentityManager.

Bug:  809440 
Change-Id: Iec117d39beb073a094c180a1d855b7f33353e0b2
Reviewed-on: https://chromium-review.googlesource.com/c/1273074
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600749}
[modify] https://crrev.com/fdda9f64969342df4a958995b7f360e18e74c23c/chrome/browser/BUILD.gn
[modify] https://crrev.com/fdda9f64969342df4a958995b7f360e18e74c23c/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/fdda9f64969342df4a958995b7f360e18e74c23c/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/fdda9f64969342df4a958995b7f360e18e74c23c/chrome/test/BUILD.gn
[modify] https://crrev.com/fdda9f64969342df4a958995b7f360e18e74c23c/components/drive/BUILD.gn
[modify] https://crrev.com/fdda9f64969342df4a958995b7f360e18e74c23c/google_apis/BUILD.gn
[add] https://crrev.com/fdda9f64969342df4a958995b7f360e18e74c23c/google_apis/drive/BUILD.gn

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

commit 26c53784109e731f413be55ce60a85e3087f8a53
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Fri Oct 19 14:46:26 2018

Overload CreateAccessTokenFetcherForAccount to accept a SharedURLLoaderFactory

This will be used to migrate AuthService to the IdentityManager, so that
we can migrate away from OAuth2TokenService::StartRequestWithContext()
but still be able to pass a custom URLLoaderfactory to AuthRequest.

Bug:  809440 
Change-Id: I349ff1fb9fea29c89fe4abb520cb652560fb65af
Reviewed-on: https://chromium-review.googlesource.com/c/1288536
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#601145}
[modify] https://crrev.com/26c53784109e731f413be55ce60a85e3087f8a53/services/identity/public/cpp/DEPS
[modify] https://crrev.com/26c53784109e731f413be55ce60a85e3087f8a53/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/26c53784109e731f413be55ce60a85e3087f8a53/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/26c53784109e731f413be55ce60a85e3087f8a53/services/identity/public/cpp/identity_manager_unittest.cc

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

commit a30584a4f47e34989d25dbb40cd06dfe69ce83b6
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Mon Oct 22 10:23:25 2018

Migrate AuthRequest to using the AccessTokenFetcher

This will remove a dependency from OAuth2TokenService::Consumer,
that will pave the way for further migrations to the IdentityManager.

Bug:  809440 
Change-Id: I81d77da696078cdc48eee63c466a815fd68062c5
Reviewed-on: https://chromium-review.googlesource.com/c/1288535
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#601516}
[modify] https://crrev.com/a30584a4f47e34989d25dbb40cd06dfe69ce83b6/google_apis/drive/BUILD.gn
[add] https://crrev.com/a30584a4f47e34989d25dbb40cd06dfe69ce83b6/google_apis/drive/DEPS
[modify] https://crrev.com/a30584a4f47e34989d25dbb40cd06dfe69ce83b6/google_apis/drive/auth_service.cc

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

commit 97e7182b18f91f4245ee21612a3ccdf6f0fdc074
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Wed Oct 24 16:55:40 2018

Migrate AuthService from OAuth2TokenService to the IdentityManager

This removes the dependency on OAuth2TokenService from AuthService,
adapting all the clients of such API across the board, that is:

  - Private files from chrome/browser/chromeos/extensions/file_manager
  - drive::DriveAPIService (components/drive/service/drive_api_service.*)

Likewise, in order to keep chromium building and tests passing, this
changeset also updates the few clients of DriveAPIService in order to
pass an IdentityManager from now on, instead of an OAuth2TokenService:

  - //c/b/chromeos/drive/drive_integration_service.cc
  - //c/b/sync_file_system/drive_backend/sync_engine.*
  - //c/b/apps/platform_apps/api/sync_file_system/sync_file_system_browsertest.cc

...where //c/b stands for //chrome/browser.

Bug:  809440 
Change-Id: I0c5691bcd2e86e8b0e7bca2f27fa42e2edc59a84
Reviewed-on: https://chromium-review.googlesource.com/c/1288571
Reviewed-by: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Cr-Commit-Position: refs/heads/master@{#602372}
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/chrome/browser/apps/platform_apps/api/sync_file_system/sync_file_system_browsertest.cc
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/chrome/browser/chromeos/drive/drive_integration_service.cc
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/chrome/browser/sync_file_system/drive_backend/sync_engine.cc
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/chrome/browser/sync_file_system/drive_backend/sync_engine.h
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/components/drive/service/drive_api_service.cc
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/components/drive/service/drive_api_service.h
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/google_apis/drive/auth_service.cc
[modify] https://crrev.com/97e7182b18f91f4245ee21612a3ccdf6f0fdc074/google_apis/drive/auth_service.h

I believe that was the last change required here, resolving!