Teach build/symlink.py --force to delete directories.

Teach build/symlink.py --force to delete directories.

Otherwise os.remove just fails saying it can't delete a directory.

R=dpranke@chromium.org

Committed: https://crrev.com/add100b7cd19ae6caad54ac6c6ed9766b7f1f70b
Cr-Commit-Position: refs/heads/master@{#337083}

[eseidel, 2015-07-01 18:19:55]

eseidel@chromium.org changed reviewers:
+ dpranke@chromium.org
- dprank@chromium.org

[Nico, 2015-07-01 18:26:42]

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

[Nico, 2015-07-01 18:26:43]

lgtm if you don't need this on Windows

https://codereview.chromium.org/1222573002/diff/1/build/symlink.py
File build/symlink.py (right):

https://codereview.chromium.org/1222573002/diff/1/build/symlink.py#newcode34
build/symlink.py:34: shutil.rmtree(t, ignore_errors=True)
Do you need this to work on Windows? shutil.rmtree() fails on Windows if there
are read-only files in the directory. That's why many places do

  if win:
    shell out to rmdir /s/q
  else:
    shutil.rmtree

[eseidel, 2015-07-01 18:27:34]

I don't think windows support is needed for this script given that it's
symlinks.  But I could be wrong.

[Nico, 2015-07-01 18:27:34]

https://codereview.chromium.org/1222573002/diff/1/build/symlink.py
File build/symlink.py (right):

https://codereview.chromium.org/1222573002/diff/1/build/symlink.py#newcode6
build/symlink.py:6: """Make a symlink and optionally touch a file (to handle
dependencies)."""
…actually, it looks like this is for build deps. Depending on a directory in
general won't do the right thing. What are you trying to do? What's this for?

[eseidel, 2015-07-01 18:27:38]

The CQ bit was checked by eseidel@chromium.org

[eseidel, 2015-07-01 18:27:49]

The CQ bit was unchecked by eseidel@chromium.org

[eseidel, 2015-07-01 18:28:36]

Perhaps we're abusing the script then.  I can ditch this.

[Nico, 2015-07-01 18:30:42]

To elaborate: If you have a directory as a build dep, then that directory's
mtime is only updated if a file directly in that directory is changed. If a file
in a subdirectory in that directory is changed, it doesn't affect the mtime of
the outer directory. So when you change things in a subdirectory, the build
system won't notice if you only have a dependency on the outer directory.

I don't know what you do with this script, so I don't know if that's a problem
or not.

[abarth-chromium, 2015-07-01 18:31:00]

On 2015/07/01 at 18:27:34, thakis wrote:
> https://codereview.chromium.org/1222573002/diff/1/build/symlink.py
> File build/symlink.py (right):
> 
> https://codereview.chromium.org/1222573002/diff/1/build/symlink.py#newcode6
> build/symlink.py:6: """Make a symlink and optionally touch a file (to handle
dependencies)."""
> …actually, it looks like this is for build deps. Depending on a directory in
general won't do the right thing. What are you trying to do? What's this for?

The idea is to make this script robust to whatever junk you have in your out
directory at this location already.  We ran into this problem because we used to
have a directory and now we have a symlink.  Using --force wasn't enough to make
people's incremental builds work.  We need --force to blow away whatever used to
be at this location in the out directory.

[Nico, 2015-07-01 18:35:25]

On 2015/07/01 18:31:00, abarth-chromium wrote:
> On 2015/07/01 at 18:27:34, thakis wrote:
> > https://codereview.chromium.org/1222573002/diff/1/build/symlink.py
> > File build/symlink.py (right):
> > 
> > https://codereview.chromium.org/1222573002/diff/1/build/symlink.py#newcode6
> > build/symlink.py:6: """Make a symlink and optionally touch a file (to handle
> dependencies)."""
> > …actually, it looks like this is for build deps. Depending on a directory in
> general won't do the right thing. What are you trying to do? What's this for?
> 
> The idea is to make this script robust to whatever junk you have in your out
> directory at this location already.  We ran into this problem because we used
to
> have a directory and now we have a symlink.  Using --force wasn't enough to
make
> people's incremental builds work.  We need --force to blow away whatever used
to
> be at this location in the out directory.

This makes sense from a distance. If you think this is the right thing given
what I said above, then go click cq – my lg from above stands :-)

[eseidel, 2015-07-01 19:00:57]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2015-07-01 19:03:23]

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

[commit-bot: I haz the power, 2015-07-01 19:09:51]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-07-01 19:11:06]

Patchset 1 (id:??) landed as
https://crrev.com/add100b7cd19ae6caad54ac6c6ed9766b7f1f70b
Cr-Commit-Position: refs/heads/master@{#337083}

[Dirk Pranke, 2015-07-01 20:05:05]

Who actually calls this script? I didn't see any references to it in cs/ or from
grepping around?

[eseidel, 2015-07-01 20:12:16]

One BUILD.gn in mojo? :p

https://github.com/domokit/mojo/blob/ac21cef12f1867925e97f96fea53e6d65c382c8e...

[Dirk Pranke, 2015-07-01 20:15:29]

On 2015/07/01 20:12:16, eseidel wrote:
> One BUILD.gn in mojo? :p
> 
>
https://github.com/domokit/mojo/blob/ac21cef12f1867925e97f96fea53e6d65c382c8e...

Does it make sense to just copy this file there and delete it from //build,
then?

[eseidel, 2015-07-01 21:02:05]

sgtm.