Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412733015:
Clarify how the `analyze` step should work in MB (and elsewhere). (Closed)
|
Created:
5 years, 1 month ago by Dirk Pranke Modified:
5 years, 1 month ago CC:
chromium-reviews, jam, smut Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClarify how the `analyze` step should work in MB (and elsewhere).
(This is just a doc/spec change. Actual code changes to implement
this will follow).
R=phajdan.jr@chromium.org, sky@chromium.org
BUG=552146
Committed: https://crrev.com/06de67bed078fcc2d011412a27ce65aa8fcba48b
Cr-Commit-Position: refs/heads/master@{#359403}
Patch Set 1 : initial spec for review #
Total comments: 35
Patch Set 2 : update w/ review feedback #
Total comments: 6
Patch Set 3 : update w/ more review feedback, clean up a bit #
Messages
Total messages: 24 (6 generated)
Description was changed from ========== Clarify how the `analyze` step should work in MB (and elsewhere). (This is just a doc/spec change. Actual code changes to implement this will follow). R=phajdan.jr@chromium.org, sky@chromium.org BUG=552146 ========== to ========== Clarify how the `analyze` step should work in MB (and elsewhere). (This is just a doc/spec change. Actual code changes to implement this will follow). R=phajdan.jr@chromium.org, sky@chromium.org BUG=552146 ==========
Patchset #1 (id:1) has been deleted
Scott, Paweł, please take a look at this and make sure it's to your liking and more-or-less matches what we've been discussing. I've specified a few edge cases that we haven't previously discussed; make sure they make sense to you as well. John, Sana, feel free to weigh in as well. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:159: present in this list should be pruned. I need to go back and look at the weird static_library logic that Scott has in the GYP analyzer and incorporate it here. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:176: We have to decide how to deal with files that don't actually exist Note the rules I've given for error handling here and below (to line 211). Let me know if you disagree with any of these things. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/user_guid... File tools/mb/docs/user_guide.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/user_guid... tools/mb/docs/user_guide.md:79: `build_targets` once all the bots have been updated. NB: this might surprise you.
Some thoughts have occurred to me to change things a bit, see the comments below. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:159: present in this list should be pruned. On 2015/11/06 04:34:19, Dirk Pranke wrote: > I need to go back and look at the weird static_library logic that Scott has in > the GYP analyzer and incorporate it here. Actually, I also need to update this to note that it's okay to replace a list of targets by the corresponding meta target if that is safe and correct to do (to keep the list small). https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:165: list of unpruned targets. Need to update this to return a 'status' field as well. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:300: not test_targets. Need to update the examples to contain status fields. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/user_guid... File tools/mb/docs/user_guide.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/user_guid... tools/mb/docs/user_guide.md:79: `build_targets` once all the bots have been updated. On 2015/11/06 04:34:19, Dirk Pranke wrote: > NB: this might surprise you. Actually, having thought about this some more, I think this change is wrong and we should keep 'status' to distinguish between the 'no compile necessary' case and the 'compile necessary, build everything by passing no targets to ninja' case (the Nico clause).
Thanks, Dirk - I consider this a good way of verifying we're all on the same page - and I think we mostly are. I posted some comments, hope you'll find them useful. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:81: goal that most try jobs should complete in a half hour. Consider removing the specific number. I was not aware of any half hour goals, only for median CQ time not to exceed one hour. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:123: 'blink_tests' that might depend on ten different binaries. If a patch only Can we use "all" or "chromium_builder_tests" as an example here? I feel blink_tests is not the best one, because it's relatively small and well correlated with other targets it contains, as opposed to examples I suggested. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:145: So, we need two things as input: Well, if files are counted here, then we'd need three: 1. files 2. test_targets 3. build_targets right? So later part of the doc seems to mention that, but I find it confusing to start with 2 and just a moment later say it's actually 3. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:194: should be treated the same as compile_targets=["all"], since that's I'm worried about this. Can we not do that, at least for now? Explicit "all" has a nice property that if you add more things to it, "all" still compiles. When empty list implicitly means "all", then *adding* an element to it has a surprising effect of *less* targets getting compiled. It might make sense for a tool like ninja which gets used in many different contexts. However for our bots, I'd strongly prefer explicit "all". https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:204: We also have an "error" field in case something goes wrong (like the I'd suggest making this change the exit code of the tool, if that's not the case already. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:211: have to be fatal. I'd like it to be fatal so that it's noticeable. It currently is fatal. Is there any reason to change it?
https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:81: goal that most try jobs should complete in a half hour. On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > Consider removing the specific number. I was not aware of any half hour goals, > only for median CQ time not to exceed one hour. See https://groups.google.com/a/chromium.org/d/msgid/hackability-cy/CAPSmAATHfmL0... But, yeah, I can remove the specific number; it's not terribly important. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:123: 'blink_tests' that might depend on ten different binaries. If a patch only On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > Can we use "all" or "chromium_builder_tests" as an example here? > > I feel blink_tests is not the best one, because it's relatively small and well > correlated with other targets it contains, as opposed to examples I suggested. I don't want to use 'all' because it's special and needs to be talked about separately. I'm reluctant to use 'chromium_builder_tests' because I think that target should go away, whereas something like 'blink_tests' or 'mandoline:all' or 'remoting_all' is still reasonable. But I also see your point, so I'll think about it more. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:145: So, we need two things as input: On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > I find it confusing to start with 2 and just a moment later say it's actually 3. Yeah, I understand the confusion. I wasn't completely happy with this wording either. I'll take another pass at it. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:194: should be treated the same as compile_targets=["all"], since that's On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > I'm worried about this. Can we not do that, at least for now? Nico seems to feel strongly that we should allow this in some form. I need to figure out how to balance out your two viewpoints :). > Explicit "all" has a nice property that if you add more things to it, "all" > still compiles. Well, sort of, but I would argue that if you're providing ['all', 'foo'] as additional_compile_targets you're doing it wrong. > When empty list implicitly means "all", then *adding* an element to it has a > surprising effect of *less* targets getting compiled. This is a good point, though. > It might make sense for a tool like ninja which gets used in many different > contexts. However for our bots, I'd strongly prefer explicit "all". I will keep thinking about it. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:204: We also have an "error" field in case something goes wrong (like the On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > I'd suggest making this change the exit code of the tool, if that's not the case > already. It is done already, but I'll add that to the text to be clear. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:211: have to be fatal. On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > I'd like it to be fatal so that it's noticeable. It currently is fatal. > > Is there any reason to change it? There's two different cases and I shouldn't have merged them together. I agree that the error case should be fatal. In the case where analyze doesn't know what to do and we default to building everything, we shouldn't error out. I'll separate them out.
smut@google.com changed reviewers: + smut@google.com
https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:90: targets affected by the patch, so that we don't blame or punish the In the first * you said that with no resource constraints you would ideally build and test "everything that a patch affected". Here you seem to be saying that because of resource constraints, we want to build and test "only targets affected by the patch". How are these goals different? Does "everything that a patch affected" not mean the same as "only targets affected by the patch"? Based on point 6 below, you meant "everything" in the first case. In other words, I think the "that a patch affected" part should be dropped in the first *. Ideally we'd build everything, practically we only build what the patch affects.
https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:90: targets affected by the patch, so that we don't blame or punish the Actually, I meant "everything that a patch affected" in point 6. For purposes of testing whether a patch is correct, the requirement is *not* to build literally everything on every patch, only to build the stuff the patch affects. We don't want to fail a patch if the build fails for reasons unrelated to the patch, and if we wanted to build literally everything on every patch, we wouldn't need analyze at all. There might also be a separate point, which is that ideally we'd *also* build everything at tip-of-tree on every commit to make sure it still works, but that's a requirement that more fits the waterfall bots, where we don't run analyze. I grant your point that the wording is unclear. I'll try to fix it, but it'll probably be a bit different than just dropping "that a patch affected".
https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:81: goal that most try jobs should complete in a half hour. On 2015/11/06 at 17:38:19, Dirk Pranke wrote: > On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > > Consider removing the specific number. I was not aware of any half hour goals, > > only for median CQ time not to exceed one hour. > > See https://groups.google.com/a/chromium.org/d/msgid/hackability-cy/CAPSmAATHfmL0... My understanding is that was about main waterfall, not the tryserver. > But, yeah, I can remove the specific number; it's not terribly important. Yes, I think that'd be best for now. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:194: should be treated the same as compile_targets=["all"], since that's On 2015/11/06 at 17:38:19, Dirk Pranke wrote: > On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > > I'm worried about this. Can we not do that, at least for now? > > Nico seems to feel strongly that we should allow this in some form. > > I need to figure out how to balance out your two viewpoints :). What is the rationale for his strong opinion here? I've even offered him to have a separate recipe where he could do what he wants. FWIW I'm not aware of any trybots for the FYI clang bots so this shouldn't have big implications for "analyze". > > Explicit "all" has a nice property that if you add more things to it, "all" > > still compiles. > > Well, sort of, but I would argue that if you're providing ['all', 'foo'] as > additional_compile_targets you're doing it wrong. Why? I'd agree doing this explicitly would look a bit weird. However, when recipes just collect all compile targets, it's quite easy to end up with a test that requires say "chrome" to be compiled, and also "all" as additional compile targets. > > When empty list implicitly means "all", then *adding* an element to it has a > > surprising effect of *less* targets getting compiled. > > This is a good point, though. > > > It might make sense for a tool like ninja which gets used in many different > > contexts. However for our bots, I'd strongly prefer explicit "all". > > I will keep thinking about it. sg https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:211: have to be fatal. On 2015/11/06 at 17:38:19, Dirk Pranke wrote: > On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > > I'd like it to be fatal so that it's noticeable. It currently is fatal. > > > > Is there any reason to change it? > > There's two different cases and I shouldn't have merged them > together. > > I agree that the error case should be fatal. sg > In the case where analyze doesn't know what to do and we default > to building everything, we shouldn't error out. What do you mean by "doesn't know what to do"? I'd be leaning towards making that an error, to make it noticeable. Usually it'd indicate a bug in analyze, right? I'd strongly prefer to bring more attention to the bug and get it fixed sooner, rather than to have longer cycle times because analyze isn't working.
https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:184: and so it should return an error accordingly. Is that what happens now with an invalid target? I know analyzer reports a warning, but I wasn't sure if we actually fail. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:238: "compile_targets": ["wtf_unittests", "webkit_tests"], As compile_target is the same as test_targets, does it need to be specified in this case? It's fine to do that, but I'm wondering if we really need to? https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:288: "compile_targets": [], My knee jerk reaction is that analyzer should never infer 'all' from an empty list and we should be explicit. Especially if we're going to treat an empty compile_targets lists as 'all' but not an empty test_targets list. That's just confusing. I get that we want an empty list to mean 'all', but can that knowledge be encoded in recipes and not analyzer? So, the recipes code would be something like: if not compile_targets: compile_targets = {'all'}
https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:184: and so it should return an error accordingly. On 2015/11/09 17:17:49, sky wrote: > Is that what happens now with an invalid target? I know analyzer reports a > warning, but I wasn't sure if we actually fail. I'm pretty sure it'll fail in a GN build. I don't know what it does in a GYP build offhand. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:194: should be treated the same as compile_targets=["all"], since that's On 2015/11/09 10:37:52, Paweł Hajdan Jr. wrote: > On 2015/11/06 at 17:38:19, Dirk Pranke wrote: > > On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > > > I'm worried about this. Can we not do that, at least for now? > > > > Nico seems to feel strongly that we should allow this in some form. > > > > I need to figure out how to balance out your two viewpoints :). > > What is the rationale for his strong opinion here? Nico answered that question here: https://groups.google.com/a/chromium.org/d/msg/infra-dev/vAZjvj8xKgU/mRL5Us1Y... Basically (a) it's redundant and (b) it tests the default case that "ninja" does the same thing as "ninja all". > I'd agree doing this explicitly would look a bit weird. > > However, when recipes just collect all compile targets, it's quite easy to end > up with a test that requires say "chrome" to be compiled, and also "all" as > additional compile targets. Yes, but that's different from specifying additional_compile_targets = ["all", "foo"]. I.e., I only care about the "explicitly" case. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:238: "compile_targets": ["wtf_unittests", "webkit_tests"], On 2015/11/09 17:17:49, sky wrote: > As compile_target is the same as test_targets, does it need to be specified in > this case? It's fine to do that, but I'm wondering if we really need to? Maybe not, but I think it's easier to just specify both. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:288: "compile_targets": [], On 2015/11/09 17:17:49, sky wrote: > My knee jerk reaction is that analyzer should never infer 'all' from an empty > list and we should be explicit. Especially if we're going to treat an empty > compile_targets lists as 'all' but not an empty test_targets list. That's just > confusing. I get that we want an empty list to mean 'all', but can that > knowledge be encoded in recipes and not analyzer? So, the recipes code would be > something like: > if not compile_targets: > compile_targets = {'all'} I understand the reaction, and obviously we can make the code do whatever we want. Nico was against this (as noted above). Given that we have opinions on both sides, we have to pick one, which I will do when I update this CL :).
for anyone who cares, I wasn't able to incorporate the comments and update things yet. I will get to this tomorrow, hopefully.
Patchset #2 (id:40001) has been deleted
Okay, all the feedback has been incorporated and I've updated it to reflect the latest discussions w/ Scott in the context of his CL to update the GYP analyzer as well. Please take another look? https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:159: present in this list should be pruned. On 2015/11/06 05:08:47, Dirk Pranke wrote: > On 2015/11/06 04:34:19, Dirk Pranke wrote: > > I need to go back and look at the weird static_library logic that Scott has in > > the GYP analyzer and incorporate it here. Okay, I've looked at the weird static library logic and I think it's an implementation detail and we don't need to spell it out in the spec. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:194: should be treated the same as compile_targets=["all"], since that's On 2015/11/09 20:15:21, Dirk Pranke wrote: > On 2015/11/09 10:37:52, Paweł Hajdan Jr. wrote: > > On 2015/11/06 at 17:38:19, Dirk Pranke wrote: > > > On 2015/11/06 15:27:53, Paweł Hajdan Jr. wrote: > > > > I'm worried about this. Can we not do that, at least for now? > > > > > > Nico seems to feel strongly that we should allow this in some form. > > > > > > I need to figure out how to balance out your two viewpoints :). > > I've decided that Nico loses out :). Everyone else seems to think trying to handle [] as ["all"] will be fragile and weird, plus there's the general concern that in most cases where we're running analyze on the try servers we often don't want to default to building all because it will make the cycle time too long. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:238: "compile_targets": ["wtf_unittests", "webkit_tests"], On 2015/11/09 20:15:20, Dirk Pranke wrote: > On 2015/11/09 17:17:49, sky wrote: > > As compile_target is the same as test_targets, does it need to be specified in > > this case? It's fine to do that, but I'm wondering if we really need to? > > Maybe not, but I think it's easier to just specify both. As we've discussed, I'm changing this to 'additional_compile_targets' so that we don't have this duplication. https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:288: "compile_targets": [], On 2015/11/09 20:15:20, Dirk Pranke wrote: > Given that we have opinions on both sides, we have to pick one, which I will do > when I update this CL :). Updated: Nico loses :).
LGTM w/minor comment (up to you) https://codereview.chromium.org/1412733015/diff/60001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/60001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:149: * `test_targets`: the list of ninja targets which, if out of date, should Wouldn't "affected by the patch" more accurate than "out of date"? If targets do not require rebuild, that'll be handled by ninja anyway. The way I read this, I'd prefer to have focus on "affected by the patch".
I would like to see us better handle gyp files being modified. Analyze does this now. Is it possible to make gn analyze do this too? https://codereview.chromium.org/1412733015/diff/60001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/60001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:111: 3. (We might also want to know when test targets are affected by data files nit: remove leading '('? https://codereview.chromium.org/1412733015/diff/60001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:222: In the case where build files themselves are modified and analyze may There are actually two different cases for analyze: . you've modified a file specified as an include file, eg common.gypi. In that case you have to assume everything has changed. . you modify a random gyp file, say base/base.gyp. In that case you can assume all the build targets in that file are dirty and continue on. Doing this is nice in that it means someone trivially adding a file to a target doesn't end up building/running all.
sky wrote: > I would like to see us better handle gyp files being modified. > Analyze does this now. Is it possible to make gn analyze do > this too? We can improve things, but see the comments below. https://codereview.chromium.org/1412733015/diff/60001/tools/mb/docs/design_sp... File tools/mb/docs/design_spec.md (right): https://codereview.chromium.org/1412733015/diff/60001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:111: 3. (We might also want to know when test targets are affected by data files On 2015/11/12 16:59:09, sky wrote: > nit: remove leading '('? Will do. https://codereview.chromium.org/1412733015/diff/60001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:149: * `test_targets`: the list of ninja targets which, if out of date, should On 2015/11/12 15:23:03, Paweł Hajdan Jr. wrote: > Wouldn't "affected by the patch" more accurate than "out of date"? > > If targets do not require rebuild, that'll be handled by ninja anyway. > > The way I read this, I'd prefer to have focus on "affected by the patch". You're right, "affected by the patch" is better. Will fix. https://codereview.chromium.org/1412733015/diff/60001/tools/mb/docs/design_sp... tools/mb/docs/design_spec.md:222: In the case where build files themselves are modified and analyze may On 2015/11/12 16:59:09, sky wrote: > There are actually two different cases for analyze: > . you've modified a file specified as an include file, eg common.gypi. In that > case you have to assume everything has changed. > . you modify a random gyp file, say base/base.gyp. In that case you can assume > all the build targets in that file are dirty and continue on. Doing this is nice > in that it means someone trivially adding a file to a target doesn't end up > building/running all. In GN you can't easily distinguish the two cases, because you can define a config in a .gn file that gets depended on from other files, but we can't tell what targets depend on that config easily. I agree that we should try to handle as many cases as we can, but I expect there will be some we can't for the foreseeable future, and I don't think we need to go into the details here, so I'd prefer to leave things vague.
Ok, LGTM
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/1412733015/#ps80001 (title: "update w/ more review feedback, clean up a bit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412733015/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412733015/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/06de67bed078fcc2d011412a27ce65aa8fcba48b Cr-Commit-Position: refs/heads/master@{#359403} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
