Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
481433003:
Makes the analyzer output the set of targets needing a build (Closed)
|
Created:
6 years, 4 months ago by sky Modified:
6 years, 4 months ago Reviewers:
scottmg CC:
gyp-developer_googlegroups.com Base URL:
http://gyp.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionMakes the analyzer output the set of targets needing a build
The set of build targets is the minimal set of targets reachable
from the all target that contains one of the specified files (matched
target), or depeneds on the set of matched targets.
BUG=109173
TEST=none
R=scottmg@chromium.org
Committed: https://code.google.com/p/gyp/source/detail?r=1966
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : affected #
Total comments: 19
Patch Set 4 : integrate review feedback #Patch Set 5 : change algorithm #Patch Set 6 : cleanup #
Total comments: 8
Patch Set 7 : review feedback #
Messages
Total messages: 17 (0 generated)
Trying to understand this now. In the description """ The set of effected targets is the minimal set of targets reachable from the all target that contains one of the specified files (matched target), or depeneds on the set of matched targets. """ what will a "matched target" be in practice? https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:11: ignore_targets: list of targets not to include in effected_targets. If a target All the 'effected/Effected's should be 'affected/Affected'.
https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:11: ignore_targets: list of targets not to include in effected_targets. If a target On 2014/08/15 19:44:45, scottmg wrote: > All the 'effected/Effected's should be 'affected/Affected'. I *knew* I would get that wrong;)
My email response likely didn't make it. For posterity: On 2014/08/15 19:44:46, scottmg wrote: > Trying to understand this now. In the description > > """ > The set of effected targets is the minimal set of targets reachable from the all > target that contains one of the specified files (matched target), or depeneds on > the set of matched targets. > """ > > what will a "matched target" be in practice? Matched targets are those that have a source/input/rule/whatever that matches one of the file names supplied to the analyzer. For example, if one of the files names is 'ui/views/view.cc' then one of the matched targets will be the views target. If you can think of a clearer name, I'm all for it. > > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > File pylib/gyp/generator/analyzer.py (right): > > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > pylib/gyp/generator/analyzer.py:11: ignore_targets: list of targets not to > include in effected_targets. If a target > All the 'effected/Effected's should be 'affected/Affected'.
Renamed to affected. But maybe I should go with compile_targets as that is the expectation of how it'll be used? WDYT? I also changed the early out of all changed to include the passed in targets.
I think I'm still missing something at a high level, questions below. (couple 'effected's in the description too :) https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:13: These targets will be output if they contained one of the supplied file names. so, if there's a target in ignored_targets, and something in that target changes it _will_ be output? https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:19: indirectly depend upon the set of paths supplied in files. This is not output "This is not output"? It looks like it is. https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:173: match_staus: one of the MatchStatus values. match_staus -> match_status https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:276: targets[target_name] = target this seems duplicated with code at 314, could one of them be removed? https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:455: # then when we resurse from the root targets we don't attempt to include resurse -> recurse https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:456: # a Target that would be included by way of another Targets depedendencies. dependencies https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:470: ignore_plus_target = set(ignore_targets) sorry, i don't quite understand ignore_targets still. when do we use it? https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... File test/analyzer/gyptest-analyzer.py (right): https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... test/analyzer/gyptest-analyzer.py:18: f = open('test_file', 'w') could you add a doc comment here? I find it confusing what's being created. |files| and |targets| are both what's noted to have changed? Or files are what's changed and we would like to build |targets|? https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... test/analyzer/gyptest-analyzer.py:301: EnsureContains(matched=True, targets={'a'}, affected_targets={'a', 'b'}) Huh, I hadn't seen set notation before https://docs.python.org/2/library/stdtypes.html#set . I think there was still some CrOS bots that use 2.6 or something (it's fine I think, but just in case someone complains something broke) https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... test/analyzer/gyptest-analyzer.py:309: EnsureContains(matched=True, affected_targets={'d'}) could you explain this one? f.c changed, so 'f' is affected. why up only 1 to 'd'? all the targets are executable, does this one need to change if d is a shared_library, i.e. a/b might need a relink in that case? https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... test/analyzer/gyptest-analyzer.py:347: _CreateTestFile(['f.c'], []) comment here that we switched to test4.gyp
https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:13: These targets will be output if they contained one of the supplied file names. On 2014/08/15 21:30:24, scottmg wrote: > so, if there's a target in ignored_targets, and something in that target changes > it _will_ be output? Ya, sorry, this is all rather complex. Consider the All target in build/all.gyp. It depends upon mojo.gyp:* . Lets say you modified a mojo/spy/spy.cc, which is in the target mojo.gyp:mojo_spy. Since All depends upon mojo.gyp:* it depends upon mojo.gyp:mojo_spy. And since mojo_spy has been modified this means we need to compile both mojo_spy and the All target. The All target doesn't contain any files, so we don't want to trigger compiling it. This is what the ignore_targets are for. With All in ignore_targets All won't be added to affected_targets, unless it has a source that matched the set of files specified. I thought perhaps I could key off the build_type. That is, instead of ignore_targets key off a build_type of None. But I couldn't convince myself that was right. Some build_types of None have actions, and it seems like if such a target depends upon a matched target then it needs to be built. https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:19: indirectly depend upon the set of paths supplied in files. This is not output On 2014/08/15 21:30:24, scottmg wrote: > "This is not output"? It looks like it is. Removed. https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:173: match_staus: one of the MatchStatus values. On 2014/08/15 21:30:24, scottmg wrote: > match_staus -> match_status Done. https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:276: targets[target_name] = target On 2014/08/15 21:30:24, scottmg wrote: > this seems duplicated with code at 314, could one of them be removed? They are slightly different, but I can indeed share parts. Done. https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:455: # then when we resurse from the root targets we don't attempt to include On 2014/08/15 21:30:24, scottmg wrote: > resurse -> recurse Done. https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:456: # a Target that would be included by way of another Targets depedendencies. On 2014/08/15 21:30:24, scottmg wrote: > dependencies Done. https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... pylib/gyp/generator/analyzer.py:470: ignore_plus_target = set(ignore_targets) On 2014/08/15 21:30:24, scottmg wrote: > sorry, i don't quite understand ignore_targets still. when do we use it? Sorry, I missed your earlier WTF. I responded to it. Hopefully it makes sense. https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... File test/analyzer/gyptest-analyzer.py (right): https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... test/analyzer/gyptest-analyzer.py:18: f = open('test_file', 'w') On 2014/08/15 21:30:24, scottmg wrote: > could you add a doc comment here? I find it confusing what's being created. > |files| and |targets| are both what's noted to have changed? Or files are what's > changed and we would like to build |targets|? Done. https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... test/analyzer/gyptest-analyzer.py:309: EnsureContains(matched=True, affected_targets={'d'}) On 2014/08/15 21:30:24, scottmg wrote: > could you explain this one? f.c changed, so 'f' is affected. why up only 1 to > 'd'? > > all the targets are executable, does this one need to change if d is a > shared_library, i.e. a/b might need a relink in that case? My assumption, which could certainly be bogus, is that only the targets that depend upon matched targets need to be recompiled. This detection isn't about the actual running phase, just the minimal set of targets to compile. Lets simplify this to A depends on B and B depends on C. If some file in C changes, I certainly need to recompile C, and I need to recompile B because files in B depend upon C. But do I also need to recompile A? None of the files in A or its dependency B have changed. If A were an executable and I wanted to run A, then certainly I would need to compile A. This phase isn't about running, just about compiling. On the chromium side if I cared about actually running A I would pass in A as one of the targets, which would trigger A being returned both as a target and affected_target because one its descendant dependencies was changed. What I'm trying to cover with affected_targets is only compiling targets that are not in the set of tests/executables but targets that need to be recompiled because of a change. Does that make sense? It could certainly be my logic is wrong here. At the end of the day what I want to ensure is that we build as little as possible, but is still correct. That if you were to build the set of targets returned, then build all you don't get any compile errors. No doubt if dependencies are wrong that won't happen, but all of this assumes dependencies are set up correctly. https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... test/analyzer/gyptest-analyzer.py:347: _CreateTestFile(['f.c'], []) On 2014/08/15 21:30:24, scottmg wrote: > comment here that we switched to test4.gyp Done.
On 2014/08/15 22:39:37, sky wrote: > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > File pylib/gyp/generator/analyzer.py (right): > > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > pylib/gyp/generator/analyzer.py:13: These targets will be output if they > contained one of the supplied file names. > On 2014/08/15 21:30:24, scottmg wrote: > > so, if there's a target in ignored_targets, and something in that target > changes > > it _will_ be output? > > Ya, sorry, this is all rather complex. Consider the All target in build/all.gyp. > It depends upon mojo.gyp:* . Lets say you modified a mojo/spy/spy.cc, which is > in the target mojo.gyp:mojo_spy. Since All depends upon mojo.gyp:* it depends > upon mojo.gyp:mojo_spy. And since mojo_spy has been modified this means we need > to compile both mojo_spy and the All target. The All target doesn't contain any > files, so we don't want to trigger compiling it. This is what the ignore_targets > are for. With All in ignore_targets All won't be added to affected_targets, > unless it has a source that matched the set of files specified. Aha, got it. I can't think of a better name, but it does seem odd that ignore_targets might not be ignored (if they actually have files in them that change.) Will we actually use that or could we error if something that's not like All is passed as an ignore? (Or, I guess not, since otherwise you wouldn't need the case where ignore contains sources?) > > I thought perhaps I could key off the build_type. That is, instead of > ignore_targets key off a build_type of None. But I couldn't convince myself that > was right. Some build_types of None have actions, and it seems like if such a > target depends upon a matched target then it needs to be built. Yeah, I don't think None is sufficient. > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > File pylib/gyp/generator/analyzer.py (right): > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > pylib/gyp/generator/analyzer.py:19: indirectly depend upon the set of paths > supplied in files. This is not output > On 2014/08/15 21:30:24, scottmg wrote: > > "This is not output"? It looks like it is. > > Removed. > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > pylib/gyp/generator/analyzer.py:173: match_staus: one of the MatchStatus values. > On 2014/08/15 21:30:24, scottmg wrote: > > match_staus -> match_status > > Done. > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > pylib/gyp/generator/analyzer.py:276: targets[target_name] = target > On 2014/08/15 21:30:24, scottmg wrote: > > this seems duplicated with code at 314, could one of them be removed? > > They are slightly different, but I can indeed share parts. Done. > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > pylib/gyp/generator/analyzer.py:455: # then when we resurse from the root > targets we don't attempt to include > On 2014/08/15 21:30:24, scottmg wrote: > > resurse -> recurse > > Done. > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > pylib/gyp/generator/analyzer.py:456: # a Target that would be included by way of > another Targets depedendencies. > On 2014/08/15 21:30:24, scottmg wrote: > > dependencies > > Done. > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > pylib/gyp/generator/analyzer.py:470: ignore_plus_target = set(ignore_targets) > On 2014/08/15 21:30:24, scottmg wrote: > > sorry, i don't quite understand ignore_targets still. when do we use it? > > Sorry, I missed your earlier WTF. I responded to it. Hopefully it makes sense. > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > File test/analyzer/gyptest-analyzer.py (right): > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > test/analyzer/gyptest-analyzer.py:18: f = open('test_file', 'w') > On 2014/08/15 21:30:24, scottmg wrote: > > could you add a doc comment here? I find it confusing what's being created. > > |files| and |targets| are both what's noted to have changed? Or files are > what's > > changed and we would like to build |targets|? > > Done. > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > test/analyzer/gyptest-analyzer.py:309: EnsureContains(matched=True, > affected_targets={'d'}) > On 2014/08/15 21:30:24, scottmg wrote: > > could you explain this one? f.c changed, so 'f' is affected. why up only 1 to > > 'd'? > > > > all the targets are executable, does this one need to change if d is a > > shared_library, i.e. a/b might need a relink in that case? > > My assumption, which could certainly be bogus, is that only the targets that > depend upon matched targets need to be recompiled. This detection isn't about > the actual running phase, just the minimal set of targets to compile. > > Lets simplify this to A depends on B and B depends on C. If some file in C > changes, I certainly need to recompile C, and I need to recompile B because > files in B depend upon C. But do I also need to recompile A? None of the files > in A or its dependency B have changed. If A were an executable and I wanted to > run A, then certainly I would need to compile A. This phase isn't about running, > just about compiling. On the chromium side if I cared about actually running A I > would pass in A as one of the targets, which would trigger A being returned both > as a target and affected_target because one its descendant dependencies was > changed. What I'm trying to cover with affected_targets is only compiling > targets that are not in the set of tests/executables but targets that need to be > recompiled because of a change. OK, I think I understand (maybe :). But you want to make sure that A still builds if something in C might affect it, right? If A is an exe, and B and C are libs, with A containing "extern void f()", and that's in C as "void f()", but then c.cc changes to rename/delete that, won't we miss that A no longer links? > Does that make sense? It could certainly be my logic is wrong here. At the end > of the day what I want to ensure is that we build as little as possible, but is > still correct. That if you were to build the set of targets returned, then build > all you don't get any compile errors. No doubt if dependencies are wrong that > won't happen, but all of this assumes dependencies are set up correctly. > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > test/analyzer/gyptest-analyzer.py:347: _CreateTestFile(['f.c'], []) > On 2014/08/15 21:30:24, scottmg wrote: > > comment here that we switched to test4.gyp > > Done.
On 2014/08/15 23:10:47, scottmg wrote: > On 2014/08/15 22:39:37, sky wrote: > > > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > > File pylib/gyp/generator/analyzer.py (right): > > > > > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > > pylib/gyp/generator/analyzer.py:13: These targets will be output if they > > contained one of the supplied file names. > > On 2014/08/15 21:30:24, scottmg wrote: > > > so, if there's a target in ignored_targets, and something in that target > > changes > > > it _will_ be output? > > > > Ya, sorry, this is all rather complex. Consider the All target in > build/all.gyp. > > It depends upon mojo.gyp:* . Lets say you modified a mojo/spy/spy.cc, which is > > in the target mojo.gyp:mojo_spy. Since All depends upon mojo.gyp:* it depends > > upon mojo.gyp:mojo_spy. And since mojo_spy has been modified this means we > need > > to compile both mojo_spy and the All target. The All target doesn't contain > any > > files, so we don't want to trigger compiling it. This is what the > ignore_targets > > are for. With All in ignore_targets All won't be added to affected_targets, > > unless it has a source that matched the set of files specified. > > Aha, got it. I can't think of a better name, but it does seem odd that > ignore_targets might not be ignored (if they actually have files in them that > change.) Will we actually use that or could we error if something that's not > like All is passed as an ignore? > > (Or, I guess not, since otherwise you wouldn't need the case where ignore > contains sources?) > > > > > I thought perhaps I could key off the build_type. That is, instead of > > ignore_targets key off a build_type of None. But I couldn't convince myself > that > > was right. Some build_types of None have actions, and it seems like if such a > > target depends upon a matched target then it needs to be built. > > Yeah, I don't think None is sufficient. > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > File pylib/gyp/generator/analyzer.py (right): > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > pylib/gyp/generator/analyzer.py:19: indirectly depend upon the set of paths > > supplied in files. This is not output > > On 2014/08/15 21:30:24, scottmg wrote: > > > "This is not output"? It looks like it is. > > > > Removed. > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > pylib/gyp/generator/analyzer.py:173: match_staus: one of the MatchStatus > values. > > On 2014/08/15 21:30:24, scottmg wrote: > > > match_staus -> match_status > > > > Done. > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > pylib/gyp/generator/analyzer.py:276: targets[target_name] = target > > On 2014/08/15 21:30:24, scottmg wrote: > > > this seems duplicated with code at 314, could one of them be removed? > > > > They are slightly different, but I can indeed share parts. Done. > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > pylib/gyp/generator/analyzer.py:455: # then when we resurse from the root > > targets we don't attempt to include > > On 2014/08/15 21:30:24, scottmg wrote: > > > resurse -> recurse > > > > Done. > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > pylib/gyp/generator/analyzer.py:456: # a Target that would be included by way > of > > another Targets depedendencies. > > On 2014/08/15 21:30:24, scottmg wrote: > > > dependencies > > > > Done. > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > pylib/gyp/generator/analyzer.py:470: ignore_plus_target = set(ignore_targets) > > On 2014/08/15 21:30:24, scottmg wrote: > > > sorry, i don't quite understand ignore_targets still. when do we use it? > > > > Sorry, I missed your earlier WTF. I responded to it. Hopefully it makes sense. > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > File test/analyzer/gyptest-analyzer.py (right): > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > test/analyzer/gyptest-analyzer.py:18: f = open('test_file', 'w') > > On 2014/08/15 21:30:24, scottmg wrote: > > > could you add a doc comment here? I find it confusing what's being created. > > > |files| and |targets| are both what's noted to have changed? Or files are > > what's > > > changed and we would like to build |targets|? > > > > Done. > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > test/analyzer/gyptest-analyzer.py:309: EnsureContains(matched=True, > > affected_targets={'d'}) > > On 2014/08/15 21:30:24, scottmg wrote: > > > could you explain this one? f.c changed, so 'f' is affected. why up only 1 > to > > > 'd'? > > > > > > all the targets are executable, does this one need to change if d is a > > > shared_library, i.e. a/b might need a relink in that case? > > > > My assumption, which could certainly be bogus, is that only the targets that > > depend upon matched targets need to be recompiled. This detection isn't about > > the actual running phase, just the minimal set of targets to compile. > > > > Lets simplify this to A depends on B and B depends on C. If some file in C > > changes, I certainly need to recompile C, and I need to recompile B because > > files in B depend upon C. But do I also need to recompile A? None of the files > > in A or its dependency B have changed. If A were an executable and I wanted to > > run A, then certainly I would need to compile A. This phase isn't about > running, > > just about compiling. On the chromium side if I cared about actually running A > I > > would pass in A as one of the targets, which would trigger A being returned > both > > as a target and affected_target because one its descendant dependencies was > > changed. What I'm trying to cover with affected_targets is only compiling > > targets that are not in the set of tests/executables but targets that need to > be > > recompiled because of a change. > > OK, I think I understand (maybe :). > > But you want to make sure that A still builds if something in C might affect it, > right? If A is an exe, and B and C are libs, with A containing "extern void > f()", and that's in C as "void f()", but then c.cc changes to rename/delete > that, won't we miss that A no longer links? We talked about this offline. I think it could happen, but we're not certain if it's a bug in the .gyp if A doesn't directly depend on C. A possible counter-example from chrome: chrome_main_dll (shared_library) depends on content. content is a static_library, and it depends on content_browser (also a static_library). So I think matches the A->B->C case. But there's no direct dependency from chrome on to content_browser. So if something in content_browser changes and is used by chrome, we might not notice that chrome doesn't link any more because we wouldn't bother to compile it? Does that seem like the same case (and/or is it actually broken)? I'm worried that there are more cases like this where a target is used as a "gatherer" target that pulls together a bunch of implementation targets, which would mean that going one level isn't sufficient. Maybe that could still be decreed a bug? But we'd probably want to have some tool that could detect this case then. I'm not sure how we could tell though. > > > Does that make sense? It could certainly be my logic is wrong here. At the end > > of the day what I want to ensure is that we build as little as possible, but > is > > still correct. That if you were to build the set of targets returned, then > build > > all you don't get any compile errors. No doubt if dependencies are wrong that > > won't happen, but all of this assumes dependencies are set up correctly. > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > test/analyzer/gyptest-analyzer.py:347: _CreateTestFile(['f.c'], []) > > On 2014/08/15 21:30:24, scottmg wrote: > > > comment here that we switched to test4.gyp > > > > Done.
On 2014/08/15 23:55:19, scottmg wrote: > On 2014/08/15 23:10:47, scottmg wrote: > > On 2014/08/15 22:39:37, sky wrote: > > > > > > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > > > File pylib/gyp/generator/analyzer.py (right): > > > > > > > > > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > > > pylib/gyp/generator/analyzer.py:13: These targets will be output if they > > > contained one of the supplied file names. > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > so, if there's a target in ignored_targets, and something in that target > > > changes > > > > it _will_ be output? > > > > > > Ya, sorry, this is all rather complex. Consider the All target in > > build/all.gyp. > > > It depends upon mojo.gyp:* . Lets say you modified a mojo/spy/spy.cc, which > is > > > in the target mojo.gyp:mojo_spy. Since All depends upon mojo.gyp:* it > depends > > > upon mojo.gyp:mojo_spy. And since mojo_spy has been modified this means we > > need > > > to compile both mojo_spy and the All target. The All target doesn't contain > > any > > > files, so we don't want to trigger compiling it. This is what the > > ignore_targets > > > are for. With All in ignore_targets All won't be added to affected_targets, > > > unless it has a source that matched the set of files specified. > > > > Aha, got it. I can't think of a better name, but it does seem odd that > > ignore_targets might not be ignored (if they actually have files in them that > > change.) Will we actually use that or could we error if something that's not > > like All is passed as an ignore? > > > > (Or, I guess not, since otherwise you wouldn't need the case where ignore > > contains sources?) > > > > > > > > I thought perhaps I could key off the build_type. That is, instead of > > > ignore_targets key off a build_type of None. But I couldn't convince myself > > that > > > was right. Some build_types of None have actions, and it seems like if such > a > > > target depends upon a matched target then it needs to be built. > > > > Yeah, I don't think None is sufficient. > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > File pylib/gyp/generator/analyzer.py (right): > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > pylib/gyp/generator/analyzer.py:19: indirectly depend upon the set of paths > > > supplied in files. This is not output > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > "This is not output"? It looks like it is. > > > > > > Removed. > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > pylib/gyp/generator/analyzer.py:173: match_staus: one of the MatchStatus > > values. > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > match_staus -> match_status > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > pylib/gyp/generator/analyzer.py:276: targets[target_name] = target > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > this seems duplicated with code at 314, could one of them be removed? > > > > > > They are slightly different, but I can indeed share parts. Done. > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > pylib/gyp/generator/analyzer.py:455: # then when we resurse from the root > > > targets we don't attempt to include > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > resurse -> recurse > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > pylib/gyp/generator/analyzer.py:456: # a Target that would be included by > way > > of > > > another Targets depedendencies. > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > dependencies > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > pylib/gyp/generator/analyzer.py:470: ignore_plus_target = > set(ignore_targets) > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > sorry, i don't quite understand ignore_targets still. when do we use it? > > > > > > Sorry, I missed your earlier WTF. I responded to it. Hopefully it makes > sense. > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > > File test/analyzer/gyptest-analyzer.py (right): > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > > test/analyzer/gyptest-analyzer.py:18: f = open('test_file', 'w') > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > could you add a doc comment here? I find it confusing what's being > created. > > > > |files| and |targets| are both what's noted to have changed? Or files are > > > what's > > > > changed and we would like to build |targets|? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > > test/analyzer/gyptest-analyzer.py:309: EnsureContains(matched=True, > > > affected_targets={'d'}) > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > could you explain this one? f.c changed, so 'f' is affected. why up only 1 > > to > > > > 'd'? > > > > > > > > all the targets are executable, does this one need to change if d is a > > > > shared_library, i.e. a/b might need a relink in that case? > > > > > > My assumption, which could certainly be bogus, is that only the targets that > > > depend upon matched targets need to be recompiled. This detection isn't > about > > > the actual running phase, just the minimal set of targets to compile. > > > > > > Lets simplify this to A depends on B and B depends on C. If some file in C > > > changes, I certainly need to recompile C, and I need to recompile B because > > > files in B depend upon C. But do I also need to recompile A? None of the > files > > > in A or its dependency B have changed. If A were an executable and I wanted > to > > > run A, then certainly I would need to compile A. This phase isn't about > > running, > > > just about compiling. On the chromium side if I cared about actually running > A > > I > > > would pass in A as one of the targets, which would trigger A being returned > > both > > > as a target and affected_target because one its descendant dependencies was > > > changed. What I'm trying to cover with affected_targets is only compiling > > > targets that are not in the set of tests/executables but targets that need > to > > be > > > recompiled because of a change. > > > > OK, I think I understand (maybe :). > > > > But you want to make sure that A still builds if something in C might affect > it, > > right? If A is an exe, and B and C are libs, with A containing "extern void > > f()", and that's in C as "void f()", but then c.cc changes to rename/delete > > that, won't we miss that A no longer links? > > We talked about this offline. I think it could happen, but we're not certain if > it's a bug in the .gyp if A doesn't directly depend on C. > > > A possible counter-example from chrome: > > chrome_main_dll (shared_library) depends on content. content is a > static_library, and it depends on content_browser (also a static_library). So I > think matches the A->B->C case. > > But there's no direct dependency from chrome on to content_browser. So if > something in content_browser changes and is used by chrome, we might not notice > that chrome doesn't link any more because we wouldn't bother to compile it? Does > that seem like the same case (and/or is it actually broken)? I'm worried that > there are more cases like this where a target is used as a "gatherer" target > that pulls together a bunch of implementation targets, which would mean that > going one level isn't sufficient. > > Maybe that could still be decreed a bug? But we'd probably want to have some > tool that could detect this case then. I'm not sure how we could tell though. Does the fact that we build release (i.e. non-component) and debug (component) for CQ jobs help in any way? i.e. for the specific example above, for non-component builds chrome.dll does depend on chrome_browser library. chrome_child.dll would depend on content_renderer.
On 2014/08/18 16:45:33, jam wrote: > On 2014/08/15 23:55:19, scottmg wrote: > > On 2014/08/15 23:10:47, scottmg wrote: > > > On 2014/08/15 22:39:37, sky wrote: > > > > > > > > > > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > > > > File pylib/gyp/generator/analyzer.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/20001/pylib/gyp/generator/anal... > > > > pylib/gyp/generator/analyzer.py:13: These targets will be output if they > > > > contained one of the supplied file names. > > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > > so, if there's a target in ignored_targets, and something in that target > > > > changes > > > > > it _will_ be output? > > > > > > > > Ya, sorry, this is all rather complex. Consider the All target in > > > build/all.gyp. > > > > It depends upon mojo.gyp:* . Lets say you modified a mojo/spy/spy.cc, > which > > is > > > > in the target mojo.gyp:mojo_spy. Since All depends upon mojo.gyp:* it > > depends > > > > upon mojo.gyp:mojo_spy. And since mojo_spy has been modified this means we > > > need > > > > to compile both mojo_spy and the All target. The All target doesn't > contain > > > any > > > > files, so we don't want to trigger compiling it. This is what the > > > ignore_targets > > > > are for. With All in ignore_targets All won't be added to > affected_targets, > > > > unless it has a source that matched the set of files specified. > > > > > > Aha, got it. I can't think of a better name, but it does seem odd that > > > ignore_targets might not be ignored (if they actually have files in them > that > > > change.) Will we actually use that or could we error if something that's not > > > like All is passed as an ignore? > > > > > > (Or, I guess not, since otherwise you wouldn't need the case where ignore > > > contains sources?) > > > > > > > > > > > I thought perhaps I could key off the build_type. That is, instead of > > > > ignore_targets key off a build_type of None. But I couldn't convince > myself > > > that > > > > was right. Some build_types of None have actions, and it seems like if > such > > a > > > > target depends upon a matched target then it needs to be built. > > > > > > Yeah, I don't think None is sufficient. > > > > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > > File pylib/gyp/generator/analyzer.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > > pylib/gyp/generator/analyzer.py:19: indirectly depend upon the set of > paths > > > > supplied in files. This is not output > > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > > "This is not output"? It looks like it is. > > > > > > > > Removed. > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > > pylib/gyp/generator/analyzer.py:173: match_staus: one of the MatchStatus > > > values. > > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > > match_staus -> match_status > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > > pylib/gyp/generator/analyzer.py:276: targets[target_name] = target > > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > > this seems duplicated with code at 314, could one of them be removed? > > > > > > > > They are slightly different, but I can indeed share parts. Done. > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > > pylib/gyp/generator/analyzer.py:455: # then when we resurse from the root > > > > targets we don't attempt to include > > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > > resurse -> recurse > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > > pylib/gyp/generator/analyzer.py:456: # a Target that would be included by > > way > > > of > > > > another Targets depedendencies. > > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > > dependencies > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/pylib/gyp/generator/anal... > > > > pylib/gyp/generator/analyzer.py:470: ignore_plus_target = > > set(ignore_targets) > > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > > sorry, i don't quite understand ignore_targets still. when do we use it? > > > > > > > > Sorry, I missed your earlier WTF. I responded to it. Hopefully it makes > > sense. > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > > > File test/analyzer/gyptest-analyzer.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > > > test/analyzer/gyptest-analyzer.py:18: f = open('test_file', 'w') > > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > > could you add a doc comment here? I find it confusing what's being > > created. > > > > > |files| and |targets| are both what's noted to have changed? Or files > are > > > > what's > > > > > changed and we would like to build |targets|? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/481433003/diff/40001/test/analyzer/gyptest-an... > > > > test/analyzer/gyptest-analyzer.py:309: EnsureContains(matched=True, > > > > affected_targets={'d'}) > > > > On 2014/08/15 21:30:24, scottmg wrote: > > > > > could you explain this one? f.c changed, so 'f' is affected. why up only > 1 > > > to > > > > > 'd'? > > > > > > > > > > all the targets are executable, does this one need to change if d is a > > > > > shared_library, i.e. a/b might need a relink in that case? > > > > > > > > My assumption, which could certainly be bogus, is that only the targets > that > > > > depend upon matched targets need to be recompiled. This detection isn't > > about > > > > the actual running phase, just the minimal set of targets to compile. > > > > > > > > Lets simplify this to A depends on B and B depends on C. If some file in C > > > > changes, I certainly need to recompile C, and I need to recompile B > because > > > > files in B depend upon C. But do I also need to recompile A? None of the > > files > > > > in A or its dependency B have changed. If A were an executable and I > wanted > > to > > > > run A, then certainly I would need to compile A. This phase isn't about > > > running, > > > > just about compiling. On the chromium side if I cared about actually > running > > A > > > I > > > > would pass in A as one of the targets, which would trigger A being > returned > > > both > > > > as a target and affected_target because one its descendant dependencies > was > > > > changed. What I'm trying to cover with affected_targets is only compiling > > > > targets that are not in the set of tests/executables but targets that need > > to > > > be > > > > recompiled because of a change. > > > > > > OK, I think I understand (maybe :). > > > > > > But you want to make sure that A still builds if something in C might affect > > it, > > > right? If A is an exe, and B and C are libs, with A containing "extern void > > > f()", and that's in C as "void f()", but then c.cc changes to rename/delete > > > that, won't we miss that A no longer links? > > > > We talked about this offline. I think it could happen, but we're not certain > if > > it's a bug in the .gyp if A doesn't directly depend on C. > > > > > > A possible counter-example from chrome: > > > > chrome_main_dll (shared_library) depends on content. content is a > > static_library, and it depends on content_browser (also a static_library). So > I > > think matches the A->B->C case. > > > > But there's no direct dependency from chrome on to content_browser. So if > > something in content_browser changes and is used by chrome, we might not > notice > > that chrome doesn't link any more because we wouldn't bother to compile it? > Does > > that seem like the same case (and/or is it actually broken)? I'm worried that > > there are more cases like this where a target is used as a "gatherer" target > > that pulls together a bunch of implementation targets, which would mean that > > going one level isn't sufficient. > > > > Maybe that could still be decreed a bug? But we'd probably want to have some > > tool that could detect this case then. I'm not sure how we could tell though. > > Does the fact that we build release (i.e. non-component) and debug (component) > for CQ jobs help in any way? > i.e. for the specific example above, for non-component builds chrome.dll does > depend on chrome_browser library. chrome_child.dll would depend on > content_renderer. I had an email exchange with Mark and worked out a different way to do this that shouldn't suffer from any of the problems outlined here. I'm in the process of updating the patch now.
I've changed this around considerably. The new approach is to include all targets that depend upon one of the targets that contains a file that changed that are of type other than 'none' or the 'none' type with actions/rules.
I'm not scared any more :) but does it actually help reduce compilations? e.g. in test3.gyp, if nothing under 'b' actually changes (i.e. only things in 'a' and 'c' change), then ninja should avoid doing anything expensive for any of those targets if we just build 'all'. If it's because we have "always-build" targets then maybe we just need to fix those instead? https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:263: (('actions' in target_dict and target_dict['actions']) or this can just be target_dict.get('actions') I think https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:264: ('rules' in target_dict and target_dict['rules'])) similar https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:454: def _WasIncludeFileModified(params, files): Could you make this _WasGypiFileModified (or _WasGypIncludeFileModified if you prefer) to avoid confusion with treatment of a .h? https://codereview.chromium.org/481433003/diff/100001/test/analyzer/gyptest-a... File test/analyzer/gyptest-analyzer.py (right): https://codereview.chromium.org/481433003/diff/100001/test/analyzer/gyptest-a... test/analyzer/gyptest-analyzer.py:293: # Assertions from test3.gyp. I found this helpful to try to understand if these were right: https://chart.googleapis.com/chart?chl=digraph+test3+%7B%0D%0Aall+-%3E+a%3B%0... Seems right to me, though sadly it seems like less benefit that before.
On 2014/08/18 19:48:16, scottmg wrote: > I'm not scared any more :) but does it actually help reduce compilations? It should certainly reduce the set of targets a particular patch needs to build. As John found link time is atrocious, so in theory fewer link targets should greatly help things. > e.g. in test3.gyp, if nothing under 'b' actually changes (i.e. only things in > 'a' and 'c' change), then ninja should avoid doing anything expensive for any of > those targets if we just build 'all'. Certainly. But the bots don't always start with a build that matches the source tree at the time the patch was compiled. In other words even though your patch didn't touch anything under 'b' someone else may have. > > If it's because we have "always-build" targets then maybe we just need to fix > those instead? > > > https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... > File pylib/gyp/generator/analyzer.py (right): > > https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... > pylib/gyp/generator/analyzer.py:263: (('actions' in target_dict and > target_dict['actions']) or > this can just be target_dict.get('actions') I think > > https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... > pylib/gyp/generator/analyzer.py:264: ('rules' in target_dict and > target_dict['rules'])) > similar > > https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... > pylib/gyp/generator/analyzer.py:454: def _WasIncludeFileModified(params, files): > Could you make this _WasGypiFileModified (or _WasGypIncludeFileModified if you > prefer) to avoid confusion with treatment of a .h? > > https://codereview.chromium.org/481433003/diff/100001/test/analyzer/gyptest-a... > File test/analyzer/gyptest-analyzer.py (right): > > https://codereview.chromium.org/481433003/diff/100001/test/analyzer/gyptest-a... > test/analyzer/gyptest-analyzer.py:293: # Assertions from test3.gyp. > I found this helpful to try to understand if these were right: > > https://chart.googleapis.com/chart?chl=digraph+test3+%7B%0D%0Aall+-%3E+a%3B%0... > > Seems right to me, though sadly it seems like less benefit that before. True, but I the previous approach assumed a perfect world, which we're pretty far from:(
https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:263: (('actions' in target_dict and target_dict['actions']) or On 2014/08/18 19:48:16, scottmg wrote: > this can just be target_dict.get('actions') I think Done. https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:264: ('rules' in target_dict and target_dict['rules'])) On 2014/08/18 19:48:16, scottmg wrote: > similar Done. https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:454: def _WasIncludeFileModified(params, files): On 2014/08/18 19:48:16, scottmg wrote: > Could you make this _WasGypiFileModified (or _WasGypIncludeFileModified if you > prefer) to avoid confusion with treatment of a .h? Done. https://codereview.chromium.org/481433003/diff/100001/test/analyzer/gyptest-a... File test/analyzer/gyptest-analyzer.py (right): https://codereview.chromium.org/481433003/diff/100001/test/analyzer/gyptest-a... test/analyzer/gyptest-analyzer.py:293: # Assertions from test3.gyp. On 2014/08/18 19:48:16, scottmg wrote: > I found this helpful to try to understand if these were right: > > https://chart.googleapis.com/chart?chl=digraph+test3+%7B%0D%0Aall+-%3E+a%3B%0... > > Seems right to me, though sadly it seems like less benefit that before. Very cool! I had to draw pictures.
On 2014/08/18 20:06:22, sky wrote: > On 2014/08/18 19:48:16, scottmg wrote: > > I'm not scared any more :) but does it actually help reduce compilations? > > It should certainly reduce the set of targets a particular patch needs to build. > As John found link time is atrocious, so in theory fewer link targets should > greatly help things. > > > e.g. in test3.gyp, if nothing under 'b' actually changes (i.e. only things in > > 'a' and 'c' change), then ninja should avoid doing anything expensive for any > of > > those targets if we just build 'all'. > > Certainly. But the bots don't always start with a build that matches the source > tree at the time the patch was compiled. In other words even though your patch > didn't touch anything under 'b' someone else may have. Ah, of course. LGTM. > > > > > If it's because we have "always-build" targets then maybe we just need to fix > > those instead? > > > > > > > https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... > > File pylib/gyp/generator/analyzer.py (right): > > > > > https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... > > pylib/gyp/generator/analyzer.py:263: (('actions' in target_dict and > > target_dict['actions']) or > > this can just be target_dict.get('actions') I think > > > > > https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... > > pylib/gyp/generator/analyzer.py:264: ('rules' in target_dict and > > target_dict['rules'])) > > similar > > > > > https://codereview.chromium.org/481433003/diff/100001/pylib/gyp/generator/ana... > > pylib/gyp/generator/analyzer.py:454: def _WasIncludeFileModified(params, > files): > > Could you make this _WasGypiFileModified (or _WasGypIncludeFileModified if you > > prefer) to avoid confusion with treatment of a .h? > > > > > https://codereview.chromium.org/481433003/diff/100001/test/analyzer/gyptest-a... > > File test/analyzer/gyptest-analyzer.py (right): > > > > > https://codereview.chromium.org/481433003/diff/100001/test/analyzer/gyptest-a... > > test/analyzer/gyptest-analyzer.py:293: # Assertions from test3.gyp. > > I found this helpful to try to understand if these were right: > > > > > https://chart.googleapis.com/chart?chl=digraph+test3+%7B%0D%0Aall+-%3E+a%3B%0... > > > > Seems right to me, though sadly it seems like less benefit that before. > > True, but I the previous approach assumed a perfect world, which we're pretty > far from:(
Message was sent while issue was closed.
Committed patchset #7 manually as r1966 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
