win ninja: Refactor manifest generate and embed to be 1-pass

win ninja: Refactor manifest generate and embed to be 1-pass

The 'normal' way to do manifests is to have link generate a manifest
based on gathering dependencies from the object files, then merge that
manifest with other manifests supplied as sources, convert the merged
manifest to a resource, and then *relink*, including the compiled
version of the manifest resource. This breaks incremental linking, and
is generally overly complicated.

Instead, we merge all the manifests provided (along with one that
includes what would normally be in the linker-generated one, see
msvs_emulation.py), and include that into the first and only link.
We still tell link to generate a manifest, but we only use that to
assert that our simpler process did not miss anything.

R=thakis@chromium.org, yukawa@chromium.org
BUG=chromium:324863
Committed: https://code.google.com/p/gyp/source/detail?r=1813

[yukawa, 2013-12-10 11:18:30]

LGTM++ with one minor question. Feel free to go ahead if it is not a problem.

https://codereview.chromium.org/111373002/diff/20001/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/111373002/diff/20001/pylib/gyp/msvs_emulation...
pylib/gyp/msvs_emulation.py:607: with open_output(os.path.join(build_dir,
generated_name), 'w') as f:
If the time stamp of |os.path.join(build_dir, generated_name)| is unnecessary
updated here, does Ninja think that all the exe/dll targets are out-of-date?

Does this make sense if we leave the original |os.path.join(build_dir,
generated_name)| as it is when it already exists and its content is exactly the
same to |generated_manifest_contents|?

[scottmg, 2013-12-10 19:27:20]

https://codereview.chromium.org/111373002/diff/20001/pylib/gyp/msvs_emulation.py
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/111373002/diff/20001/pylib/gyp/msvs_emulation...
pylib/gyp/msvs_emulation.py:607: with open_output(os.path.join(build_dir,
generated_name), 'w') as f:
On 2013/12/10 11:18:30, yukawa OOO Dec 10 - Jan 5 wrote:
> If the time stamp of |os.path.join(build_dir, generated_name)| is unnecessary
> updated here, does Ninja think that all the exe/dll targets are out-of-date?
> 

Good question! I believe it will not cause unnecessary rebuilds because
.generated.manifest is not mentioned as a ninja input. I added a test for this
to gyptest-link-generate-manifest.py.

However, I think this also points to a bug, because now if e.g. the 'EnableUAC'
setting is modified it will not be enough to trigger a relink. I will address
this and add a test for it.

> Does this make sense if we leave the original |os.path.join(build_dir,
> generated_name)| as it is when it already exists and its content is exactly
the
> same to |generated_manifest_contents|?

Once the above is fixed, I will need to do this also.

[scottmg, 2013-12-10 19:34:39]

On 2013/12/10 19:27:20, scottmg wrote:
>
https://codereview.chromium.org/111373002/diff/20001/pylib/gyp/msvs_emulation.py
> File pylib/gyp/msvs_emulation.py (right):
> 
>
https://codereview.chromium.org/111373002/diff/20001/pylib/gyp/msvs_emulation...
> pylib/gyp/msvs_emulation.py:607: with open_output(os.path.join(build_dir,
> generated_name), 'w') as f:
> On 2013/12/10 11:18:30, yukawa OOO Dec 10 - Jan 5 wrote:
> > If the time stamp of |os.path.join(build_dir, generated_name)| is
unnecessary
> > updated here, does Ninja think that all the exe/dll targets are out-of-date?
> > 
> 
> Good question! I believe it will not cause unnecessary rebuilds because
> .generated.manifest is not mentioned as a ninja input. I added a test for this
> to gyptest-link-generate-manifest.py.
> 
> However, I think this also points to a bug, because now if e.g. the
'EnableUAC'
> setting is modified it will not be enough to trigger a relink. I will address
> this and add a test for it.

In fact even before this CL, a modification to any files listed in
AdditionalManifestFiles will incorrectly not cause a relink.

> 
> > Does this make sense if we leave the original |os.path.join(build_dir,
> > generated_name)| as it is when it already exists and its content is exactly
> the
> > same to |generated_manifest_contents|?
> 
> Once the above is fixed, I will need to do this also.

[scottmg, 2013-12-11 03:45:51]

On 2013/12/10 19:34:39, scottmg wrote:
> On 2013/12/10 19:27:20, scottmg wrote:
> >
>
https://codereview.chromium.org/111373002/diff/20001/pylib/gyp/msvs_emulation.py
> > File pylib/gyp/msvs_emulation.py (right):
> > 
> >
>
https://codereview.chromium.org/111373002/diff/20001/pylib/gyp/msvs_emulation...
> > pylib/gyp/msvs_emulation.py:607: with open_output(os.path.join(build_dir,
> > generated_name), 'w') as f:
> > On 2013/12/10 11:18:30, yukawa OOO Dec 10 - Jan 5 wrote:
> > > If the time stamp of |os.path.join(build_dir, generated_name)| is
> unnecessary
> > > updated here, does Ninja think that all the exe/dll targets are
out-of-date?
> > > 
> > 
> > Good question! I believe it will not cause unnecessary rebuilds because
> > .generated.manifest is not mentioned as a ninja input. I added a test for
this
> > to gyptest-link-generate-manifest.py.
> > 
> > However, I think this also points to a bug, because now if e.g. the
> 'EnableUAC'
> > setting is modified it will not be enough to trigger a relink. I will
address
> > this and add a test for it.
> 
> In fact even before this CL, a modification to any files listed in
> AdditionalManifestFiles will incorrectly not cause a relink.

Test added and made to work for modifying manifest settings, or a manifest
listed in AdditionalManifestFiles.

> 
> > 
> > > Does this make sense if we leave the original |os.path.join(build_dir,
> > > generated_name)| as it is when it already exists and its content is
exactly
> > the
> > > same to |generated_manifest_contents|?
> > 
> > Once the above is fixed, I will need to do this also.

[yukawa, 2013-12-11 05:28:56]

Great!
Looked at patch set #6 and still LGTM.

[Nico, 2013-12-12 17:45:54]

https://codereview.chromium.org/111373002/diff/160001/pylib/gyp/msvs_emulatio...
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/111373002/diff/160001/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:77: f.write(contents)
(There's gyp.common.WriteOnDiff fwiw. It's a bit overengineered compared to this
function, but it already exists)

[scottmg, 2013-12-12 18:14:05]

https://codereview.chromium.org/111373002/diff/160001/pylib/gyp/msvs_emulatio...
File pylib/gyp/msvs_emulation.py (right):

https://codereview.chromium.org/111373002/diff/160001/pylib/gyp/msvs_emulatio...
pylib/gyp/msvs_emulation.py:77: f.write(contents)
On 2013/12/12 17:45:54, Nico wrote:
> (There's gyp.common.WriteOnDiff fwiw. It's a bit overengineered compared to
this
> function, but it already exists)

Thanks, done with a little rejiggering.

That function is trying hard to be clever... but doesn't work with 'with'
unfortunately. (__enter__/__exit__ aren't delegated to __getattr__ maybe? not
sure, but it doesn't write the file anyway)

[simpler-manifest]D:\src\cr\src\tools\gyp\pylib\gyp>python
Python 2.7.6 (default, Nov 10 2013, 19:24:18) [MSC v.1500 32 bit (Intel)] on
win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import common
>>> with common.WriteOnDiff('a') as f:
...   f.write('xyz')
...
>>> ^Z


[simpler-manifest]D:\src\cr\src\tools\gyp\pylib\gyp>dir a
 Volume in drive D is ssd512
 Volume Serial Number is 768D-3ADE

 Directory of D:\src\cr\src\tools\gyp\pylib\gyp

File Not Found

[scottmg, 2013-12-12 18:17:01]

On 2013/12/12 18:14:05, scottmg wrote:
>
https://codereview.chromium.org/111373002/diff/160001/pylib/gyp/msvs_emulatio...
> File pylib/gyp/msvs_emulation.py (right):
> 
>
https://codereview.chromium.org/111373002/diff/160001/pylib/gyp/msvs_emulatio...
> pylib/gyp/msvs_emulation.py:77: f.write(contents)
> On 2013/12/12 17:45:54, Nico wrote:
> > (There's gyp.common.WriteOnDiff fwiw. It's a bit overengineered compared to
> this
> > function, but it already exists)
> 
> Thanks, done with a little rejiggering.
> 
> That function is trying hard to be clever... but doesn't work with 'with'
> unfortunately. (__enter__/__exit__ aren't delegated to __getattr__ maybe? not
> sure, but it doesn't write the file anyway)
> 
> [simpler-manifest]D:\src\cr\src\tools\gyp\pylib\gyp>python
> Python 2.7.6 (default, Nov 10 2013, 19:24:18) [MSC v.1500 32 bit (Intel)] on
> win32
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import common
> >>> with common.WriteOnDiff('a') as f:
> ...   f.write('xyz')
> ...
> >>> ^Z
> 
> 
> [simpler-manifest]D:\src\cr\src\tools\gyp\pylib\gyp>dir a
>  Volume in drive D is ssd512
>  Volume Serial Number is 768D-3ADE
> 
>  Directory of D:\src\cr\src\tools\gyp\pylib\gyp
> 
> File Not Found

Yeah, file_exit
https://android.googlesource.com/toolchain/python/+/2d345c2b39272339d8d672bfa...
calls close on itself, but not this wrapper one. Meh.

[Nico, 2013-12-12 18:36:17]

Diff from patch set 7 to 9 lgtm :-P

[scottmg, 2013-12-12 21:09:45]

Committed patchset #9 manually as r1813 (presubmit successful).