Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265833002:
Implement `gn analyze`. (Closed)
|
Created:
4 years, 4 months ago by Dirk Pranke Modified:
4 years, 3 months ago Reviewers:
brettw CC:
chromium-reviews, Dirk Pranke, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement `gn analyze`.
This CL implements `analyze` natively in GN, fixing some
long-standing issues that the implementation in MB had
due to its not having access to the full build graph.
In particular, `analyze` will now work correctly when given
"all" as a compile target, and the list of compile targets
will now be pruned/filtered correctly.
Note that the input to analyze is different from the input that
MB expects, in that GN expects lists of GN labels and
source-absolute paths to files, rather than lists of ninja
targets and relative paths. MB will be responsible for doing
the conversion until we have updated the build recipes to pass
the format that GN expects.
R=brettw@chromium.org
BUG=555273
Committed: https://crrev.com/6d8fdaabe1a8faed9a5573047a2cebdd8456a996
Cr-Commit-Position: refs/heads/master@{#415050}
Patch Set 1 #
Total comments: 1
Patch Set 2 : sample tests #
Total comments: 41
Patch Set 3 : update with review feedback, rename graph -> analyzer, add proper unit tests #
Total comments: 24
Patch Set 4 : update w/ 2nd round of review feedback #Patch Set 5 : add IsPathSourceAbsolute() to filesystem_utils and allow absolute paths in analyze also #Patch Set 6 : fix win compile err #Patch Set 7 : do not pretty print the written json; this gets around crlf issues #
Messages
Total messages: 37 (20 generated)
Mostly minor comments, this looks good from a high level. https://codereview.chromium.org/2265833002/diff/1/tools/gn/graph.cc File tools/gn/graph.cc (right): https://codereview.chromium.org/2265833002/diff/1/tools/gn/graph.cc#newcode1 tools/gn/graph.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) https://codereview.chromium.org/2265833002/diff/20001/tools/gn/gn.gyp File tools/gn/gn.gyp (right): https://codereview.chromium.org/2265833002/diff/20001/tools/gn/gn.gyp#newcode91 tools/gn/gn.gyp:91: 'graph.cc', Let's rename this graph_query or graph_analysis or something to make clear this is an optional thing you construct at the end rather than something that actually hooks up the build graph. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc File tools/gn/graph.cc (right): https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:24: Graph::Graph(const TargetVector& all_targets, const Label& default_toolchain) I think this can just take a "const Builder&" parameter (the builder has a pointer to the loader). https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:39: TargetSet Graph::Prune(const TargetSet& targets) { This should be down below according to header file order. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:140: for (DepMap::const_iterator cur_dep = dep_begin; cur_dep != dep_end; Is it possible to use auto for the type here? https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:147: using Inputs = struct Inputs { Remove these usings as discussed in person. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:157: using Outputs = struct Outputs { Ditto https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:197: Err GetStringVector(const base::DictionaryValue& dict, Generally the GN code uses Err as an out param and returns the thing computed. Can this return the vector and have Err* as the last param? https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:202: if (!ret) Use {} since contents are multi-line (Chrome style). https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:210: if (!ret) {} as above. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:213: strings->push_back(s); I think if you do: ...push_back(std::move(s)); it will avoid a string copy (just OCD copy avoidance). https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:237: Label StringToLabel(const Label& default_toolchain, const std::string path) { 2nd param should be a ref. Alternatively, you could pass by non-const value and use std::move to take the value out and pass it into "v" below (this will save a copy if the caller can pass it in without a copy). https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:238: SourceDir source_root("//"); The commands ("ls", "refs" and friends) all take inputs relative to the current directory if they're not absolute. I suspect this function is used as the backend for similar-type input, so it should follow the same rule. Maybe we should pass the current directory into the constructor? https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:255: if (value == nullptr) I think "if (!value)" should work (unique_ptr provides an implicit conversion to bool for this purpose). https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:267: inputs->source_vec.push_back(SourceFile(s)); This assumes the string "s" is ab absolute file path. This assumption is probably OK, but SourceFile will assert if it's not, so you should probably check for this and return a proper error. Alternatively, you could resolve relative to the current directory, but that seems confusing in the context of JSON input. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:277: else { No {} (style guide says all of the arms should have {} or none of them should. In this case you don't need them. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:358: for (auto& root : graph.roots()) { No {} for consistency. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h File tools/gn/graph.h (right): https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode16 tools/gn/graph.h:16: using DepMap = std::multimap<const Target*, const Target*>; These shouldn't be added to the global namespace. I'd put them inside the class right after "public". https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode17 tools/gn/graph.h:17: using LabelSet = std::set<const Label*>; This should be std::set<Label> since labels have no particular identity and can be efficiently copied. I don't really know who manages the storage for these pointers either. The rest of the pointers are OK since those are big things whose lifetimes are managed by somebody else. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode26 tools/gn/graph.h:26: class Graph { Class name should match file name suggestion from above. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode70 tools/gn/graph.h:70: void PruneTarget(const Target*, TargetSet& seen, TargetSet& pruned); Don't pass non-const refs. The style guide says these should be pointers since they're really out params. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode92 tools/gn/graph.h:92: Err Analyze(Graph& graph, const std::string& input, std::string* output); Having "graph" be a non-const ref is against the style guide. I'd say it should be const but I assume this is what you're complaining about with the caching. Is there any reason this can't be a member of Grap?
https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc File tools/gn/graph.cc (right): https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:24: Graph::Graph(const TargetVector& all_targets, const Label& default_toolchain) On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > I think this can just take a "const Builder&" parameter (the builder has a > pointer to the loader). This was me thinking in a principle-of-least-authority sort of way, plus I thought it'd make it easier for testing. However, it turns out that in order to write decent tests it's easiest to use a builder + a fake loader). https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:39: TargetSet Graph::Prune(const TargetSet& targets) { On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > This should be down below according to header file order. Good catch. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:140: for (DepMap::const_iterator cur_dep = dep_begin; cur_dep != dep_end; On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > Is it possible to use auto for the type here? Possibly; will try. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:147: using Inputs = struct Inputs { On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > Remove these usings as discussed in person. Done. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:157: using Outputs = struct Outputs { On 2016/08/22 21:08:26, brettw (ping on IM after 24h) wrote: > Ditto Done. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:197: Err GetStringVector(const base::DictionaryValue& dict, On 2016/08/22 21:08:26, brettw (ping on IM after 24h) wrote: > Generally the GN code uses Err as an out param and returns the thing computed. > Can this return the vector and have Err* as the last param? Oh, I actually thought the convention was the opposite from the subset of the code I had looked at. Will change. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:202: if (!ret) On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > Use {} since contents are multi-line (Chrome style). Right, good catch. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:210: if (!ret) On 2016/08/22 21:08:26, brettw (ping on IM after 24h) wrote: > {} as above. Acknowledged. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:213: strings->push_back(s); On 2016/08/22 21:08:26, brettw (ping on IM after 24h) wrote: > I think if you do: > ...push_back(std::move(s)); > it will avoid a string copy (just OCD copy avoidance). Acknowledged. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:237: Label StringToLabel(const Label& default_toolchain, const std::string path) { On 2016/08/22 21:08:26, brettw (ping on IM after 24h) wrote: > 2nd param should be a ref. Alternatively, you could pass by non-const value and > use std::move to take the value out and pass it into "v" below (this will save a > copy if the caller can pass it in without a copy). Acknowledged. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:238: SourceDir source_root("//"); On 2016/08/22 21:08:26, brettw (ping on IM after 24h) wrote: > The commands ("ls", "refs" and friends) all take inputs relative to the current > directory if they're not absolute. I suspect this function is used as the > backend for similar-type input, so it should follow the same rule. Maybe we > should pass the current directory into the constructor? Ok. I think that's probably a good idea for this function generically, but I think we should still require analyze to only take absolute paths as inputs in the JSON file, given that it's such a constrained function (one controlled user). https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:255: if (value == nullptr) On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > I think "if (!value)" should work (unique_ptr provides an implicit conversion to > bool for this purpose). ok. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:267: inputs->source_vec.push_back(SourceFile(s)); On 2016/08/22 21:08:26, brettw (ping on IM after 24h) wrote: > This assumes the string "s" is ab absolute file path. This assumption is > probably OK, but SourceFile will assert if it's not, so you should probably > check for this and return a proper error. Alternatively, you could resolve > relative to the current directory, but that seems confusing in the context of > JSON input. Good idea. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:277: else { On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > No {} (style guide says all of the arms should have {} or none of them should. > In this case you don't need them. Whoops, yes. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.cc#newco... tools/gn/graph.cc:358: for (auto& root : graph.roots()) { On 2016/08/22 21:08:26, brettw (ping on IM after 24h) wrote: > No {} for consistency. Acknowledged. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h File tools/gn/graph.h (right): https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode16 tools/gn/graph.h:16: using DepMap = std::multimap<const Target*, const Target*>; On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > These shouldn't be added to the global namespace. I'd put them inside the class > right after "public". Good catch, agreed. Some of the names are used in the free functions in the .cc file, so I'll probably need to play with this a bit. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode17 tools/gn/graph.h:17: using LabelSet = std::set<const Label*>; On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > This should be std::set<Label> since labels have no particular identity and can > be efficiently copied. I don't really know who manages the storage for these > pointers either. The rest of the pointers are OK since those are big things > whose lifetimes are managed by somebody else. Ack. My thinking was simply that "someone else" needed to owns the elements, but a vector of values is better. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode26 tools/gn/graph.h:26: class Graph { On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > Class name should match file name suggestion from above. Acknowledged. https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode70 tools/gn/graph.h:70: void PruneTarget(const Target*, TargetSet& seen, TargetSet& pruned); On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > Don't pass non-const refs. The style guide says these should be pointers since > they're really out params. Good catch. (They're in-out parameters, but, yeah, mutated). https://codereview.chromium.org/2265833002/diff/20001/tools/gn/graph.h#newcode92 tools/gn/graph.h:92: Err Analyze(Graph& graph, const std::string& input, std::string* output); On 2016/08/22 21:08:27, brettw (ping on IM after 24h) wrote: > Having "graph" be a non-const ref is against the style guide. I'd say it should > be const but I assume this is what you're complaining about with the caching. Is > there any reason this can't be a member of Grap? No particular reason apart from keeping the class minimal (the implementation of analyze can and should be done using only the public members), but perhaps that's not a very strong reason. Yes, it's not const because of the caches.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Okay, everything has been updated (and addressed), I think. Please take another look? Also, let me know if you want to see more tests, this was just a fairly basic pass over the key cases.
Description was changed from ========== Implement `gn analyze`, take 3. R=brettw@chromium.org BUG=555273 ========== to ========== Implement `gn analyze`. This CL implements `analyze` natively in GN, fixing some long-standing issues that the implementation in MB had due to its not having access to the full build graph. In particular, `analyze` will now work correctly when given "all" as a compile target, and the list of compile targets will now be pruned/filtered correctly. Note that the input to analyze is different from the input that MB expects, in that GN expects lists of GN labels and source-absolute paths to files, rather than lists of ninja targets and relative paths. MB will be responsible for doing the conversion until we have updated the build recipes to pass the format that GN expects. R=brettw@chromium.org BUG=555273 ==========
Description was changed from ========== Implement `gn analyze`. This CL implements `analyze` natively in GN, fixing some long-standing issues that the implementation in MB had due to its not having access to the full build graph. In particular, `analyze` will now work correctly when given "all" as a compile target, and the list of compile targets will now be pruned/filtered correctly. Note that the input to analyze is different from the input that MB expects, in that GN expects lists of GN labels and source-absolute paths to files, rather than lists of ninja targets and relative paths. MB will be responsible for doing the conversion until we have updated the build recipes to pass the format that GN expects. R=brettw@chromium.org BUG=555273 ========== to ========== Implement `gn analyze`. This CL implements `analyze` natively in GN, fixing some long-standing issues that the implementation in MB had due to its not having access to the full build graph. In particular, `analyze` will now work correctly when given "all" as a compile target, and the list of compile targets will now be pruned/filtered correctly. Note that the input to analyze is different from the input that MB expects, in that GN expects lists of GN labels and source-absolute paths to files, rather than lists of ninja targets and relative paths. MB will be responsible for doing the conversion until we have updated the build recipes to pass the format that GN expects. R=brettw@chromium.org BUG=555273 ==========
lgtm https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc File tools/gn/analyzer.cc (right): https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:119: bool IsSourceAbsolute(const std::string& path) { I think this should allow filesystem absolute paths as well. I know some people use targets with these names and we handle these consistently in other contexts. I think the only thing you want to prevent here is relative paths. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:123: Label SourceAbsolutePathToLabel(const Label& default_toolchain, You're really taking label strings as inputs rather than paths to things, which makes me find this name confusing. How about SourceAbsoluteStringToLabel? https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:132: Value v(nullptr, Value::STRING); These two lines can be: Value v(nullptr, path); https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:239: std::string Analyzer::Analyze(const std::string& input, Err* err) { I can live with what you wrote but I would have personally made this function take an AnalyzeInputs struct (move the anon-namespace one to the class) and return an AnalyzeOutputs struct and leave the JSON serialization to the very end points in command_analyze.cc. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:407: DepMap::const_iterator dep_end = dep_map_.upper_bound(target); I would have used "auto dep_begin" and "auto dep_end" for these decls. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h File tools/gn/analyzer.h (right): https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h#ne... tools/gn/analyzer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h#ne... tools/gn/analyzer.h:27: Analyzer(const Builder& builder); Prepend "explicit" since this is a one-arg constructor. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h#ne... tools/gn/analyzer.h:35: std::string Analyze(const std::string& input, Err* err); I don't see any caching in this object any more, so I think all the functions on this object can be marked "const". https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h#ne... tools/gn/analyzer.h:75: TargetSet Filter(const TargetSet& targets); Would "CollapseGroups" or "FlattenGroups" be a better name for this function? https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer_unit... File tools/gn/analyzer_unittest.cc (right): https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer_unit... tools/gn/analyzer_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016
https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc File tools/gn/analyzer.cc (right): https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/08/25 21:22:30, brettw (ping on IM after 24h) wrote: > No (c) Done. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:119: bool IsSourceAbsolute(const std::string& path) { On 2016/08/25 21:22:29, brettw (ping on IM after 24h) wrote: > I think this should allow filesystem absolute paths as well. I know some people > use targets with these names and we handle these consistently in other contexts. > I think the only thing you want to prevent here is relative paths. I don't know where we use absolute paths, and doing so seems like it would be a bad thing. I'll defer this discussion to a follow-up thing so we can land something, but we should discuss this further. At the very least, if we wanted to allow non-source-absolute paths, I'd want to change the function name :). As an aside, this is simply duplicating the logic in filesystem_utils.h. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:123: Label SourceAbsolutePathToLabel(const Label& default_toolchain, On 2016/08/25 21:22:29, brettw (ping on IM after 24h) wrote: > You're really taking label strings as inputs rather than paths to things, which > makes me find this name confusing. How about SourceAbsoluteStringToLabel? Fair enough. Will change. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:132: Value v(nullptr, Value::STRING); On 2016/08/25 21:22:30, brettw (ping on IM after 24h) wrote: > These two lines can be: > Value v(nullptr, path); Done. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:239: std::string Analyzer::Analyze(const std::string& input, Err* err) { On 2016/08/25 21:22:29, brettw (ping on IM after 24h) wrote: > I can live with what you wrote but I would have personally made this function > take an AnalyzeInputs struct (move the anon-namespace one to the class) and > return an AnalyzeOutputs struct and leave the JSON serialization to the very end > points in command_analyze.cc. Yeah, I struggled with this tradeoff and the code was actually in command_analyze.cc for most of the lifetime of this patch. I ended up choosing this route because it's much easier to write tests with strings as input and output, testing the conversion is reasonably important, and we don't necessarily have a good way to test stuff in the command_* files. The other advantage is that the Input and Output structures are very much an implementation detail of the actual analyze() routine and it doesn't make a lot of sense to expose it through the interface. If we fix the testing problem I'd be happy to move stuff around. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:407: DepMap::const_iterator dep_end = dep_map_.upper_bound(target); On 2016/08/25 21:22:29, brettw (ping on IM after 24h) wrote: > I would have used "auto dep_begin" and "auto dep_end" for these decls. Done. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h File tools/gn/analyzer.h (right): https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h#ne... tools/gn/analyzer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/08/25 21:22:30, brettw (ping on IM after 24h) wrote: > No (c) Done. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h#ne... tools/gn/analyzer.h:27: Analyzer(const Builder& builder); On 2016/08/25 21:22:30, brettw (ping on IM after 24h) wrote: > Prepend "explicit" since this is a one-arg constructor. Done. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h#ne... tools/gn/analyzer.h:35: std::string Analyze(const std::string& input, Err* err); On 2016/08/25 21:22:30, brettw (ping on IM after 24h) wrote: > I don't see any caching in this object any more, so I think all the functions on > this object can be marked "const". Yup, I got rid it (or, rather, pushed it into the constructor). Will do. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.h#ne... tools/gn/analyzer.h:75: TargetSet Filter(const TargetSet& targets); On 2016/08/25 21:22:30, brettw (ping on IM after 24h) wrote: > Would "CollapseGroups" or "FlattenGroups" be a better name for this function? No, I don't think those are better. If the concern is that "Filter" is too generic, I'd probably switch it back to "Prune". However, I'll leave it as "Filter" for now and we can change in a follow-up CL if need be.
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2265833002/#ps140001 (title: "update w/ 2nd round of review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc File tools/gn/analyzer.cc (right): https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:119: bool IsSourceAbsolute(const std::string& path) { Most of our support for stuff outside of the source root was added by Opera. This function is correct for something called IsSourceAbsolute, but I think we just should have called IsPathAbsolute in filesystem utils and deleted this one.
still lgtm https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc File tools/gn/analyzer.cc (right): https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:119: bool IsSourceAbsolute(const std::string& path) { Most of our support for stuff outside of the source root was added by Opera. This function is correct for something called IsSourceAbsolute, but I think we just should have called IsPathAbsolute in filesystem utils and deleted this one.
The CQ bit was unchecked by dpranke@chromium.org
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2265833002/#ps160001 (title: "add IsPathSourceAbsolute() to filesystem_utils and allow absolute paths in analyze also")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Description was changed from ========== Implement `gn analyze`. This CL implements `analyze` natively in GN, fixing some long-standing issues that the implementation in MB had due to its not having access to the full build graph. In particular, `analyze` will now work correctly when given "all" as a compile target, and the list of compile targets will now be pruned/filtered correctly. Note that the input to analyze is different from the input that MB expects, in that GN expects lists of GN labels and source-absolute paths to files, rather than lists of ninja targets and relative paths. MB will be responsible for doing the conversion until we have updated the build recipes to pass the format that GN expects. R=brettw@chromium.org BUG=555273 ========== to ========== Implement `gn analyze`. This CL implements `analyze` natively in GN, fixing some long-standing issues that the implementation in MB has due to its not having access to the full build graph. In particular, `analyze` will now work correctly when given "all" as a compile target, and the list of compile targets will now be pruned/filtered correctly. Note that the input to analyze is different from the input that MB expects, in that GN expects lists of GN labels and source-absolute paths to files, rather than lists of ninja targets and relative paths. MB will be responsible for doing the conversion until we have updated the build recipes to pass the format that GN expects. R=brettw@chromium.org BUG=555273 ==========
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2265833002/#ps180001 (title: "fix win compile err")
https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc File tools/gn/analyzer.cc (right): https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer.cc#n... tools/gn/analyzer.cc:119: bool IsSourceAbsolute(const std::string& path) { On 2016/08/26 17:09:19, brettw (ping on IM after 24h) wrote: > Most of our support for stuff outside of the source root was added by Opera. > > This function is correct for something called IsSourceAbsolute, but I think we > just should have called IsPathAbsolute in filesystem utils and deleted this one. It's true that we support absolute paths in some situations, but it's not clear to me that they would come up in this one. However, I can probably change the code to allow them without creating too much risk. https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer_unit... File tools/gn/analyzer_unittest.cc (right): https://codereview.chromium.org/2265833002/diff/120001/tools/gn/analyzer_unit... tools/gn/analyzer_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/08/25 21:22:30, brettw (ping on IM after 24h) wrote: > 2016 Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2265833002/#ps200001 (title: "do not pretty print the written json; this gets around crlf issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement `gn analyze`. This CL implements `analyze` natively in GN, fixing some long-standing issues that the implementation in MB had due to its not having access to the full build graph. In particular, `analyze` will now work correctly when given "all" as a compile target, and the list of compile targets will now be pruned/filtered correctly. Note that the input to analyze is different from the input that MB expects, in that GN expects lists of GN labels and source-absolute paths to files, rather than lists of ninja targets and relative paths. MB will be responsible for doing the conversion until we have updated the build recipes to pass the format that GN expects. R=brettw@chromium.org BUG=555273 ========== to ========== Implement `gn analyze`. This CL implements `analyze` natively in GN, fixing some long-standing issues that the implementation in MB had due to its not having access to the full build graph. In particular, `analyze` will now work correctly when given "all" as a compile target, and the list of compile targets will now be pruned/filtered correctly. Note that the input to analyze is different from the input that MB expects, in that GN expects lists of GN labels and source-absolute paths to files, rather than lists of ninja targets and relative paths. MB will be responsible for doing the conversion until we have updated the build recipes to pass the format that GN expects. R=brettw@chromium.org BUG=555273 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Implement `gn analyze`. This CL implements `analyze` natively in GN, fixing some long-standing issues that the implementation in MB had due to its not having access to the full build graph. In particular, `analyze` will now work correctly when given "all" as a compile target, and the list of compile targets will now be pruned/filtered correctly. Note that the input to analyze is different from the input that MB expects, in that GN expects lists of GN labels and source-absolute paths to files, rather than lists of ninja targets and relative paths. MB will be responsible for doing the conversion until we have updated the build recipes to pass the format that GN expects. R=brettw@chromium.org BUG=555273 ========== to ========== Implement `gn analyze`. This CL implements `analyze` natively in GN, fixing some long-standing issues that the implementation in MB had due to its not having access to the full build graph. In particular, `analyze` will now work correctly when given "all" as a compile target, and the list of compile targets will now be pruned/filtered correctly. Note that the input to analyze is different from the input that MB expects, in that GN expects lists of GN labels and source-absolute paths to files, rather than lists of ninja targets and relative paths. MB will be responsible for doing the conversion until we have updated the build recipes to pass the format that GN expects. R=brettw@chromium.org BUG=555273 Committed: https://crrev.com/6d8fdaabe1a8faed9a5573047a2cebdd8456a996 Cr-Commit-Position: refs/heads/master@{#415050} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6d8fdaabe1a8faed9a5573047a2cebdd8456a996 Cr-Commit-Position: refs/heads/master@{#415050} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
