Add infrastructure enabling tests of installer upgrade scenarios....

Add infrastructure enabling tests of installer upgrade scenarios.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67099

[grt (UTC plus 2), 2010-11-19 19:29:12]

This CL introduces some plumbing to make a clone of a mini_installer.exe that is
identical in all respects to the original with the exception of the version
number, which has been incremented.  This is useful for testing upgrade
scenarios.

The change to base/win/pe_image.cc fixes PEImageAsData::RVAToAddr, which has
apparently not been used before.

[tommi (sloooow) - chröme, 2010-11-19 20:14:37]

lgtm.  only nits - looks pretty awesome [positive] to me

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
File chrome/installer/test/alternate_version_generator.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:135: LPVOID data() const {
return view_; }
I think we prefer void* to LPVOID like defs (except for HANDLE)

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:186: bool
RunProcessAndWait(const wchar_t* exe_path, const std::wstring& cmdline,
Would it make sense to use the CommandLine class here?

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:210: }
maybe add a DCHECK(exit_code != STILL_RUNNING) if GetExitCode succeeds?

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:380: bool doing_great =
true;
/like

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:441:
work_dir.directory().Append(original_installer_path.BaseName());
indent

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
File chrome/installer/test/alternate_version_generator.h (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.h:7: #ifndef
CHROME_INSTALLER_TEST_ALTERNATE_VERSION_GENERATOR_H__
iirc should be only one trailing _

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
File chrome/installer/test/pe_image_resources.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
chrome/installer/test/pe_image_resources.cc:31: upgrade_test::EnumResource_Fn
function, uintptr_t context) {
function -> callback

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
chrome/installer/test/pe_image_resources.cc:50:
resource_directory->NumberOfIdEntries;
that's a pretty big initialization of scan and end.  In fact I had to read it a
couple of times before I realized there were two variables being declared and
initialized.

Can you make it a bit more readable by initializing them outside of the loop:

const IMAGE_RESOURCE_DIRECTORY_ENTRY* scan =
    reinterpret_cast<const IMAGE_RESOURCE_DIRECTORY_ENTRY*>(
        tree_base + directory_offset + sizeof(IMAGE_RESOURCE_DIRECTORY)),
const IMAGE_RESOURCE_DIRECTORY_ENTRY* end =
    scan + resource_directory->NumberOfNamedEntries +
    resource_directory->NumberOfIdEntries;

for(; success && scan != end; ++scan) {
...

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
chrome/installer/test/pe_image_resources.cc:109: bool EnumResources(const
base::win::PEImage& image, EnumResource_Fn function,
function -> callback

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
File chrome/installer/test/pe_image_resources.h (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
chrome/installer/test/pe_image_resources.h:8: #ifndef
CHROME_INSTALLER_TEST_PE_IMAGE_RESOURCES_H__
_

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
File chrome/installer/test/resource_loader.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_loader.cc:52: ResourceLoader::ResourceLoader() :
module_() {
module_(NULL)

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
File chrome/installer/test/resource_loader.h (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_loader.h:7: #ifndef
CHROME_INSTALLER_TEST_RESOURCE_LOADER_H__
_

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
File chrome/installer/test/resource_updater.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_updater.cc:15: ResourceUpdater::ResourceUpdater()
: handle_() {
handle_(NULL)

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
File chrome/installer/test/resource_updater.h (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_updater.h:7: #ifndef
CHROME_INSTALLER_TEST_RESOURCE_UPDATER_H__
_

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_updater.h:29: bool Initialize(const FilePath&
pe_image_path);
do you need to include file_path.h as well here?

if you run "gcl lint" on this change, it should tell you about some nits like
that.

[robertshield, 2010-11-19 20:56:42]

http://codereview.chromium.org/5236002/diff/1/base/win/pe_image.cc
File base/win/pe_image.cc (right):

http://codereview.chromium.org/5236002/diff/1/base/win/pe_image.cc#newcode561
base/win/pe_image.cc:561: DWORD dummy;
could we rename dummy to something else?

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
File chrome/installer/test/alternate_version_generator.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:41: // Notes:
Could we move this Notes section up to the comment above?

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:49:
L"..\\..\\third_party\\lzma_sdk\\Executable\\7za.exe";
I'm uneasy about the "..\\"s, I'd prefer if the string just has the named parts
of the path and the thing that uses the string appends the necessary "..\\s" to
use it from the right base path.

Also, could we call this k7zaRelativePath[]?

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:187: int* exit_code) {
can't you use base::LaunchApp() in process_utils.h here?

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:304: data, data + size,
for readability, these two arguments should be on separate lines.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:357: // Bumps the version
of all .exe and .dll files in |work_dir|.
update comment to reference version bumping on the directory from the archive.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:409: command_line
since this is a relative path, this code assumes it resides in an exe in The
Right Place in the world. This should be documented at the top of the file at
least, possibly in a help string.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/upgrade_test.gyp
File chrome/installer/upgrade_test.gyp (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/upgrade_test.g...
chrome/installer/upgrade_test.gyp:23: # 'mini_installer.gyp:mini_installer',
Add a TODO() here to track this down?

[grt (UTC plus 2), 2010-11-22 17:53:28]

Thanks for the comments.  I've addressed them all one way or another; see
comments for details.  PTAL.

http://codereview.chromium.org/5236002/diff/1/base/win/pe_image.cc
File base/win/pe_image.cc (right):

http://codereview.chromium.org/5236002/diff/1/base/win/pe_image.cc#newcode561
base/win/pe_image.cc:561: DWORD dummy;
On 2010/11/19 20:56:42, robertshield wrote:
> could we rename dummy to something else?

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
File chrome/installer/test/alternate_version_generator.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:41: // Notes:
On 2010/11/19 20:56:42, robertshield wrote:
> Could we move this Notes section up to the comment above?

I've deleted the notes since they are no longer relevant.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:49:
L"..\\..\\third_party\\lzma_sdk\\Executable\\7za.exe";
On 2010/11/19 20:56:42, robertshield wrote:
> I'm uneasy about the "..\\"s, I'd prefer if the string just has the named
parts
> of the path and the thing that uses the string appends the necessary "..\\s"
to
> use it from the right base path.
> 
> Also, could we call this k7zaRelativePath[]?

I've changed these a bit.  See my response to your comment in CreateArchive.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:135: LPVOID data() const {
return view_; }
On 2010/11/19 20:14:38, tommi wrote:
> I think we prefer void* to LPVOID like defs (except for HANDLE)

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:186: bool
RunProcessAndWait(const wchar_t* exe_path, const std::wstring& cmdline,
On 2010/11/19 20:14:38, tommi wrote:
> Would it make sense to use the CommandLine class here?

I'm not sure.  It looks to me like CommandLine::AppendSwitch will write switches
in "--foo" form, which is no good for expand.exe, makecab.exe, and 7za.exe.  It
looks like I could use AppendArg for everything, but that feels a bit like
square-peg-round-hole syndrome.  What do you think?

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:187: int* exit_code) {
On 2010/11/19 20:56:42, robertshield wrote:
> can't you use base::LaunchApp() in process_utils.h here?

I thought it would be nice to use CREATE_NO_WINDOW so there's no spewage.  Do
you prefer code reuse with spewage (which could be considered a feature for
humans blessed enough to watch the test in action) or no code reuse and no
spewage (which prevents logs from being filled up with many lines of
decompression/compression messages)?

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:210: }
On 2010/11/19 20:14:38, tommi wrote:
> maybe add a DCHECK(exit_code != STILL_RUNNING) if GetExitCode succeeds?

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:304: data, data + size,
On 2010/11/19 20:56:42, robertshield wrote:
> for readability, these two arguments should be on separate lines.

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:357: // Bumps the version
of all .exe and .dll files in |work_dir|.
On 2010/11/19 20:56:42, robertshield wrote:
> update comment to reference version bumping on the directory from the archive.

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:380: bool doing_great =
true;
On 2010/11/19 20:14:38, tommi wrote:
> /like

Done. :-)

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:409: command_line
On 2010/11/19 20:56:42, robertshield wrote:
> since this is a relative path, this code assumes it resides in an exe in The
> Right Place in the world. This should be documented at the top of the file at
> least, possibly in a help string.

I've done three things:
1) Documented.
2) Made the lookup of 7za.exe relative to the location of the test executable
rather than CWD.
3) Added support for a --7za_path commnd-line swtich (e.g.,
"Debug\upgrade_test.exe --7za_path=E:\src\third_party\lzma_sdk\Executable").

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:441:
work_dir.directory().Append(original_installer_path.BaseName());
On 2010/11/19 20:14:38, tommi wrote:
> indent

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
File chrome/installer/test/alternate_version_generator.h (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.h:7: #ifndef
CHROME_INSTALLER_TEST_ALTERNATE_VERSION_GENERATOR_H__
On 2010/11/19 20:14:38, tommi wrote:
> iirc should be only one trailing _

Ooops.  Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
File chrome/installer/test/pe_image_resources.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
chrome/installer/test/pe_image_resources.cc:31: upgrade_test::EnumResource_Fn
function, uintptr_t context) {
On 2010/11/19 20:14:38, tommi wrote:
> function -> callback

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
chrome/installer/test/pe_image_resources.cc:50:
resource_directory->NumberOfIdEntries;
On 2010/11/19 20:14:38, tommi wrote:
> that's a pretty big initialization of scan and end.  In fact I had to read it
a
> couple of times before I realized there were two variables being declared and
> initialized.
> 
> Can you make it a bit more readable by initializing them outside of the loop:
> 
> const IMAGE_RESOURCE_DIRECTORY_ENTRY* scan =
>     reinterpret_cast<const IMAGE_RESOURCE_DIRECTORY_ENTRY*>(
>         tree_base + directory_offset + sizeof(IMAGE_RESOURCE_DIRECTORY)),
> const IMAGE_RESOURCE_DIRECTORY_ENTRY* end =
>     scan + resource_directory->NumberOfNamedEntries +
>     resource_directory->NumberOfIdEntries;
> 
> for(; success && scan != end; ++scan) {
> ...

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
chrome/installer/test/pe_image_resources.cc:109: bool EnumResources(const
base::win::PEImage& image, EnumResource_Fn function,
On 2010/11/19 20:14:38, tommi wrote:
> function -> callback

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
File chrome/installer/test/pe_image_resources.h (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/pe_image_...
chrome/installer/test/pe_image_resources.h:8: #ifndef
CHROME_INSTALLER_TEST_PE_IMAGE_RESOURCES_H__
On 2010/11/19 20:14:38, tommi wrote:
> _

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
File chrome/installer/test/resource_loader.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_loader.cc:52: ResourceLoader::ResourceLoader() :
module_() {
On 2010/11/19 20:14:38, tommi wrote:
> module_(NULL)

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
File chrome/installer/test/resource_loader.h (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_loader.h:7: #ifndef
CHROME_INSTALLER_TEST_RESOURCE_LOADER_H__
On 2010/11/19 20:14:38, tommi wrote:
> _

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
File chrome/installer/test/resource_updater.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_updater.cc:15: ResourceUpdater::ResourceUpdater()
: handle_() {
On 2010/11/19 20:14:38, tommi wrote:
> handle_(NULL)

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
File chrome/installer/test/resource_updater.h (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_updater.h:7: #ifndef
CHROME_INSTALLER_TEST_RESOURCE_UPDATER_H__
On 2010/11/19 20:14:38, tommi wrote:
> _

Done.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/resource_...
chrome/installer/test/resource_updater.h:29: bool Initialize(const FilePath&
pe_image_path);
On 2010/11/19 20:14:38, tommi wrote:
> do you need to include file_path.h as well here?

Style guide says "Don't use an #include when a forward declaration would
suffice."  Since only a reference to FilePath is needed, the type doesn't need
to be fully-defined.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/upgrade_test.gyp
File chrome/installer/upgrade_test.gyp (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/upgrade_test.g...
chrome/installer/upgrade_test.gyp:23: # 'mini_installer.gyp:mini_installer',
On 2010/11/19 20:56:42, robertshield wrote:
> Add a TODO() here to track this down?

Done.

[tommi (sloooow) - chröme, 2010-11-22 19:43:42]

lgtm

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
File chrome/installer/test/alternate_version_generator.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:186: bool
RunProcessAndWait(const wchar_t* exe_path, const std::wstring& cmdline,
agreed. keep as is.

http://codereview.chromium.org/5236002/diff/31001/chrome/installer/upgrade_te...
File chrome/installer/upgrade_test.gyp (right):

http://codereview.chromium.org/5236002/diff/31001/chrome/installer/upgrade_te...
chrome/installer/upgrade_test.gyp:23: # TODO: fix rules/targets/etc for
mini_installer.gyp:mini_installer
TODO(grt)

[robertshield, 2010-11-22 19:59:07]

LGTM

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
File chrome/installer/test/alternate_version_generator.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:187: int* exit_code) {
On 2010/11/22 17:53:29, grt wrote:
> On 2010/11/19 20:56:42, robertshield wrote:
> > can't you use base::LaunchApp() in process_utils.h here?
> 
> I thought it would be nice to use CREATE_NO_WINDOW so there's no spewage.  Do
> you prefer code reuse with spewage (which could be considered a feature for
> humans blessed enough to watch the test in action) or no code reuse and no
> spewage (which prevents logs from being filled up with many lines of
> decompression/compression messages)?

Ideally there would be a --silent argument that could be passed to the
(de)compression command to suppress much of the spew. Even if there isn't, then
I still wouldn't want to miss having something like "Error: Out of disk space"
get printed to the logs.

[grt (UTC plus 2), 2010-11-22 20:49:23]

Comments addressed.  Will commit when I have time to monitor the builders. 
Thanks.

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
File chrome/installer/test/alternate_version_generator.cc (right):

http://codereview.chromium.org/5236002/diff/1/chrome/installer/test/alternate...
chrome/installer/test/alternate_version_generator.cc:187: int* exit_code) {
On 2010/11/22 19:59:08, robertshield wrote:
> On 2010/11/22 17:53:29, grt wrote:
> > On 2010/11/19 20:56:42, robertshield wrote:
> > > can't you use base::LaunchApp() in process_utils.h here?
> > 
> > I thought it would be nice to use CREATE_NO_WINDOW so there's no spewage. 
Do
> > you prefer code reuse with spewage (which could be considered a feature for
> > humans blessed enough to watch the test in action) or no code reuse and no
> > spewage (which prevents logs from being filled up with many lines of
> > decompression/compression messages)?
> 
> Ideally there would be a --silent argument that could be passed to the
> (de)compression command to suppress much of the spew. Even if there isn't,
then
> I still wouldn't want to miss having something like "Error: Out of disk space"
> get printed to the logs.

Done (although no such luck finding flags for the various programs to make them
silent on success).

http://codereview.chromium.org/5236002/diff/31001/chrome/installer/upgrade_te...
File chrome/installer/upgrade_test.gyp (right):

http://codereview.chromium.org/5236002/diff/31001/chrome/installer/upgrade_te...
chrome/installer/upgrade_test.gyp:23: # TODO: fix rules/targets/etc for
mini_installer.gyp:mini_installer
On 2010/11/22 19:43:42, tommi wrote:
> TODO(grt)

Done.

[Nico, 2014-04-03 18:17:49]

https://codereview.chromium.org/5236002/diff/45001/chrome/installer/upgrade_t...
File chrome/installer/upgrade_test.gyp (right):

https://codereview.chromium.org/5236002/diff/45001/chrome/installer/upgrade_t...
chrome/installer/upgrade_test.gyp:6: # 'branding_dir' is set in the 'conditions'
section at the bottom.
Are the variables up here used for anything?

[grt (UTC plus 2), 2014-04-03 19:16:10]

https://codereview.chromium.org/5236002/diff/45001/chrome/installer/upgrade_t...
File chrome/installer/upgrade_test.gyp (right):

https://codereview.chromium.org/5236002/diff/45001/chrome/installer/upgrade_t...
chrome/installer/upgrade_test.gyp:6: # 'branding_dir' is set in the 'conditions'
section at the bottom.
On 2014/04/03 18:17:49, Nico wrote:
> Are the variables up here used for anything?

I don't remember. Possibly they were needed by one of the dependent gyp files.
If the target builds without them, then I guess they aren't needed.

[robertshield, 2014-04-03 19:35:10]

https://codereview.chromium.org/5236002/diff/45001/chrome/installer/upgrade_t...
File chrome/installer/upgrade_test.gyp (right):

https://codereview.chromium.org/5236002/diff/45001/chrome/installer/upgrade_t...
chrome/installer/upgrade_test.gyp:6: # 'branding_dir' is set in the 'conditions'
section at the bottom.
On 2014/04/03 18:17:49, Nico wrote:
> Are the variables up here used for anything?

The ones above seem not to be, runhooks completes without them, I can build
without them and the code here doesn't appear to reference them. 

The ones below (msvs_use_*) appear to be referenced in common.gypi and look like
they might mutate the release build options which for this and the two
mini_installer targets are meaningful. I haven't tested to see whether they
actually do this though.