Do not compress chrome.7z into chrome.packed.7z for developer (component) builds.

Do not compress chrome.7z into chrome.packed.7z for developer (component) builds.

Note that setup is already configured to use chrome.7z directly if chrome.packed.7z is absent.

Change setup to have it copy chrome.7z to the <version>\Installer folder (instead of moving it) for component builds (to allow developers to test installs more than once with the same archive).

BUG=None
TEST=Install/uninstall chrome after copying setup.exe to another machine using tools\win\copy-installer.bat
Test non-component build and make sure archive is still moved (i.e.not copied).

[gab, 2012-08-14 16:14:27]

PTAL, it drastically improves build timings for the mini_installer.


i.e. building with no changes (i.e. just remaking the archive):

Debug: 3:30 -> 0:18
Release: 1:13 -> 0:13

The copying is then a bit longer (as chrome.7z is bigger than its packed
version...)... about 30s to copy the whole Debug chrome.7z, but then you also
benefit at install when you don't need to unpack chrome.packed.7z :)!

Cheers,
Gab

[robertshield, 2012-08-14 16:45:28]

http://codereview.chromium.org/10825342/diff/1/chrome/installer/mini_installe...
File chrome/installer/mini_installer.gyp (right):

http://codereview.chromium.org/10825342/diff/1/chrome/installer/mini_installe...
chrome/installer/mini_installer.gyp:261:
'<(PRODUCT_DIR)/<(RULE_INPUT_NAME).packed.7z',
this still specifies the packed.7z as an output, won't it get unhappy if the
file is not present?

http://codereview.chromium.org/10825342/diff/1/chrome/tools/build/win/create_...
File chrome/tools/build/win/create_installer_archive.py (right):

http://codereview.chromium.org/10825342/diff/1/chrome/tools/build/win/create_...
chrome/tools/build/win/create_installer_archive.py:218: return
options.output_name + ARCHIVE_SUFFIX
change this to os.path.basename(archive_file)

http://codereview.chromium.org/10825342/diff/1/chrome/tools/build/win/create_...
chrome/tools/build/win/create_installer_archive.py:577: 'skipped.')
Mention that this also deletes any existing chrome.packed.7z file

http://codereview.chromium.org/10825342/diff/1/tools/win/copy-installer.bat
File tools/win/copy-installer.bat (right):

http://codereview.chromium.org/10825342/diff/1/tools/win/copy-installer.bat#n...
tools/win/copy-installer.bat:53: SET TOCOPY=*.pdb setup.exe setup.exe.manifest
chrome*.7z *.dll
Ideally this should copy chrome.packed.7z if it exists and chrome.7z if it
doesn't, but this is fine too.

[gab, 2012-08-14 19:15:37]

/dance

http://codereview.chromium.org/10825342/diff/1/chrome/installer/mini_installe...
File chrome/installer/mini_installer.gyp (right):

http://codereview.chromium.org/10825342/diff/1/chrome/installer/mini_installe...
chrome/installer/mini_installer.gyp:261:
'<(PRODUCT_DIR)/<(RULE_INPUT_NAME).packed.7z',
On 2012/08/14 16:45:28, robertshield wrote:
> this still specifies the packed.7z as an output, won't it get unhappy if the
> file is not present?

no, I think outputs only matter for targets that depend on them... no one
depends on chrome.packed.7z...for now... so I'll add it anyways :).

http://codereview.chromium.org/10825342/diff/1/chrome/tools/build/win/create_...
File chrome/tools/build/win/create_installer_archive.py (right):

http://codereview.chromium.org/10825342/diff/1/chrome/tools/build/win/create_...
chrome/tools/build/win/create_installer_archive.py:218: return
options.output_name + ARCHIVE_SUFFIX
On 2012/08/14 16:45:28, robertshield wrote:
> change this to os.path.basename(archive_file)

Done.

http://codereview.chromium.org/10825342/diff/1/chrome/tools/build/win/create_...
chrome/tools/build/win/create_installer_archive.py:577: 'skipped.')
On 2012/08/14 16:45:28, robertshield wrote:
> Mention that this also deletes any existing chrome.packed.7z file

Wrote an inspiring comment :)

http://codereview.chromium.org/10825342/diff/1/tools/win/copy-installer.bat
File tools/win/copy-installer.bat (right):

http://codereview.chromium.org/10825342/diff/1/tools/win/copy-installer.bat#n...
tools/win/copy-installer.bat:53: SET TOCOPY=*.pdb setup.exe setup.exe.manifest
chrome*.7z *.dll
On 2012/08/14 16:45:28, robertshield wrote:
> Ideally this should copy chrome.packed.7z if it exists and chrome.7z if it
> doesn't, but this is fine too.

Done.

[robertshield, 2012-08-14 19:29:24]

LGTM

[commit-bot: I haz the power, 2012-08-14 19:29:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10825342/7001

[commit-bot: I haz the power, 2012-08-14 19:29:47]

Failed to apply patch for tools/win/copy-installer.bat:
While running patch -p1 --forward --force;
patching file tools/win/copy-installer.bat
Hunk #1 FAILED at 9.
Hunk #2 FAILED at 50.
2 out of 2 hunks FAILED -- saving rejects to file
tools/win/copy-installer.bat.rej

[gab, 2012-08-14 21:12:40]

+jam: OWNERS review for chrome/

PS: we are working on an OWNERS file in chrome\tools\build\win to avoid needing
chrome\OWNERS for these tools anymore, but haven't settled on appropriate people
yet.

[jam, 2012-08-15 14:59:47]

redirecting to another owner who knows about gyp a lot more than me.

[Nico, 2012-08-15 16:32:18]

I don't like "secret" tweakables that one has to remember to set to make one's
build not slow. I could live with this if it defaults to the fast-for-developers
option, but for that you need to update all the build bots to add that flag.
Alternatively, you could turn this on automatically in component builds, then
it'll be on by default by developers automatically.

[gab, 2012-08-15 19:33:02]

On 2012/08/15 16:32:18, Nico wrote:
> I don't like "secret" tweakables that one has to remember to set to make one's
> build not slow. I could live with this if it defaults to the
fast-for-developers
> option, but for that you need to update all the build bots to add that flag.
> Alternatively, you could turn this on automatically in component builds, then
> it'll be on by default by developers automatically.

Done. Let me know what you think now.

Thanks,
Gab

[robertshield, 2012-08-15 19:38:32]

http://codereview.chromium.org/10825342/diff/12002/tools/win/copy-installer.bat
File tools/win/copy-installer.bat (right):

http://codereview.chromium.org/10825342/diff/12002/tools/win/copy-installer.b...
tools/win/copy-installer.bat:74: ECHO Renaming old %ARCHIVETODELETE% to
%ARCHIVETODELETE%.old
I'd just delete it. All the other files have been replaced, this archive can't
be used anymore anyway.

[Nico, 2012-08-15 19:40:08]

patch set 3 lgtm

[commit-bot: I haz the power, 2012-08-15 19:50:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10825342/1006

[commit-bot: I haz the power, 2012-08-15 19:50:53]

Failed to apply patch for tools/win/copy-installer.bat:
While running patch -p1 --forward --force;
patching file tools/win/copy-installer.bat
Hunk #1 FAILED at 9.
Hunk #2 FAILED at 50.
Hunk #3 FAILED at 66.
3 out of 3 hunks FAILED -- saving rejects to file
tools/win/copy-installer.bat.rej

[gab, 2012-08-17 16:32:44]

@robert: tweaked it a little more, can you PTAL?

Thanks!
Gab

[robertshield, 2012-08-17 16:46:56]

LGTM, w/ nits

http://codereview.chromium.org/10825342/diff/1007/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/10825342/diff/1007/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:120: // move it as mentionned above;
however if it is not in |temp_path| (e.g. in
nit: mentioned

http://codereview.chromium.org/10825342/diff/1007/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:128: // that setup.exe cannot be ran
again without regenerating the archive, so
ran -> run

[gab, 2012-08-17 17:01:36]

merged both comments and fixed nits

http://codereview.chromium.org/10825342/diff/1007/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/10825342/diff/1007/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:120: // move it as mentionned above;
however if it is not in |temp_path| (e.g. in
On 2012/08/17 16:46:56, robertshield wrote:
> nit: mentioned

Oops didn't realize I'd left this comment there... unified it with the one
below.

http://codereview.chromium.org/10825342/diff/1007/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:128: // that setup.exe cannot be ran
again without regenerating the archive, so
On 2012/08/17 16:46:56, robertshield wrote:
> ran -> run

Done.