gn: Add 'clean' command.

gn: Add 'clean' command.

This emulates in C++ and in the GN tool what
 https://chromium.googlesource.com/chromium/src/+/master/build/landmines.py#82 does.

gn-dev mailing list discussion for more background is:
https://groups.google.com/a/chromium.org/d/topic/gn-dev/KTyLEh2Um98/discussion

The link to the doc proposing this command is:
https://docs.google.com/document/d/1DH-t-jYwQxZAXhqoBvnWGHWeF1z4Lltxny9D7lgC4HU/edit?pli=1

Tested with the following command lines:

$ gn gen out_gn_testing_clean_command/Debug --args='is_component_build=true'

$ out_gn/Debug/gn clean out_gn_testing_clean_command/Debug

Then:

$ ls out_gn_testing_clean_command/Debug
args.gn  build.ninja  build.ninja.d

To verify the files were written to disk correctly.

And:

$ cat out_gn_testing_clean_command/Debug/args.gn
$ cat out_gn_testing_clean_command/Debug/build.ninja
$ cat out_gn_testing_clean_command/Debug/build.ninja.d

To see if everything was written to the files correctly.

BUG=None
TEST=see above
R=brettw@chromium.org

Committed: https://crrev.com/7ad216606be9aa7b6d0da6109fe1ecf5d4968dab
Cr-Commit-Position: refs/heads/master@{#316670}

[tfarina, 2015-02-10 18:07:34]

tfarina@chromium.org changed reviewers:
- cjhopman@chromium.org

[tfarina, 2015-02-10 22:59:53]

Hi Brett, could you review this to me?

I'm happy to polish this. I'm excited to rear any comments and suggestions on
how to improve this implementation. I tried to be very loyal to the Python
implementation, so I used it as my guidance.

Dirk, Scott, Chris, please feel free to review this too, I look forward to your
comments as well.

As I spent my afternoon on trying to get this done, it was at least fun to see
something working ;)

Best regards,

[scottmg, 2015-02-11 00:16:35]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2015-02-11 00:16:37]

https://codereview.chromium.org/909103003/diff/280001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/280001/tools/gn/command_clean....
tools/gn/command_clean.cc:36: if (!base::ReadFileToString(build_ninja_file,
&file_contents)) {
It looks like you meant to remove this.

Alternatively, instead maybe keep this and use SplitString on '\n'?

https://codereview.chromium.org/909103003/diff/280001/tools/gn/command_clean....
tools/gn/command_clean.cc:91: // GN writes a build.ninja.d file. Note that not
all GN builds have args.gn.
This is GN, so it seems odd to say "GN writes a build.ninja.d file".

https://codereview.chromium.org/909103003/diff/280001/tools/gn/command_clean....
tools/gn/command_clean.cc:99: // GN builds aren't automatically regenerated when
you sync. To avoid
I guess this changes what the command does somewhat, but would it make sense to
instead have clean save args.gn, remove the rest, and then unconditionally do
"gn gen", rather than the build.ninja.d dance?

[cjhopman, 2015-02-11 01:59:41]

cjhopman@chromium.org changed reviewers:
+ cjhopman@chromium.org

[cjhopman, 2015-02-11 01:59:42]

https://codereview.chromium.org/909103003/diff/280001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/280001/tools/gn/command_clean....
tools/gn/command_clean.cc:99: // GN builds aren't automatically regenerated when
you sync. To avoid
On 2015/02/11 00:16:37, scottmg wrote:
> I guess this changes what the command does somewhat, but would it make sense
to
> instead have clean save args.gn, remove the rest, and then unconditionally do
> "gn gen", rather than the build.ninja.d dance?

I guess there's this weird issue that we treat gn gen options (i.e. things like
--time) differently than we treat args. After doing `gn gen --time`, if I do a
build and the ninja files are stale, it will run `gn gen` w/ the --time option.
This is really just that whatever options I passed the last time I did `gn gen`
are recorded in the build.ninja file.

I think I would expect gn clean to preserve these options just like a normal
call to ninja does (even though it may regenerate the underlying .ninja files).

Maybe we should keep these options in a file like args.gn (but sort of a
lower-level concept so that it could just be an opaque-ish set of options to the
`gn gen` call). Then in the ninja file we could just record that to regenerate
the build.ninja file we do `gn gen @opts.gn` or whatever. I guess it's important
then that opts.gn doesn't contain the --args option, or we could say that to
regen the files we do `gn gen @opts.gn --args-file args.gn` to ensure that
args.gn overrides opts.gn. Idk, I just guess that the recording of these options
directly in the build.ninja has always seemed cumbersome to me.

[tfarina, 2015-02-11 12:29:26]

PTAL

https://codereview.chromium.org/909103003/diff/280001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/280001/tools/gn/command_clean....
tools/gn/command_clean.cc:36: if (!base::ReadFileToString(build_ninja_file,
&file_contents)) {
On 2015/02/11 00:16:36, scottmg wrote:
> It looks like you meant to remove this.
> 
> Alternatively, instead maybe keep this and use SplitString on '\n'?

Hey, that was nice. Done.

https://codereview.chromium.org/909103003/diff/280001/tools/gn/command_clean....
tools/gn/command_clean.cc:91: // GN writes a build.ninja.d file. Note that not
all GN builds have args.gn.
On 2015/02/11 00:16:37, scottmg wrote:
> This is GN, so it seems odd to say "GN writes a build.ninja.d file".

Done.

[tfarina, 2015-02-12 15:43:27]

Chris, Scott, are you guys OK with it?

Brett, when you have a time, please, take a look as well. I need your approval
of course.

[brettw, 2015-02-13 20:53:15]

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:71: "Usage: \"gn clean <out_dir> \"").PrintToStdout();
No space after <out_dir>

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:76: if (!setup->DoSetup(args[0], false) ||
!setup->Run())
We don't want to do this since this will read the build files. "gn clean" should
be able to do its work without actually reading any build files.

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:87: base::DeleteFile(build_dir, true);
We don't want this. GN clean should only delete GN build directories and it
should give errors and do nothing for other directories.

[tfarina, 2015-02-14 01:46:40]

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:87: base::DeleteFile(build_dir, true);
On 2015/02/13 20:53:14, brettw wrote:
> We don't want this. GN clean should only delete GN build directories and it
> should give errors and do nothing for other directories.
Brett, what you mean by GN build directories? Do you want me to remove this line
or do something like traversing the output directory removing the ninja files?

[tfarina, 2015-02-16 20:33:33]

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:76: if (!setup->DoSetup(args[0], false) ||
!setup->Run())
On 2015/02/13 20:53:14, brettw wrote:
> We don't want to do this since this will read the build files. "gn clean"
should
> be able to do its work without actually reading any build files.

I need the DoSetup() call because it calls FillBuildDir(), and I need it to call
FillBuildDir(), so I can get the path to build_dir.

I will remove the Run() call though, it should not be necessary for what we want
to do here.

[brettw, 2015-02-16 23:35:21]

I can't think of a good reason not to run Setup as long as we don't do Run(), so
your plan seems OK.

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:66: "  Deletes the contents of the output directory
except for args.gn.\n";
add "and creates a Ninja build environment sufficient to regenerate the build."

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:91: // GN builds aren't automatically regenerated when
you sync. To avoid
This comment doesn't make sense in this context. Just start it from "Erase
everything..."

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:101: // Not all GN builds have a args.gn file.
I'd just delete the conditional and put a comment above this block of code like:
"Read the args.gn file, if any. Not all GN builds have one."

[tfarina, 2015-02-17 02:31:41]

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:66: "  Deletes the contents of the output directory
except for args.gn.\n";
On 2015/02/16 23:35:21, brettw wrote:
> add "and creates a Ninja build environment sufficient to regenerate the
build."

Done.

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:71: "Usage: \"gn clean <out_dir> \"").PrintToStdout();
On 2015/02/13 20:53:14, brettw wrote:
> No space after <out_dir>

Done.

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:76: if (!setup->DoSetup(args[0], false) ||
!setup->Run())
On 2015/02/13 20:53:14, brettw wrote:
> We don't want to do this since this will read the build files. "gn clean"
should
> be able to do its work without actually reading any build files.

Done.

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:91: // GN builds aren't automatically regenerated when
you sync. To avoid
On 2015/02/16 23:35:21, brettw wrote:
> This comment doesn't make sense in this context. Just start it from "Erase
> everything..."

Done.

https://codereview.chromium.org/909103003/diff/300001/tools/gn/command_clean....
tools/gn/command_clean.cc:101: // Not all GN builds have a args.gn file.
On 2015/02/16 23:35:21, brettw wrote:
> I'd just delete the conditional and put a comment above this block of code
like:
> "Read the args.gn file, if any. Not all GN builds have one."

Done.

[brettw, 2015-02-17 17:41:56]

https://codereview.chromium.org/909103003/diff/320001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/320001/tools/gn/command_clean....
tools/gn/command_clean.cc:88: base::DeleteFile(build_dir, true);
I don't think we want to do this. I think if we don't find a .d file (and
therefore know it's not a GN build directory) we should print an error that it
doesn't look like a build directory and return 1 without deleting anything.

[tfarina, 2015-02-17 18:49:19]

https://codereview.chromium.org/909103003/diff/320001/tools/gn/command_clean.cc
File tools/gn/command_clean.cc (right):

https://codereview.chromium.org/909103003/diff/320001/tools/gn/command_clean....
tools/gn/command_clean.cc:88: base::DeleteFile(build_dir, true);
On 2015/02/17 17:41:56, brettw wrote:
> I don't think we want to do this. I think if we don't find a .d file (and
> therefore know it's not a GN build directory) we should print an error that it
> doesn't look like a build directory and return 1 without deleting anything.

Done.

[brettw, 2015-02-17 20:40:27]

lgtm

[tfarina, 2015-02-17 22:11:47]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2015-02-17 22:12:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/909103003/340001

[commit-bot: I haz the power, 2015-02-17 22:17:41]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-02-17 22:18:35]

Patchset 18 (id:??) landed as
https://crrev.com/7ad216606be9aa7b6d0da6109fe1ecf5d4968dab
Cr-Commit-Position: refs/heads/master@{#316670}