Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1919103003:
Add ability to override DEPS file in recursedeps. (Closed)
|
Created:
4 years, 8 months ago by iannucci Modified:
4 years, 7 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionAdd ability to override DEPS file in recursedeps.
This will allow ANGLE to have a recursible deps-file in their repo instead of
relying on chromium's DEPS file to specify an accurate dependency for itself.
R=agable@chromium.org, jmadill@chromium.org
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300293
Patch Set 1 #
Total comments: 3
Messages
Total messages: 18 (3 generated)
PTAL. The intent here is that recursedeps has been changed so that instead of
just
recursedeps = ["dep", "other"]
It now allows
recursedeps = ["dep", ("other", "DEPS.special")]
Which will recurse into both dep and other, using the 'default' DEPS file for
dep, and then using "DEPS.special" when recursing into other.
There's even a test, which means that it MIGHT EVEN WORK.
Is this because angle has one 'DEPS' file that they use in a standalone build, and another that they want recursed into when part of the chromium build? (I'm trying to understand why they can't use 'DEPS' in the part-of-chromium case).
https://codereview.chromium.org/1919103003/diff/1/gclient.py File gclient.py (right): https://codereview.chromium.org/1919103003/diff/1/gclient.py#newcode663 gclient.py:663: self.recursedeps[ent[0]] = {"deps_file": ent[1]} I changed the internal representation entirely. It's now either None, or a dictionary, and every dictionary entry has the same {"deps_file": filename} format. I picked "deps_file", because that's the key used in .gclient. https://codereview.chromium.org/1919103003/diff/1/gclient.py#newcode720 gclient.py:720: if self.recursedeps is not None: it could be None in the case that we're still using recursion_limit (which is apparently yet another deprecated-but-still-in-the-code feature) https://codereview.chromium.org/1919103003/diff/1/tests/gclient_test.py File tests/gclient_test.py (right): https://codereview.chromium.org/1919103003/diff/1/tests/gclient_test.py#newco... tests/gclient_test.py:868: write(os.path.join('bar', 'DEPS'), 'ERROR ERROR ERROR') This test would fail before this change, because it would recurse into bar using this DEPS error. I've confirmed that this test fails in that condition (with a SyntaxError during the test execution, as you would expect)
On 2016/04/27 01:20:52, Dirk Pranke wrote: > Is this because angle has one 'DEPS' file that they use in a standalone build, > and another that they > want recursed into when part of the chromium build? > > (I'm trying to understand why they can't use 'DEPS' in the part-of-chromium > case). Yes that's the reason. The specific reason is that they depend on dEQP, which is pulled in by chromium's DEPS purely to facilitate ANGLE, but is also pulled via ANGLE's DEPS when they do standalone builds. When they change the pinned hash for it in the ANGLE side, there's no way to test the change with their 'chromium' trybots (which use chromium+ANGLE CL). After this change, they would remove dEQP from chromium's DEPS, and change chromium to recurse into ANGLE with e.g. DEPS.chromium that contains just dEQP (at the correct pin). This way they could make a self-contained CL that would roll dEQP for the standalone and chromium builds that could land via normal CQ means. It's clearly a stopgap until you and agable come up with your new proposal, but I don't think this adds an unsupportable feature. If there's a way I can make it more palatable, please let me know.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm (i.e., this is fine).
On 2016/04/27 01:34:54, Dirk Pranke wrote: > lgtm (i.e., this is fine). Thanks :) agable + Jamie can you take a look too and make sure this looks right?
On 2016/04/27 01:49:38, iannucci wrote: > On 2016/04/27 01:34:54, Dirk Pranke wrote: > > lgtm (i.e., this is fine). > > Thanks :) > > agable + Jamie can you take a look too and make sure this looks right? This LGTM modulo a snag or two - Tested locally, and it definitely unblocks allowing standalone ANGLE to live with a secondary DEPS file, so that's the most important thing. The downside is that for standalone ANGLE we'll now have to maintain two DEPS files with the secondary entries duplicated in both. IE DEPS will have dEQP, and DEPS.recurse will also have the same revision. This isn't a huge problem but thought I'd mention in case you had an idea of how to fix.
LGTM. jmadill is right, that this does still only allow recursing on files within other dependencies, and so requires them to maintain the same entry in two files (which, as we know from build_internal, sucks). But I don't think there's much to be done about that.
On 2016/04/27 17:13:02, agable wrote: > LGTM. > > jmadill is right, that this does still only allow recursing on files within > other dependencies, and so requires them to maintain the same entry in two files > (which, as we know from build_internal, sucks). But I don't think there's much > to be done about that. That's fine, it's only a few lines duplicated, and it's much better that they're duped in the same repo, instead of between two.
The CQ bit was checked by iannucci@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919103003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919103003/1
Message was sent while issue was closed.
Description was changed from ========== Add ability to override DEPS file in recursedeps. This will allow ANGLE to have a recursible deps-file in their repo instead of relying on chromium's DEPS file to specify an accurate dependency for itself. R=agable@chromium.org, jmadill@chromium.org BUG= ========== to ========== Add ability to override DEPS file in recursedeps. This will allow ANGLE to have a recursible deps-file in their repo instead of relying on chromium's DEPS file to specify an accurate dependency for itself. R=agable@chromium.org, jmadill@chromium.org BUG= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300293 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300293
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1932063002/ by martiniss@chromium.org. The reason for reverting is: Looks like this broke some bots; see https://build.chromium.org/p/chromium.infra.cron/builders/recipe-autoroller-p... https://build.chromium.org/p/chromium.infra.cron/builders/gsubtreed-luci-py/b....
Message was sent while issue was closed.
On 2016/04/28 21:56:46, martiniss wrote: > A revert of this CL (patchset #1 id:1) has been created in > https://codereview.chromium.org/1932063002/ by mailto:martiniss@chromium.org. > > The reason for reverting is: Looks like this broke some bots; see > https://build.chromium.org/p/chromium.infra.cron/builders/recipe-autoroller-p... > > https://build.chromium.org/p/chromium.infra.cron/builders/gsubtreed-luci-py/b.... Have repro, this CL did cause recursedeps checkouts to become SVN. Not sure why.
Message was sent while issue was closed.
On 2016/05/03 21:59:11, iannucci wrote: > On 2016/04/28 21:56:46, martiniss wrote: > > A revert of this CL (patchset #1 id:1) has been created in > > https://codereview.chromium.org/1932063002/ by mailto:martiniss@chromium.org. > > > > The reason for reverting is: Looks like this broke some bots; see > > > https://build.chromium.org/p/chromium.infra.cron/builders/recipe-autoroller-p... > > > > > https://build.chromium.org/p/chromium.infra.cron/builders/gsubtreed-luci-py/b.... > > Have repro, this CL did cause recursedeps checkouts to become SVN. Not sure why. New CL is here: https://chromiumcodereview.appspot.com/1948853002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
