ninja generator vs file permissions

ninja/mac: Let mac_bundle_resources keep file permissions.

Also add a test that this is true for 'copies' too.

BUG=chromium:315383
R=mark@chromium.org

Committed: https://code.google.com/p/gyp/source/detail?r=1779

[Mark Mentovai, 2013-11-06 02:33:43]

https://codereview.chromium.org/61293004/diff/50001/test/mac/gyptest-bundle-r...
File test/mac/gyptest-bundle-resources.py (right):

https://codereview.chromium.org/61293004/diff/50001/test/mac/gyptest-bundle-r...
test/mac/gyptest-bundle-resources.py:41: if in_stat.st_mode != out_stat.st_mode:
They might not be exactly equal if you check out with a more restrictive umask
like, say, 077. Then the file in the source tree would have mode 0700, but the
built product could still wind up with mode 0755.

It’s probably fine to just check that the user execute bit is equal on both
files. A more comprehensive check would be to see if any execute bit is set on
the source, then at least those execute bits must be set on the destination,
otherwise, no execute bits must be set on the destination.

[Nico, 2013-11-06 02:39:27]

On 2013/11/06 02:33:43, Mark Mentovai wrote:
>
https://codereview.chromium.org/61293004/diff/50001/test/mac/gyptest-bundle-r...
> File test/mac/gyptest-bundle-resources.py (right):
> 
>
https://codereview.chromium.org/61293004/diff/50001/test/mac/gyptest-bundle-r...
> test/mac/gyptest-bundle-resources.py:41: if in_stat.st_mode !=
out_stat.st_mode:
> They might not be exactly equal if you check out with a more restrictive umask
> like, say, 077. Then the file in the source tree would have mode 0700, but the
> built product could still wind up with mode 0755.

Oh, you mean if the umask is different at build time and checkout time? This is
only a problem if the build-time umask is more restrictive than the checkout
time umask, right? And it's probably fine if the test requires that your test
time umask isn't more restrictive than the checkout time umask?

> 
> It’s probably fine to just check that the user execute bit is equal on both
> files. A more comprehensive check would be to see if any execute bit is set on
> the source, then at least those execute bits must be set on the destination,
> otherwise, no execute bits must be set on the destination.

[Nico, 2013-11-06 02:45:33]

…but checking for X on the X file and not X on a regular file is a good idea,
done. PTAL!

[Mark Mentovai, 2013-11-06 02:46:54]

The build-time umask might not matter. Xcode always copies files as though the
umask were set to 022, even if it’s set to something else. Then it doesn’t
matter if you check out and build with the same restrictive umask (say, 077),
because the files in the build will have a different mode.

I know we don’t care about Xcode for Chrome any more, but in GYP, the test will
still run under Xcode.

So yes, I think it’s OK to assume that the build umask will be at least as
permissive as the checkout umask, but not OK to assume anything else.

[Mark Mentovai, 2013-11-06 02:50:40]

https://codereview.chromium.org/61293004/diff/210001/pylib/gyp/mac_tool.py
File pylib/gyp/mac_tool.py (right):

https://codereview.chromium.org/61293004/diff/210001/pylib/gyp/mac_tool.py#ne...
pylib/gyp/mac_tool.py:53: # be consistent with the single-file branch.
Hmm, that’s kind of annoying.

https://codereview.chromium.org/61293004/diff/210001/test/copies/gyptest-attr...
File test/copies/gyptest-attribs.py (right):

https://codereview.chromium.org/61293004/diff/210001/test/copies/gyptest-attr...
test/copies/gyptest-attribs.py:30: if in_stat.st_mode != out_stat.st_mode:
Fix me too.

https://codereview.chromium.org/61293004/diff/210001/test/copies/src/copies-a...
File test/copies/src/copies-attribs.gyp (right):

https://codereview.chromium.org/61293004/diff/210001/test/copies/src/copies-a...
test/copies/src/copies-attribs.gyp:21: 
No blank line here.

https://codereview.chromium.org/61293004/diff/210001/test/mac/gyptest-bundle-...
File test/mac/gyptest-bundle-resources.py (right):

https://codereview.chromium.org/61293004/diff/210001/test/mac/gyptest-bundle-...
test/mac/gyptest-bundle-resources.py:24: if in_stat.st_mode != out_stat.st_mode:
You’re returning the mode for comparison outside of this function, you shouldn’t
have this check here.

But if you want to do some common check here for the user read bit, that’d be
cool.

[Nico, 2013-11-06 02:58:16]

https://codereview.chromium.org/61293004/diff/210001/test/copies/gyptest-attr...
File test/copies/gyptest-attribs.py (right):

https://codereview.chromium.org/61293004/diff/210001/test/copies/gyptest-attr...
test/copies/gyptest-attribs.py:30: if in_stat.st_mode != out_stat.st_mode:
On 2013/11/06 02:50:40, Mark Mentovai wrote:
> Fix me too.

Done.

https://codereview.chromium.org/61293004/diff/210001/test/copies/src/copies-a...
File test/copies/src/copies-attribs.gyp (right):

https://codereview.chromium.org/61293004/diff/210001/test/copies/src/copies-a...
test/copies/src/copies-attribs.gyp:21: 
On 2013/11/06 02:50:40, Mark Mentovai wrote:
> No blank line here.

Done.

https://codereview.chromium.org/61293004/diff/210001/test/mac/gyptest-bundle-...
File test/mac/gyptest-bundle-resources.py (right):

https://codereview.chromium.org/61293004/diff/210001/test/mac/gyptest-bundle-...
test/mac/gyptest-bundle-resources.py:24: if in_stat.st_mode != out_stat.st_mode:
On 2013/11/06 02:50:40, Mark Mentovai wrote:
> You’re returning the mode for comparison outside of this function, you
shouldn’t
> have this check here.

I'm only returning the out_stat.st_mode, and only if it was equal to the in_stat
mode.

> But if you want to do some common check here for the user read bit, that’d be
> cool.

Done.

[Mark Mentovai, 2013-11-06 03:10:11]

https://codereview.chromium.org/61293004/diff/150003/test/copies/gyptest-attr...
File test/copies/gyptest-attribs.py (right):

https://codereview.chromium.org/61293004/diff/150003/test/copies/gyptest-attr...
test/copies/gyptest-attribs.py:3: # Copyright (c) 2009 Google Inc. All rights
reserved.
2013

https://codereview.chromium.org/61293004/diff/150003/test/copies/gyptest-attr...
test/copies/gyptest-attribs.py:23: if in_stat.st_mode != out_stat.st_mode:
But checking whether the source file and the destination file have exactly the
same mode, bit-for-bit, is the exact umask problem we were talking about.

[Nico, 2013-11-06 03:16:40]

https://codereview.chromium.org/61293004/diff/150003/test/copies/gyptest-attr...
File test/copies/gyptest-attribs.py (right):

https://codereview.chromium.org/61293004/diff/150003/test/copies/gyptest-attr...
test/copies/gyptest-attribs.py:3: # Copyright (c) 2009 Google Inc. All rights
reserved.
On 2013/11/06 03:10:12, Mark Mentovai wrote:
> 2013

Done.

https://codereview.chromium.org/61293004/diff/150003/test/copies/gyptest-attr...
test/copies/gyptest-attribs.py:23: if in_stat.st_mode != out_stat.st_mode:
On 2013/11/06 03:10:12, Mark Mentovai wrote:
> But checking whether the source file and the destination file have exactly the
> same mode, bit-for-bit, is the exact umask problem we were talking about.

ok, removed this

(looks like mac_tool.py ignores umask and xcode doesn't when copying)

[Mark Mentovai, 2013-11-06 03:18:12]

LGTM. I assume the trybot failures are because it doesn’t apply patches with the
x-bit.

https://codereview.chromium.org/61293004/diff/550001/test/copies/gyptest-attr...
File test/copies/gyptest-attribs.py (right):

https://codereview.chromium.org/61293004/diff/550001/test/copies/gyptest-attr...
test/copies/gyptest-attribs.py:3: # Copyright (c) 2009 Google Inc. All rights
reserved.
2013 (you said you fixed this).

[Nico, 2013-11-06 03:19:08]

Thanks!

Will land and watch go/gypbot

https://codereview.chromium.org/61293004/diff/550001/test/copies/gyptest-attr...
File test/copies/gyptest-attribs.py (right):

https://codereview.chromium.org/61293004/diff/550001/test/copies/gyptest-attr...
test/copies/gyptest-attribs.py:3: # Copyright (c) 2009 Google Inc. All rights
reserved.
On 2013/11/06 03:18:13, Mark Mentovai wrote:
> 2013 (you said you fixed this).

Done.

[Nico, 2013-11-06 03:20:44]

Committed patchset #12 manually as r1779 (presubmit successful).

[Nico, 2013-11-06 03:40:51]

(the tests pass on gypbot, so the trybots just had an issue with the executable
files, like you suspected)