Have the component build use the same manifest tricks for the installer archive that are now used b…

Have the component build use the same manifest tricks for the installer archive that are now used by chrome_elf.dll on trunk.

This means:
1) chrome.exe.manifest remains the same
2) Component DLLs are now the only special cased DLLs (instead of *.dll) -- this avoids allowing official DLLs to somehow load in the component build for someone that made a change that would otherwise break their load in an official build.
2) {version}.manifest is augmented with all the component DLLs.
3) Component DLLs are also copied in the Installer/ folder for setup.exe to work as well (without having to use the old setup.exe.config\probing\privatePath technique which isn't XP compatible).

I'd like to get a better solution up for (3), but since this is currently breaking developers I'll get this fix in now and we can iterate after.

TBR=grt
NOTRY=True
BUG=318950, 246169
TEST=Install a component build of Chrome, verify that chrome.exe and setup.exe run properly.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235244

[gab, 2013-11-13 23:48:22]

Greg, PTAL.

In particular I would like your opinion (and that of people CC'ed should they
desire) on the following:

Should we take a slightly more complex approach that will make setup.exe usable
from the Installer/ folder on XP?

i.e.:
This CL currently:
 - Augments the version_dir assembly to include all the DLLs
 - Thus chrome.exe.manifest no longer requires any modifications in component
builds
 - and chrome.exe will now work on XP installs
 - all of this thanks to the manifest system we just committed for
chrome_elf.dll).
 - great!

However, setup.exe can't use the same trick as the name of the assembly is
{version} and there is no {version} folder besides setup.exe (so for now I added
a dependency on the {version} assembly as required , but still had to use a
.config\probing\privatePath to tell setup.exe where to find that assembly...
this only works on Win7+...).

What we could do instead is to have setup.exe declare a dependency on an
assembly called '..' and add a ...manifest in the version dir (identical to
{version}.manifest in all but the 'name' property) -- I've tested this locally
and, surprisingly enough, it works...!

I would of course never recommend shipping anything with an assembly named '..',
but do we prefer:
 - using that hack and have something that uses the same technique everywhere
and allows us to use the installed setup.exe (e.g., to perform uninstalls) from
a component build on XP.
 or
 - Use a setup.exe.config to point into '..' as a "privatePath" where missing
assemblies, e.g. the {version} assembly, are looked for (only works on Win7+).

I personally think it would be nice to finally be able to use the component
build installer on XP :)!

Cheers!
Gab

[gab, 2013-11-14 22:39:38]

Changed approach, new approach is now fully XP compatible (yet to be tested
though!) -- see CL description, committing now to fix developers and we can
iterate tomorrow.

Cheers,
Gab

[gab, 2013-11-14 22:50:02]

Committed patchset #3 manually as r235244 (presubmit successful).

[grt (UTC plus 2), 2013-11-15 17:26:24]

LGTM with one minor nit. As discussed, please have someone with python
readability take a look at your changes. I suspect that this script is an
eyesore from a readability standpoint. It would be nice to have any new changes
and additions be compliant with our python style. Thanks for getting this in so
quickly!

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:346: 'w') as
modified_manifest_file :
minor nit: i think it's okay to use "f" or "file" in place of
modified_manifest_file here to get rid of the line wrapping since the code is so
obvious.

[Sigurður Ásgeirsson, 2013-11-15 18:31:11]

lgtm with a couple of nits (python looks fine apart from nits).

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:481: # assembly it
constitutes which will allow dependents of this assembly to
nit: constitutes, which

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:486: "  <file
name='{dll_filename}'/>".format(dll_filename=dll_filename))
nit: this is a fairly long-winded way to say

"  <file name='%s'" % dll_filename

the named format specifiers are generally best left for more complex cases, as
where you take a non-static or a multi-line string that may need multiple
expansions of the same parameter. 
Same below.

[Sigurður Ásgeirsson, 2013-11-15 18:33:51]

NB: I only reviewed the python from the standpoint of consistency with the
script at large and compliance to coding guidelines - not with an eye to
improvements.


On Fri, Nov 15, 2013 at 1:31 PM, <siggi@chromium.org> wrote:

> lgtm with a couple of nits (python looks fine apart from nits).
>
>
>
> https://codereview.chromium.org/63153013/diff/110001/
> chrome/tools/build/win/create_installer_archive.py
> File chrome/tools/build/win/create_installer_archive.py (right):
>
> https://codereview.chromium.org/63153013/diff/110001/
> chrome/tools/build/win/create_installer_archive.py#newcode481
> chrome/tools/build/win/create_installer_archive.py:481: # assembly it
> constitutes which will allow dependents of this assembly to
> nit: constitutes, which
>
> https://codereview.chromium.org/63153013/diff/110001/
> chrome/tools/build/win/create_installer_archive.py#newcode486
> chrome/tools/build/win/create_installer_archive.py:486: "  <file
> name='{dll_filename}'/>".format(dll_filename=dll_filename))
> nit: this is a fairly long-winded way to say
>
> "  <file name='%s'" % dll_filename
>
> the named format specifiers are generally best left for more complex
> cases, as where you take a non-static or a multi-line string that may
> need multiple expansions of the same parameter.
> Same below.
>
> https://codereview.chromium.org/63153013/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2013-11-15 21:18:52]

Thanks, comments addressed on https://codereview.chromium.org/60233008/

Cheers,
Gab

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:346: 'w') as
modified_manifest_file :
On 2013/11/15 17:26:24, grt wrote:
> minor nit: i think it's okay to use "f" or "file" in place of
> modified_manifest_file here to get rid of the line wrapping since the code is
so
> obvious.

Done.

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:481: # assembly it
constitutes which will allow dependents of this assembly to
On 2013/11/15 18:31:12, Sigurður Ásgeirsson wrote:
> nit: constitutes, which

Done.

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:486: "  <file
name='{dll_filename}'/>".format(dll_filename=dll_filename))
On 2013/11/15 18:31:12, Sigurður Ásgeirsson wrote:
> nit: this is a fairly long-winded way to say
> 
> "  <file name='%s'" % dll_filename
> 
> the named format specifiers are generally best left for more complex cases, as
> where you take a non-static or a multi-line string that may need multiple
> expansions of the same parameter. 
> Same below.

Done.

[gab, 2013-11-15 21:24:02]

Math, could you please take a look at the python style in this CL.

Changes will be made in https://codereview.chromium.org/60233008/ (a few things
were already done there -- sorry for the double-CL confusion had to commit this
quick as it was fixing dev builds but would still like a style review now).

Thanks!
Gab

[gab, 2013-11-15 21:28:48]

Math, could you please take a look at the python style in this CL.

Changes will be made in https://codereview.chromium.org/60233008/ (a few things
were already done there -- sorry for the double-CL confusion had to commit this
quick as it was fixing dev builds but would still like a style review now).

Thanks!
Gab

[Mathieu, 2013-11-18 19:10:05]

Sorry for the delay. Lgtm

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:451:
glob.glob(os.path.join(version_dir, '*.dll'))]
nit: you can de-indent by 4

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:454: os.path.basename(dll)
not in staged_dll_basenames]:
nit: same here

[gab, 2013-11-18 20:37:16]

Thanks,

comments addressed on https://codereview.chromium.org/60233008/

Cheers,
Gab

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
File chrome/tools/build/win/create_installer_archive.py (right):

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:451:
glob.glob(os.path.join(version_dir, '*.dll'))]
On 2013/11/18 19:10:06, Mathieu Perreault wrote:
> nit: you can de-indent by 4

Done.

https://codereview.chromium.org/63153013/diff/110001/chrome/tools/build/win/c...
chrome/tools/build/win/create_installer_archive.py:454: os.path.basename(dll)
not in staged_dll_basenames]:
On 2013/11/18 19:10:06, Mathieu Perreault wrote:
> nit: same here

Done.