The first bullet point in the last block, about different things in the same place, isn't an issue. We simply don't allow that at all. Getting rid of deps_os and replacing it with conditions means no two things can ever point at the same path, and the same should be true for cipd.

Bumping priority, as this is blocking a few things that we want to make progress on and is generally blocking further use of CIPD in src.

agable, iannucci, and I discussed this to some extent in a hangout that regrettably had history disabled. Salient parts of the conversation:

iannucci
--------
I feel like the desired behavior is something along the lines of
each DEPS file processed by gclient
will render to a single ensure file
and gclient will invoke `cipd ensure`
at the location of the DEPS file
once per DEPS file
per sync

agable
------
Why once per DEPS file?
That's in the middle between "once per dep" and "once per gclient root"

iannucci
--------
once per dep is clearly undesired
once per gclient root, I think, would be more difficult to implement
though maybe that's not the case?

jbudorick
---------
once per DEPS file would essentially be once per CIPD root, eh?

iannucci
--------
yes exactly
each DEPS file would ~= a cipd root

[extended discussion on that resulting in us all agreeing that 1 CIPD root / DEPS file should be viable]

iannucci
--------
so at this point
it gets into the ... vagueries
of gclient
I'm not sure how all the DEPS recursion stuff works
or if there's a sane place to insert this code

agable
------
Basically, when it encounters a recursed-upon repo it:
1) checks out that repo as normal
2) parses its DEPS file
3) (if configured to do so) prepends the path to that DEPS file to all paths contained within it
4) appends the deps and hooks from that file to the global list
5) continues its BFS of the dependency graph
There are in-memory objects representing each DEPS file it encounters, I think
Maybe they stick around for calculating all the hooks after deps are done syncing?
So you could insert a cipd step between deps and hooks?

iannucci
--------
I think that would be ideal
ideally all cipd packages should be available before the hooks run

agable
------
Yeah ok the in-memory tree object is a tree of Dependency objects
each of which optionally has its own set of dependencies, hooks, etc if it has its own DEPS file
So that class could just grow a ._cipd_packages member or something

iannucci
--------
ok
oh, right
the other question was
how do the cipd packages get bodged into DEPS
are they additional 'deps' entries?
I know that Aaron you were leaning that way, right?
I'm not sure that this approach is flexible enough for conditions and stuffs

agable
------
Right, which would actually change what I just said 30 seconds ago :)
So the two main options are:
a) Add a top-level "cipd = {}" to the DEPS file schema, add a ._cipd_packages member to the Dependency class, and crawl the tree of Dependencies again after finishing deps but before starting hooks
b) incorporate cipd packages into the existing "deps = {}" part of the DEPS file schema, modify the existing Dependency object to be able to represent a CIPD dependency (maybe as a subclass with a ._parsed_package instead of the current ._parsed_url?), and then process them in-line with the git-based deps
You'd probably do the latter by adding a new class to gclient_scm.py
There's already a lot of scaffolding in there to handle git vs svn dependencies; a lot of that can probably be reused for git vs cipd deps.

iannucci
--------
fwiw, my preference is a

agable
------
Well mostly, I'm in favor of (b) :P

Blocking issue 475693 on this, since it may help implement the replacement of "hermetic" Xcode using CIPD. With a caveat that Xcode CIPD packages are only avaialble to Googlers... Not sure if gclient conditions are expressive enough for that.

You can set a gclient flag to indicate "I want internal things", but you can't (currently) set a flag to determine if "I can get internal things". I think it would be fine to look into adding something like this if we need to, and if we can make it sufficiently generic (like how exec_script() works in GN).

We discussed this exact feature with iannucci where we would like to use it for exactly that purpose. We went through several ideas and the last one we settled on was to extend the ensure files to support ? operator in the package name:

some/package/pattern/${platform}? a_cool:tag

This would mean "install the package if you have permissions, ignore otherwise". Does this seem reasonable?

Er... I didn't agree to that :D

The reason I don't like tagging stuff in the ensure file is that there's no way for cipd to know if you SHOULD have permissions or not. An explicit "I want internal stuff" condition would allow gclient to unequivocally add it to its list of packages to install. Then cipd operates as-usual, and if there's a permission issue, it will fail and the user will need to resolve the issue.

I'm worried about the case where the user (mentally) "wants the internal things", doesn't log in (or logs in with the wrong account), and gclient "works", but doesn't actually get the internal things. I would vastly prefer a solution where the user tells the tool (hopefully once) "I want the internal stuff, so do what needs to be done to make that happen".

That means we'll have to integrate CIPD into gclient and Jiri rather than simply relying on ensure files though; this could mean generating ensure file, passing an additional one (or even integrating CIPD directly in case of Jiri).

+1 to #c10 - in this case, we don't need to check if a dev is a Googler, the dev will tell that to gclient explicitly, and I'll add it to the docs on Xcode installation. I especially like the fact that this flag only needs to be set once for all current and future internal dependencies.

(re: #11 I would note that this entire bug is about integrating cipd into gclient :))

Working on adding this to gclient this week.

One concern here: whatever URL I put in to represent a CIPD package won't have a good web representation. The chrome-infra-packages.appspot.com webapp is ... not in good shape.

+vadimsh raised some concerns about this effort on my CL to add -subdir to cipd install (https://chromium-review.googlesource.com/c/infra/luci/luci-go/+/826050).

I have thus far been working on option b as discussed in #6.

Are you aware of @Subdir option in ensure file? E.g https://godoc.org/go.chromium.org/luci/cipd/client/cipd/ensure#hdr-Example

I think existing 'cipd ensure' + @Subdir option in the ensure file are sufficient to implement the integration.

indeed, I am: https://codesearch.chromium.org/chromium/src/build/cipd/android/android.ensure?rcl=fbae541fa00bdfffcc720a3302a5e7e516904d61&l=9

Is there a design doc for the integration you're working on? It looks like you have some other implementation in mind... The last we discussed (and what I outlined in this bug) is to have gclient calculate an ensure file, 1 per DEPS-file, and then call `cipd ensure` once per DEPS-file.

Not at the moment.

Per-package management fits in much better with gclient's existing structure, and in rereading through #6 and previously talking to you about install vs ensure, I must've misremembered what we'd discussed.

(especially given ensure's unfriendliness toward local modifications that can happen on dev machines)

To clarify, what I was intending before was for gclient to:
  * Do all of its DEPS processing, figure out what's included/what's not
  * Calculate a single ensure file
  * Call `cipd ensure -root path/to/checkout --ensure-file -` (where - is stdin; gclient would feed the ensure file directly to cipd)

The questions I had raised centered around syntax inside the DEPS file (either part of deps, or a new cipd top-level thingy), as well as the behavior of gclient processing stuff (does it generate a single uber-ensure-file, and have a single cipd root with really deep subdirs? or does it make each repo self-contained by having each repo be its own cipd root with shallower subdirs? My strong preference is the former, since it means that the shape of repo X is the same whether or not its checked out as the main solution, or as part of some other repo's DEPS). All of the design proposals were intended to culminate with an invocation of `cipd ensure`.

Which unfriendliness is that?

I have *definitely* discussed this part w/ you in the past re ensure's use of the cache as the source of truth for what's set up in the tree -- "I have package X locally, thus the symlink must exist". I would strongly prefer to not have to tell many (more) chromium devs to delete their src/.cipd directories and then rerun gclient sync.

I remember an issue where someone was taking a checkout and configuring it for one platform, and then another platform, and things were switching back and forth... I thought the conclusion was that the conditions in gclient should support "mac+linux+windows" mode? I also remember a different possibility where we might add a -leave-trash option on ensure which would never delete any packages (i.e. skip all remove actions). I don't remembering settling on one or the other.

Oh, right, I think you filed a separate feature request for that behavior?

re #25: that's a different issue than what I meant w/ the unfriendliness; w/ either install or ensure, gclient conditions will wind up determining what gets installed/added to the ensure file.

(I can't find it now though? I'll file a new one and we can dedup the other one when you find it.)

#26: not yet; we've discussed it, and a flag would be a viable long-term solution as far as I'm concerned, but I was ok w "delete src/.cipd" in the short term.

Well, here's the bug:  crbug.com/794764 

Hm... you say that deleting .cipd and resyncing is an ok short-term workaround, but you are also saying that it's severe enough to block the integration of gclient with cipd ensure... I think I'm losing something :(. Either it's a severe issue which blocks the integration, or it's a low-priority non-blocker, right?

I'm currently expecting to manually nuke .cipd & re-ensure in gclient revert.

and to be clear: my strong preferences don't necessarily constitute blockers :)

(though they may inform my initial implementation decisions when I forget parts of our prior discussion)

I think if we implement  crbug.com/794764  you could use that flag in gclient to make sure that cipd ensure ensures the state of the filesystem, regardless of whatever chaos monkeys had their way with the checkout (probably short of manually editing files inside of the .cipd directory to alter their contents).

This will definitely come with a performance cost though, so it might be worth an opt-in (or opt-out) switch.

Probably will still be faster than download_from_google_storage, however, so all in all it's still better than before.

on review: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/829953

FYI issue 789364 - as a potential additional requirement for how gclient supports CIPD.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/0f7b2007a53d5158e42e97b938fd03f57cdf9786

commit 0f7b2007a53d5158e42e97b938fd03f57cdf9786
Author: John Budorick <jbudorick@chromium.org>
Date: Sun Jan 21 18:57:08 2018

Add cipd support to gclient.

Bug:  789809 
Change-Id: I9942eaa613d51edd77ee5195603932a103f5e3cd
Reviewed-on: https://chromium-review.googlesource.com/829953
Commit-Queue: John Budorick <jbudorick@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>

[modify] https://crrev.com/0f7b2007a53d5158e42e97b938fd03f57cdf9786/gclient_eval.py
[modify] https://crrev.com/0f7b2007a53d5158e42e97b938fd03f57cdf9786/gclient.py
[modify] https://crrev.com/0f7b2007a53d5158e42e97b938fd03f57cdf9786/gclient_scm.py
[modify] https://crrev.com/0f7b2007a53d5158e42e97b938fd03f57cdf9786/tests/gclient_test.py
[modify] https://crrev.com/0f7b2007a53d5158e42e97b938fd03f57cdf9786/tests/gclient_scm_test.py

w/ #38 landing & sticking, CIPD packages can now be added to DEPS. Dependency specifications should take the following form:


  '$INSTALLATION_DIRECTORY': {
    'packages': [
       {
           'package': '$PACKAGE_NAME',  # e.g. android_support_test_runner
           'version': '$PACKAGE_VERSION',  # e.g. version:0.5-cr0
       },
       ...  # other packages to be installed in the same directory
    ],
    'condition': '$CONDITION',  # optional; e.g. checkout_android
    'dep_type': 'cipd',
  },

I'll be adding documentation to src/ with the CL that moves our existing CIPD dependencies into DEPS.

er, tweaks to #40:
 - should've referred to the CL in #39, not in #38, obviously
 - example $INSTALLATION_DIRECTORY given the other examples would be src/third_party/android_support_test_runner

Is there any sort of design note or something that talks about how this is implemented (or are you planning to add that as part of the documentation)?

I'm wondering what ended up happening vis-a-vis `cipd ensure` et al ...

The documentation I'm currently writing is focused on users in src.

This was implemented basically doing what agable mentioned as option 2 in https://bugs.chromium.org/p/chromium/issues/detail?id=789809#c6. It builds up a list of packages that should be installed given their conditions, writes an ensure file, and runs cipd ensure. I'm happy to expound more here or, probably more usefully, inline.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/3929e9ee94affead0d2e5c8e6c12e1ce60b09e2a

commit 3929e9ee94affead0d2e5c8e6c12e1ce60b09e2a
Author: John Budorick <jbudorick@chromium.org>
Date: Mon Feb 05 02:26:49 2018

Add a name attribute to CipdWrapper.

TBR=agable@chromium.org

Bug:  789809 
Change-Id: I4ede72a3584b23d01063e5cfa20401c921762b50
Reviewed-on: https://chromium-review.googlesource.com/900664
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: John Budorick <jbudorick@chromium.org>

[modify] https://crrev.com/3929e9ee94affead0d2e5c8e6c12e1ce60b09e2a/gclient_scm.py

 Issue 663843  has been merged into this issue.