Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1431343002:
Changes semantics of analyzer (Closed)
DescriptionChanges semantics of analyzer
Analyzer now gets two inputs:
. test_targets: targets to search from.
. additional_compile_targets: additional targets to search from
for which we only care that the affected dependencies are
returned.
For output the same names are used:
. test_targets: any supplied test_targets that directly or indirectly
depend upon the change files.
. compile_targets: minimal set of targets to build. The targets
returned here are searched from the union or the supplied
test_targets and additional_compile_targets. The set of
targets output here are not necessarily values in either
the input test_targets or additional_compile_targets,
but they will be at least dependencies of those targets.
This brings back logic that was removed here:
https://codereview.chromium.org/1402813002 , with the following
changes:
. Previously what is now output as compile_targets would implicitly
include anything from the 'all' target. That is no longer the
case, if you want 'all' you have to specify it.
. There are now two sets of supplied targets. This is necessary to
handle cases where we want to take some sort of action if a target
directly or indirectly depends upon a change, e.g., run a test. We
want support for targets of type none here as well as just exes.
. Added support for 'all'.
I forked the old logic out into its own set of functions. These will
be removed once we update the code that consumes this.
TEST=covered by tests
BUG=552146
R=dpranke@chromium.org, mark@chromium.org
Committed: https://chromium.googlesource.com/external/gyp/+/70fa8bbeb2921c51b34598d3eda8e5fc485e952c
Patch Set 1 #Patch Set 2 : comments #
Total comments: 34
Patch Set 3 : feedback #Patch Set 4 : compile_targets -> additional_compile_targets #
Total comments: 11
Patch Set 5 : feedback #
Total comments: 2
Patch Set 6 : fix all modified output #
Messages
Total messages: 15 (2 generated)
https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:466: def _DoesTargetDependOn(target): This function and the next are exactly the same as was removed here: https://codereview.chromium.org/1402813002 . https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:496: def _AddCompileTargets(target, roots, add_if_no_ancestor, result): And this is _AddBuildTargets from before https://codereview.chromium.org/1402813002.
Lots of comments here. Hopefully you are able to follow them all. Much of this is stuff we've talked about this afternoon, but there are a couple points that I only realized later. In particular, I realized that the way the spec describes 'compile_targets' and 'test_targets' is kinda weird, and we might want to change it. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:13: compile_targets: Additional unqualified targets to search for. Targets in this "Additional" is perhaps a bit confusing in this context, since compile_targets might contain the exact same items as test_targets. However, see the comment below line 712; maybe we should actually change the input to call this 'additional_compile_targets' instead. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:22: of the supplied targets depends on. maybe note that the second clause only applies if the supplied target is of type 'none' and also depends on unaffected targets that we don't actually want to build? Don't know if you want to use the "meta" phrase here to match the analyze spec in MB or not. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:25: directly or indictly depend upon a file in |files|. This list if useful typo: s/indictly/indirectly . https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:32: invalid_targets: list of supplied targets thare were not found. typo: s/thare/that/ https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:242: # Needed until recipes is updated. Nit: "are updated" :). https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:320: is the set of targets built by the 'all' target. As we discussed, in a sense 'root Targets' is misleading here, because it's not actually the roots of the build graph. It would be good if there was some discussion somewhere about the difference between {set of targets built by 'all'} and {every actual root target}, maybe either here or at the top of the file near your example. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:468: set of paths. This updates |matches| of the Targets as it recurses. "the supplied set of paths" is unclear (to me) here. You're actually referring to the set of paths passed to analyze as a whole (the 'files' parameter), which is passed to _GenerateTargets(), right? Maybe say that explicitly? I was a bit confused because you're not actually supplying a set of paths to this specific function. Put differently _DoesTargetDependOn as a name suggests something that takes two parameters, i.e., does X depend on Y. But it's not clear what X and Y are in this function (the "supplied paths" that aren't really supplied to this function directly). Maybe call this _DoesTargetDependOnAffectedFiles() or something? I was particularly thrown because I initially expected this function and _GetTargetsDependingOn(possible_targets) to actually be looking for all of the targets that depend on possible_targets, not all of the targets that possible_targets are depending on that reach a matched target. I.e., you're traversing down toward the changed leaves, but I initially expected you to be traversing up towards the roots. I think this is also partially confusing because you do the traversals two different ways (downward from test_targets to the changed_targets, but upwards from changed_targets to compile_targets). Is there a reason to do the traversals in two different directions? (I probably would've done them both in the same direction, probably both upwards from the changed targets, but I might be biased because that's what GN makes easiest). https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:484: def _GetTargetsDependingOn(possible_targets): See comments above for _DoesTargetDependOn() (as relating to the name of this function, etc.) https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:541: roots: set of root targets in the build files to search from.""" calling this 'roots' is also a bit confusing, because this isn't necessarily the root targets in the build graph, it's just the list of targets you care about. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:549: def _WriteOutput(params, **values): Nit: it looks like you only ever call this like _WriteOutput(params, **some_dict). Maybe get rid of the ** and just pass the dict as-is? https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:645: 'targets': list(config.targets) } this is probably a nit, since this is the way the code was before, but shouldn't this also contain 'build_targets': list(config.targets) ? https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:707: General comments on the rest of this function: It's long and involved, longer than I personally like individual python functions to be. I would maybe change this to something like: all_targets, changed_targets, root_targets = _GenerateTargets(...) if not changed_targets: WriteOutput(params, test_targets=[], compile_targets=[], status=no_dependency_string} return test_targets = ComputeTestTargets(...) compile_targets = ComputeCompileTargets(...) WriteOutput(...) Also, it's confusing that 'test_targets' and 'test_target_names_contains_all' get updated over the course of the function. I'd probably prefer it if you created new variables for them. I suggest some possible changes below, but feel free to adapt as you prefer. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:708: all_targets, changed_targets, root_targets = _GenerateTargets( 'changed_targets' is the list of targets *directly* affected by the changed files (and doesn't not include the targets *indirectly* affected), right? In other words, using your example from the top of the file, it includes B but not A? Also, the above comments about the use of 'root' also apply here and below. I feel like changing 'root_targets' to 'dependencies_of_all' might make the code below clearer. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:712: supplied_target_names = config.compile_targets | config.test_targets We discussed this earlier (as to how this was safe to use supplied_target_names for determining compile_targets but not test_targets), but I've now realized something different as well. In the spec I've proposed: https://codereview.chromium.org/1412733015/diff/20001/tools/mb/docs/design_sp... the wording strongly suggests that you should only ever look at compile_targets to figure out which things to compile, and not look at test_targets at all. It also says that the caller should generally compute compile_targets = test_targets + additional_test_targets but it does not say that that's explicitly required. This could suggest that if we passed in { compile_targets: ['base_unittests'], test_targets: ['base_unittests', 'net_unittests'] } it would be *incorrect* to return 'net_unittests' in the 'compile_targets' field of the returned output. That's strange, but it would be even stranger if we did that and also returned 'net_unittests' in the 'test_targets' field of the returned output. That suggests to me that we should actually change the spec to pass in 'additional_compile_targets' instead of 'compile_targets', to make this clearer. WDYT? https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:723: # match, we remove it and replace it with 'all'. Same concerns about 'all' and 'root targets' here as in previous comments. There should also be some explanation as to why set(test_targets) might not be a strict subset of set(root_targets). https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:730: test_targets = [x for x in (set(test_targets) | set(root_targets))] Rewrite as test_targets = list(set(test_targets) | set(root_targets)) maybe? I think that's a bit clearer. I might also call this 'expanded_test_targets' to avoid having to keep original_test_targets around and changing the contents (and meaning) of test_targets. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:738: test_targets = _GetTargetsDependingOn(test_targets) maybe instead something like: affected_test_targets = _GetTargetsDependingOn(expanded_test_targets) https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:740: set(test_targets) & set(root_targets)) and then rewriting this as something like: affected_test_targets_should_contain_all = ( test_target_names_originally_contained_all and set(affected_test_targets) & set(dependencies_of_all) ) https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:745: set(original_test_targets))] affected_test_targets = list(set(affected_test_targets) & set(test_targets)) instead? Actually, maybe 'original_test_targets' is better here. Not sure. Also nit: you misspelled subsequently. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:761: supplied_targets = _LookupTargets(supplied_target_names, see comments above re: supplied_target_names being the union of compile_targets + test_targets. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:783: result_dict['invalid_targets'] = invalid_targets Looks like you *don't* return an error if there are invalid_targets. Should you?
Description was changed from ========== Changes semantics of analyzer Analyzer now gets two inputs: . compile_targets: list of targets to search from. . test_targets: additional targets to search from. For output the same names are used: . test_targets: any supplied test_targets that directly or indirectly depend upon the change files. . compile_targets: minimal set of targets to build. The targets returned here are searched from the union or the supplied compile_targets and test_targets. The set of targets output here are not necessarily values in compile_targets, they are reachable from compile_targets though. This brings back logic that was removed here: https://codereview.chromium.org/1402813002 , with the following changes: . Previously what is now output as compile_targets would implicitly include anything from the 'all' target. That is no longer the case, if you want 'all' you have to specify it. . There are now two sets of supplied targets. This is necessary to handle cases where we want to take some sort of action if a target directly or indirectly depends upon a change, e.g., run a test. We want support for targets of type none here as well as just exes. . Added support for 'all'. I forked the old logic out into its own set of functions. These will be removed once we update the code that consumes this. TEST=covered by tests BUG=552146 R=dpranke@chromium.org, mark@chromium.org ========== to ========== Changes semantics of analyzer Analyzer now gets two inputs: . compile_targets: list of targets to search from. . test_targets: additional targets to search from. For output the same names are used: . test_targets: any supplied test_targets that directly or indirectly depend upon the change files. . compile_targets: minimal set of targets to build. The targets returned here are searched from the union or the supplied compile_targets and test_targets. The set of targets output here are not necessarily values in compile_targets, they are reachable from compile_targets though. This brings back logic that was removed here: https://codereview.chromium.org/1402813002 , with the following changes: . Previously what is now output as compile_targets would implicitly include anything from the 'all' target. That is no longer the case, if you want 'all' you have to specify it. . There are now two sets of supplied targets. This is necessary to handle cases where we want to take some sort of action if a target directly or indirectly depends upon a change, e.g., run a test. We want support for targets of type none here as well as just exes. . Added support for 'all'. I forked the old logic out into its own set of functions. These will be removed once we update the code that consumes this. TEST=covered by tests BUG=552146 R=dpranke@chromium.org, mark@chromium.org ==========
Sana (and Paweł), please take a look at my comment #2, in particular the part at the beginning, since you've had concerns about the differences between 'compile_targets' and 'additional_compile_targets' as well.
New snapshot uploaded. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:13: compile_targets: Additional unqualified targets to search for. Targets in this On 2015/11/11 03:51:16, Dirk Pranke wrote: > "Additional" is perhaps a bit confusing in this context, since compile_targets > might contain the exact same items as test_targets. However, see the comment > below line 712; maybe we should actually change the input to call this > 'additional_compile_targets' instead. I thought we agreed on compile_targets as the name? I'm leaving it as compile_targets. I did change the wording though. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:22: of the supplied targets depends on. On 2015/11/11 03:51:16, Dirk Pranke wrote: > maybe note that the second clause only applies if the supplied target is of type > 'none' and also depends on unaffected targets that we don't actually want to > build? Don't know if you want to use the "meta" phrase here to match the analyze > spec in MB or not. Done. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:25: directly or indictly depend upon a file in |files|. This list if useful On 2015/11/11 03:51:16, Dirk Pranke wrote: > typo: s/indictly/indirectly . Done. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:32: invalid_targets: list of supplied targets thare were not found. On 2015/11/11 03:51:16, Dirk Pranke wrote: > typo: s/thare/that/ Done. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:242: # Needed until recipes is updated. On 2015/11/11 03:51:16, Dirk Pranke wrote: > Nit: "are updated" :). Done. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:320: is the set of targets built by the 'all' target. On 2015/11/11 03:51:16, Dirk Pranke wrote: > As we discussed, in a sense 'root Targets' is misleading here, because it's not > actually the roots of the build graph. Agreed. > It would be good if there was some discussion somewhere about the difference > between {set of targets built by 'all'} and {every actual root target}, maybe > either here or at the top of the file near your example. Added. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:468: set of paths. This updates |matches| of the Targets as it recurses. On 2015/11/11 03:51:16, Dirk Pranke wrote: > "the supplied set of paths" is unclear (to me) here. You're actually referring > to the set of paths passed to analyze as a whole (the 'files' parameter), which > is passed to _GenerateTargets(), right? Maybe say that explicitly? I was a bit > confused because you're not actually supplying a set of paths to this specific > function. > > Put differently _DoesTargetDependOn as a name suggests something that takes two > parameters, i.e., does X depend on Y. But it's not clear what X and Y are in > this function (the "supplied paths" that aren't really supplied to this function > directly). Maybe call this _DoesTargetDependOnAffectedFiles() or something? > > I was particularly thrown because I initially expected this function and > _GetTargetsDependingOn(possible_targets) to actually be looking for all of the > targets that depend on possible_targets, not all of the targets that > possible_targets are depending on that reach a matched target. I.e., you're > traversing down toward the changed leaves, but I initially expected you to be > traversing up towards the roots. > > I think this is also partially confusing because you do the traversals two > different ways (downward from test_targets to the changed_targets, but upwards > from changed_targets to compile_targets). > > Is there a reason to do the traversals in two different directions? (I probably > would've done them both in the same direction, probably both upwards from the > changed targets, but I might be biased because that's what GN makes easiest). I updated the docs and changed names. As to the direction. In thinking about this code a bit more I think it possible to change _AddCompileTargets to set something in Target that indicates it is impacted by the change so that the code in this function trivially checks a bit in Target. Since this code was working before, I was hoping to use it as is. I can optimize later on if we see value in it (meaning we think gyp is going to be around long enough that's it worth improving this code). https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:541: roots: set of root targets in the build files to search from.""" On 2015/11/11 03:51:16, Dirk Pranke wrote: > calling this 'roots' is also a bit confusing, because this isn't necessarily the > root targets in the build graph, it's just the list of targets you care about. Done. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:549: def _WriteOutput(params, **values): On 2015/11/11 03:51:16, Dirk Pranke wrote: > Nit: it looks like you only ever call this like _WriteOutput(params, > **some_dict). > > Maybe get rid of the ** and just pass the dict as-is? There is one place that doesn't. See the last line of the file. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:645: 'targets': list(config.targets) } On 2015/11/11 03:51:16, Dirk Pranke wrote: > this is probably a nit, since this is the way the code was before, but shouldn't > this also contain 'build_targets': list(config.targets) ? I believe the recipe code ignores the targets if all changed, so I don't think it's an issue. Given this code is going to go away, and the code was like this before it doesn't seem worth changing. https://codereview.chromium.org/1431343002/diff/20001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:707: On 2015/11/11 03:51:16, Dirk Pranke wrote: > General comments on the rest of this function: > > It's long and involved, longer than I personally like individual python > functions to be. > > I would maybe change this to something like: > > all_targets, changed_targets, root_targets = _GenerateTargets(...) > if not changed_targets: > WriteOutput(params, test_targets=[], compile_targets=[], > status=no_dependency_string} > return > > test_targets = ComputeTestTargets(...) > compile_targets = ComputeCompileTargets(...) > WriteOutput(...) > > Also, it's confusing that 'test_targets' and 'test_target_names_contains_all' > get updated over the course of the function. I'd probably prefer it if you > created new variables for them. I suggest some possible changes below, but feel > free to adapt as you prefer. I moved all of this off into a class. Hopefully it's more readable.
As discussed I renamed the input parameter compile_targets to additional_compile_targets.
Description was changed from ========== Changes semantics of analyzer Analyzer now gets two inputs: . compile_targets: list of targets to search from. . test_targets: additional targets to search from. For output the same names are used: . test_targets: any supplied test_targets that directly or indirectly depend upon the change files. . compile_targets: minimal set of targets to build. The targets returned here are searched from the union or the supplied compile_targets and test_targets. The set of targets output here are not necessarily values in compile_targets, they are reachable from compile_targets though. This brings back logic that was removed here: https://codereview.chromium.org/1402813002 , with the following changes: . Previously what is now output as compile_targets would implicitly include anything from the 'all' target. That is no longer the case, if you want 'all' you have to specify it. . There are now two sets of supplied targets. This is necessary to handle cases where we want to take some sort of action if a target directly or indirectly depends upon a change, e.g., run a test. We want support for targets of type none here as well as just exes. . Added support for 'all'. I forked the old logic out into its own set of functions. These will be removed once we update the code that consumes this. TEST=covered by tests BUG=552146 R=dpranke@chromium.org, mark@chromium.org ========== to ========== Changes semantics of analyzer Analyzer now gets two inputs: . test_targets: targets to search from. . additional_compile_targets: additional targets to search from for which we only care that the affected dependencies are returned. For output the same names are used: . test_targets: any supplied test_targets that directly or indirectly depend upon the change files. . compile_targets: minimal set of targets to build. The targets returned here are searched from the union or the supplied test_targets and additional_compile_targets. The set of targets output here are not necessarily values in either the input test_targets or additional_compile_targets, but they will be at least dependencies of those targets. This brings back logic that was removed here: https://codereview.chromium.org/1402813002 , with the following changes: . Previously what is now output as compile_targets would implicitly include anything from the 'all' target. That is no longer the case, if you want 'all' you have to specify it. . There are now two sets of supplied targets. This is necessary to handle cases where we want to take some sort of action if a target directly or indirectly depends upon a change, e.g., run a test. We want support for targets of type none here as well as just exes. . Added support for 'all'. I forked the old logic out into its own set of functions. These will be removed once we update the code that consumes this. TEST=covered by tests BUG=552146 R=dpranke@chromium.org, mark@chromium.org ==========
lgtm. I updated the CL description a bit to match the latest set of changes. Feel free to edit back as you like. The comments below are just nits and questions for clarification. https://codereview.chromium.org/1431343002/diff/60001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/1431343002/diff/60001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:58: on "b2" and gyp is supplied "a.gyp" then "all" cosists of "a1" and "a2". Notice nit: s/cosists/consists/ https://codereview.chromium.org/1431343002/diff/60001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:349: # Set of Targets in |build_files|. this is actually the set of all targets in build_files and their dependencies, right? https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/gyptest-a... File test/analyzer/gyptest-analyzer-deprecated.py (right): https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/gyptest-a... test/analyzer/gyptest-analyzer-deprecated.py:167: _CreateConfigFile(['exe2.c'], ['bad_target', 'allx', 'exe2']) why does this change to 'allx' ? https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/gyptest-a... File test/analyzer/gyptest-analyzer.py (right): https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/gyptest-a... test/analyzer/gyptest-analyzer.py:17: def _CreateConfigFile(files, compile_targets, test_targets=[]): nit: call the arg 'additional_compile_targets' ? https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/test.gyp File test/analyzer/test.gyp (right): https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/test.gyp#... test/analyzer/test.gyp:106: 'target_name': 'allx', this changes to 'allx' to avoid the special handling of 'all' ?
https://codereview.chromium.org/1431343002/diff/60001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/1431343002/diff/60001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:58: on "b2" and gyp is supplied "a.gyp" then "all" cosists of "a1" and "a2". Notice On 2015/11/12 00:37:20, Dirk Pranke wrote: > nit: s/cosists/consists/ Done. https://codereview.chromium.org/1431343002/diff/60001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:349: # Set of Targets in |build_files|. On 2015/11/12 00:37:20, Dirk Pranke wrote: > this is actually the set of all targets in build_files and their dependencies, > right? No, just the set of targets in the build files. The set is added to on 376 only. https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/gyptest-a... File test/analyzer/gyptest-analyzer-deprecated.py (right): https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/gyptest-a... test/analyzer/gyptest-analyzer-deprecated.py:167: _CreateConfigFile(['exe2.c'], ['bad_target', 'allx', 'exe2']) On 2015/11/12 00:37:20, Dirk Pranke wrote: > why does this change to 'allx' ? You found out in test/analyzer/test.gyp . I renamed the target to not conflict with 'all'. I didn't duplicate all the gyp files, so I had to rename here. https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/gyptest-a... File test/analyzer/gyptest-analyzer.py (right): https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/gyptest-a... test/analyzer/gyptest-analyzer.py:17: def _CreateConfigFile(files, compile_targets, test_targets=[]): On 2015/11/12 00:37:20, Dirk Pranke wrote: > nit: call the arg 'additional_compile_targets' ? Done. https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/test.gyp File test/analyzer/test.gyp (right): https://codereview.chromium.org/1431343002/diff/60001/test/analyzer/test.gyp#... test/analyzer/test.gyp:106: 'target_name': 'allx', On 2015/11/12 00:37:20, Dirk Pranke wrote: > this changes to 'allx' to avoid the special handling of 'all' ? Exactly.
Mark, did you want to review this patch?
https://codereview.chromium.org/1431343002/diff/60001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/1431343002/diff/60001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:349: # Set of Targets in |build_files|. On 2015/11/12 00:47:23, sky wrote: > On 2015/11/12 00:37:20, Dirk Pranke wrote: > > this is actually the set of all targets in build_files and their dependencies, > > right? > > No, just the set of targets in the build files. The set is added to on 376 only. Acknowledged.
Actually, one possible bug ... https://codereview.chromium.org/1431343002/diff/80001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/1431343002/diff/80001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:810: config.additional_compile_target_names) } Hm. Should we return the union of test_target_names and additional_compile_target_names here?
I’m a little swamped and this is a bit long, so I’ll trust Dirk’s review on this one.
https://codereview.chromium.org/1431343002/diff/80001/pylib/gyp/generator/ana... File pylib/gyp/generator/analyzer.py (right): https://codereview.chromium.org/1431343002/diff/80001/pylib/gyp/generator/ana... pylib/gyp/generator/analyzer.py:810: config.additional_compile_target_names) } On 2015/11/12 03:29:04, Dirk Pranke wrote: > Hm. Should we return the union of test_target_names and > additional_compile_target_names here? Done.
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 70fa8bbeb2921c51b34598d3eda8e5fc485e952c (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
