Limit the set of Visual Studio projects generated by GN

Limit the set of Visual Studio projects generated by GN

BUG=589099
Committed: https://crrev.com/014e81d144453d840e09eb9695b0dee262bc3507
Cr-Commit-Position: refs/heads/master@{#377871}

[Tomasz Moniuszko, 2016-02-23 13:49:26]

tmoniuszko@opera.com changed reviewers:
+ brettw@chromium.org

[brettw, 2016-02-24 22:11:26]

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/command_gen.cc
File tools/gn/command_gen.cc (right):

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/command_gen.cc...
tools/gn/command_gen.cc:217: "GN optionally generates files for IDEs.
Possibilities for <ide options>\n"
Can you do this instead:
"""
IDE options

  GN optionally generates files for IDEs. Possibilities for <ide options>\n
  ...
"""

(so "IDE options" is the title, the string you have not is the text under it,
indented, followed by the options).

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/command_gen.cc...
tools/gn/command_gen.cc:228: "      Override default sln file name (\"all\").
Only for Visual Studio.\n"
Can you explain where this file will be put (I assume in the root build
directory)?

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/command_gen.cc...
tools/gn/command_gen.cc:231: "      Semicolon-separated list of path prefixes
used to limit the set of\n"
Assuming we do the label pattern thing I suggested in the other file, this
should reference "gn help label_pattern"

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/visual_studio_...
File tools/gn/visual_studio_writer.cc (right):

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/visual_studio_...
tools/gn/visual_studio_writer.cc:239: (filter[0] == '/' &&
This seems complicated and isn't something we do anywhere else. My idea is to do
this the same way we handle the build dirs (which allow you to specify
"//foo/bar" GN paths if you want): it will also resolve paths relative to the
current directory. You can see in Setup::FillBuildDir how this works.

Also, GN has this "LabelPattern" concept that is used on various places. I think
it would be advantageous to use this rather than rolling custom prefix matching
for this case, just for consistency. You can see in
ResolveTargetsFromCommandLinePattern in commands.cc an example.

Putting this together, I think your command line would look like
  --filters="//base/*;chrome/*"
(you can see you can omit the leading slashes assuming the current directory is
"src", which should solve the MSVS problem).

[Tomasz Moniuszko, 2016-02-25 13:28:20]

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/command_gen.cc
File tools/gn/command_gen.cc (right):

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/command_gen.cc...
tools/gn/command_gen.cc:217: "GN optionally generates files for IDEs.
Possibilities for <ide options>\n"
On 2016/02/24 22:11:26, brettw wrote:
> Can you do this instead:
> """
> IDE options
> 
>   GN optionally generates files for IDEs. Possibilities for <ide options>\n
>   ...
> """
> 
> (so "IDE options" is the title, the string you have not is the text under it,
> indented, followed by the options).

Done.

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/command_gen.cc...
tools/gn/command_gen.cc:228: "      Override default sln file name (\"all\").
Only for Visual Studio.\n"
On 2016/02/24 22:11:26, brettw wrote:
> Can you explain where this file will be put (I assume in the root build
> directory)?

Yes, exactly (I added this information to the help).

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/command_gen.cc...
tools/gn/command_gen.cc:231: "      Semicolon-separated list of path prefixes
used to limit the set of\n"
On 2016/02/24 22:11:26, brettw wrote:
> Assuming we do the label pattern thing I suggested in the other file, this
> should reference "gn help label_pattern"

Done.

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/visual_studio_...
File tools/gn/visual_studio_writer.cc (right):

https://codereview.chromium.org/1718093006/diff/20001/tools/gn/visual_studio_...
tools/gn/visual_studio_writer.cc:239: (filter[0] == '/' &&
On 2016/02/24 22:11:26, brettw wrote:
> This seems complicated and isn't something we do anywhere else. My idea is to
do
> this the same way we handle the build dirs (which allow you to specify
> "//foo/bar" GN paths if you want): it will also resolve paths relative to the
> current directory. You can see in Setup::FillBuildDir how this works.
> 
> Also, GN has this "LabelPattern" concept that is used on various places. I
think
> it would be advantageous to use this rather than rolling custom prefix
matching
> for this case, just for consistency. You can see in
> ResolveTargetsFromCommandLinePattern in commands.cc an example.
> 
> Putting this together, I think your command line would look like
>   --filters="//base/*;chrome/*"
> (you can see you can omit the leading slashes assuming the current directory
is
> "src", which should solve the MSVS problem).

Many thanks for this hint. I was thinking about using string pattern from base
before. I missed label pattern and it seems it's exactly what we want. It works
perfectly.

[brettw, 2016-02-25 22:38:33]

much nicer, lgtm

https://codereview.chromium.org/1718093006/diff/40001/tools/gn/visual_studio_...
File tools/gn/visual_studio_writer.cc (right):

https://codereview.chromium.org/1718093006/diff/40001/tools/gn/visual_studio_...
tools/gn/visual_studio_writer.cc:233: for (std::string& token : tokens) {
Can you make this a "const std::string& token"?

[Tomasz Moniuszko, 2016-02-26 11:47:14]

https://codereview.chromium.org/1718093006/diff/40001/tools/gn/visual_studio_...
File tools/gn/visual_studio_writer.cc (right):

https://codereview.chromium.org/1718093006/diff/40001/tools/gn/visual_studio_...
tools/gn/visual_studio_writer.cc:233: for (std::string& token : tokens) {
On 2016/02/25 22:38:33, brettw wrote:
> Can you make this a "const std::string& token"?

Done.

[Tomasz Moniuszko, 2016-02-26 11:47:21]

The CQ bit was checked by tmoniuszko@opera.com

[Tomasz Moniuszko, 2016-02-26 11:47:22]

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/1718093006/#ps60001
(title: "Rebase onto current master + review and comment fix")

[commit-bot: I haz the power, 2016-02-26 11:47:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1718093006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1718093006/60001

[commit-bot: I haz the power, 2016-02-26 12:25:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-26 12:26:34]

Description was changed from

==========
Limit the set of Visual Studio projects generated by GN

BUG=589099
==========

to

==========
Limit the set of Visual Studio projects generated by GN

BUG=589099

Committed: https://crrev.com/014e81d144453d840e09eb9695b0dee262bc3507
Cr-Commit-Position: refs/heads/master@{#377871}
==========

[commit-bot: I haz the power, 2016-02-26 12:26:35]

Patchset 4 (id:??) landed as
https://crrev.com/014e81d144453d840e09eb9695b0dee262bc3507
Cr-Commit-Position: refs/heads/master@{#377871}