554752 - Post-repack version check

Status: RESOLVED WONTFIX

(Release Engineering :: General, defect, P3)

Description

[Nick Thomas [:nthomas] (UTC+12)]

(from the 3.6.2 problem, bug 554232)

We should check the versions used in repacks after repacking each locale. That means
* verifying application.ini (which should just be carried over from the en-US build but it's worth checking)
* for windows, verifying the versions in setup.exe and/or helper.exe

We should do this before signing so as to fail out asap.

Comment 1

[Chris Cooper [:coop] (he/him)]

Assigned during triage; if you think this is better assigned to someone else, please swap with them for another old bug.

joey: welcome to releng! bhearsum and/or myself can talk you through what needs to be added here (and where).

Comment 2

[Joey Armstrong [:joey]]

if either of you have time tomorrow can I get a braindump of what will be needed for the "verify locale repack" logic.

Comment 3

[Joey Armstrong [:joey]]

ping: whenever someone has time to go over these bug details.

Comment 4

[Joey Armstrong [:joey]]

bhearsum        joey: i'm just getting myself up to speed on that bug now
bhearsum        joey: so, it looks to me like there's two things to be done
bhearsum        1) after the installers-% target is finished, pull out application.ini from the tarball/dmg/exe and compare the version in it to the expected one
bhearsum        2) on windows, compare the "product version" to the expected one
bhearsum        i'm not sure whether that code should go in the build system or in a separate script
bhearsum        i'm also not sure how to get the "product version" (which i can see in the Properties dialog of a file) from the command line - there's probably some tool that lets you though
bhearsum        it's probably worth asking ted and/or pike whether or not any of that belongs in browser/locales/Makefile.in

Comment 5

[Joey Armstrong [:joey]]

bhearsum	this post has a few different methods that might work: http://www.tech-archive.net/Archive/Windows/microsoft.public.windows.server.general/2006-03/msg00518.html

bhearsum: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#720 might be a good place to call your script
bhearsum: that'd avoid any duplication
Callek: bhearsuM: of course packager.mk is used for installer/ too, if we only care about locales/ I10n.mk is probably a better place.

Comment 6

[Joey Armstrong [:joey]]

Commands launched externally during repackaging:

sh -- ./configure --enable-application=browser --with-l10n-base=../releases/l10n/mozilla-aurora --enable-update-packaging
make wget-en-US
make unpack
make ident
sh -c make installers-de LOCALE_MERGEDIR=$PWD/merged

Might be able to move them into the makefile and/or pass flags to enable actions so all of the processing logic could reside within the makefile.

Comment 7

[Joey Armstrong [:joey]]

Question: where should this script live: build/, config/, tools/, ?

Comment 8

[Joey Armstrong [:joey]]

Attachment: A script for archive version string extraction and comparison

Initial pass: supported on cygwin, linux, mac, mingw
Unit test: verify extraction from text files and tarballs.

Pass in a list of file paths and/or arguments to use for checking.
[li|u]nux: tarball extraction
      mac: mount a dmg disk image and find application.ini
      win: win32api::GetFileVersionInfo(foo.exe)

The tool will verify a version string can be found.
If multiple files are passed verify version strings match.

browser/* makefile will be setup to invoke this tool and pass in application.ini and path to the .dmg, tar archive for checking.

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 552739 [details] [diff] [review]
A script for archive version string extraction and comparison

Review of attachment 552739 [details] [diff] [review]:
-----------------------------------------------------------------

A couple style nits:
- Use '%s foo' % bar style string construction instead of bar + 'foo'
- Put variables on the left hand side of comparisons, and don't bother with the parenthesis: if foo != 'bar' instead of if ('bar' != foo)

It doesn't look like integration with the build system is finished yet, based on http://tinderbox.mozilla.org/showlog.cgi?log=Try/1313178541.1313191713.8932.gz&fulltext=1, I'd like to see a try run once that's ready.

Ted should have a look at this, too.

::: toolkit/mozapps/installer/checkFileVersion.py
@@ +43,5 @@
> +
> +Usage: checkFileVersion.py <filename> <entry> [<entry> ...]
> +'''
> +
> +import getopt

We usually use optparse for any non-trivial things, can you use that instead?

@@ +55,5 @@
> +
> +# import win32api
> +# win32pipe::popen('....')
> +
> +ARGV = { 'debug': 0, 'verbose': 0, 'verfile': None, 'version': 0.0 }

I'd prefer to see this passed into functions rather than globally available.

@@ +120,5 @@
> +## -----------------------------------------------------------------------
> +## Note: Could search for '[' + '[App]' as anchors to better verify the
> +##       version string match but let's keep parsing simple for now.
> +###########################################################################
> +def getVersionString(path):

Have you tried using ConfigParser for this? I think it would work here.

@@ +177,5 @@
> +    if ('darwin' != arch):
> +        print "getVerFromDMG: Is not currently supported on " + arch
> +        return None
> +
> +    if ('linux' == arch):

This seems redundant after the arch != 'darwin' check

@@ +203,5 @@
> +        ver = getVersionString(path)
> +        if (ver):
> +            break
> +
> +    cmd = 'hdiutil detach -quiet ' + tmpdir # + '-force'

This needs to go in a finally: block -- otherwise it'll stay mounted in error cases.

@@ +265,5 @@
> +##   o unmount the disk image
> +###########################################################################
> +def getVerFromFILE(src):
> +
> +    if (ARGV['debug']):

You're doing a lot of these checks, you might be better off using the logging module. Up to you.

@@ +313,5 @@
> +        for path in todo:
> +            ver = getVersionString(path)
> +            if (ver): break
> +
> +    if (os.path.exists(tmpdir)):

Same thing here re: cleanup - use a finally: block

@@ +322,5 @@
> +
> +###########################################################################
> +## Intent: Display program usage
> +###########################################################################
> +def usage():

I don't see --ini documented here, can you add that?

Comment 10

[Joey Armstrong [:joey]]

(In reply to Ben Hearsum [:bhearsum] from comment #9)
> Comment on attachment 552739 [details] [diff] [review]
> A script for archive version string extraction and comparison
> 
> Review of attachment 552739 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A couple style nits:
> - Put variables on the left hand side of comparisons, and don't bother with
> the parenthesis: if foo != 'bar' instead of if ('bar' != foo)

Is this a coding standard or a nice to have ?  Dropping parens will create a little more work when switching between languages having to remember to remove parens specifically for python.  Also too many parens are an eyesore but one set can be a visual cue for the presence of a conditional.


> ::: toolkit/mozapps/installer/checkFileVersion.py
> @@ +43,5 @@
> > +
> > +Usage: checkFileVersion.py <filename> <entry> [<entry> ...]
> > +'''
> > +
> > +import getopt
> 
> We usually use optparse for any non-trivial things, can you use that instead?

The optparse module has been deprecated in v2.7, is this a pending issue ?
A replacement 'argparse' module is available but ./configure only requires v2.5 at a minimum so the module may not be present.  I can port this to using optparse/argparse but it will require a version specific import with two code paths.  Seemed cleaner to just import and use the portable module:

http://docs.python.org/library/optparse.html


> @@ +120,5 @@
> > +## -----------------------------------------------------------------------
> > +## Note: Could search for '[' + '[App]' as anchors to better verify the
> > +##       version string match but let's keep parsing simple for now.
> > +###########################################################################
> > +def getVersionString(path):
> 
> Have you tried using ConfigParser for this? I think it would work here.

I'll have a look at the module


> @@ +177,5 @@
> > +    if ('darwin' != arch):
> > +        print "getVerFromDMG: Is not currently supported on " + arch
> > +        return None
> > +
> > +    if ('linux' == arch):
> 
> This seems redundant after the arch != 'darwin' check

This was only a placeholder to note that dmg disk images could be mounted under linux with some extra work.  Could either comment out the code or remove the code and simply make note of it in the function docs.


> @@ +203,5 @@
> > +        ver = getVersionString(path)
> > +        if (ver):
> > +            break
> > +
> > +    cmd = 'hdiutil detach -quiet ' + tmpdir # + '-force'
> 
> This needs to go in a finally: block -- otherwise it'll stay mounted in
> error cases.

Good catch, to be added.


> > +###########################################################################
> > +## Intent: Display program usage
> > +###########################################################################
> > +def usage():
> 
> I don't see --ini documented here, can you add that?

This should be --verfile, the argument name was changed but not all references to it were removed from the getopt block.

Comment 11

[Joey Armstrong [:joey]]

Attachment: checkFileVersion.py + Fixes for Ben's nit list

Archiving fixes, makefile edits are still needed.
Replaced getopts with optparse, can deal with argparse use after 2.7 becomes the default.
Removed ARGV global and added calls to the logging module.
Replaced getVersion() internals with a call to ConfigParser()
Removed raise from getVersion's try/catch, return None to force a string check failure.  Helps avoid a long grody try/catch block for unmounting a dmg image.

Comment 12

[Joey Armstrong [:joey]]

Todo - also support checking apk, rpm and 7z images

Comment 13

[Joey Armstrong [:joey]]

existing l10n unit test: make -f browser/build.mk l10n-check

Comment 14

[Joey Armstrong [:joey]]

Attachment: alpha: makefile changes + more unit test coverage

NOTE: testing makefile targets is still pending

target "package-check-version" added as the trigger for performing a version check of a generated package.  Exit status for package checking and the unit test will be ignored initially until platform status and nightly build interactions are known.

checkFileVersion.py: inlined shell commands replaced with modules: tarfile, zipfile
Unit test: added more data files for validating: apk, dmg & zip packages

Comment 15

[Axel Hecht [:Pike]]

I have to admin, I don't understand the makefile foo here.

Is there something special about ^ ?

Comment 16

[Axel Hecht [:Pike]]

admit, even.

Comment 17

[Joey Armstrong [:joey]]

(In reply to Axel Hecht [:Pike] from comment #15)
> I have to admin, I don't understand the makefile foo here.
> 
> Is there something special about ^ ?

In a nutshell when a repack is requested, the target '*^package-check-version' will be appended to the dependency chain.  This will trigger a version check as the last step before the repack is considered successful.

There is nothing significant about the '^' delimiter aside from it being unique.

The target format "$(version)^$(path)^package-check-version" was used to:
  o Create a unique target name that can be processed by a wildcard target.
  o Name is unique enough to act as a ".PHONY" target.  This will force package-check-version to always be invoked.
  o The caret delimited fields are used as a form of argument passing within a makefile.  Values are passed in (delimited) as part of the target name.  Then while a target is processing, split & extract positional parameters as needed.

Comment 18

[Chris Cooper [:coop] (he/him)]

Comment on attachment 554477 [details] [diff] [review]
alpha: makefile changes + more unit test coverage

Review of attachment 554477 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good so far. You seem to have address most of Ben's concerns about checkFileVersion.py in this new version.

How much more work is still to do before you'll be ready for formal review? More unittests? Testing?

::: toolkit/locales/l10n.mk
@@ +53,5 @@
>  #   for both packages and language packs.
>  # installer-%
>  #   This target should list all required targets, a typical rule would be
> +#	installers-%: clobber-% langpack-% repackage-zip-%\
> +#         ${ver}^${path}%package-check-version\

Does this example actually match the rule below, i.e. %^package-check-version? (my makefile fu is weak)

::: toolkit/mozapps/installer/Makefile.in
@@ +42,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +# MODULE = install
> +
> +ifdef ENABLE_TESTS

So this Makefile is needed just so we can get down into the tests dir when required?

::: toolkit/mozapps/installer/checkFileVersion.py
@@ +513,5 @@
> +
> +##############################################################################
> +# Enhancements:
> +#   o Support and test on more platforms
> +#   o gzip and bzip2 tarballs handled, are there others ? [ l7, zip, ... ]

Those are the only ones we support on linux. We create a 7-zip package on Windows, but unpacking the installer(exe) to check is preferred.

Comment 19

[Joey Armstrong [:joey]]

Attachment: post repack version check, improved platform handling, more tests added

makefile target package-check-version can now accept a list of files to
verify version information for.  application.ini is the default filename
to be checked, firefox.exe will also be passed for windows platforms.

Added more user defined macros to more clearly document the argument
manipulation logic.

tarfile module will trigger an __exit__ attribute not defined exception
for python version < 2.7.  Added a version check and import contextlib
/closing to correct the problem for pre-2.7 python installations.

Added more sample data files for unit testing by all platforms.

Added helper script checkPackageVersion as a first step toward automated
testing.  Script can be invoked passing in a directory path or URI
(http://nightly.mozilla.org).  Packages will be dowloaded, gathered,
unpacked then checked.  Platform conditionals prevent checking invalid
packages (dmg/not mac), exe/not win).  Makefile check target can
eventually be written to invoke cPV on the dist/ directory.

Verified against nightly packages and several on the try server.

Comment 20

[Joey Armstrong [:joey]]

(In reply to Chris Cooper [:coop] from comment #18)
> Comment on attachment 554477 [details] [diff] [review]
> alpha: makefile changes + more unit test coverage
> 
> Review of attachment 554477 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good so far. You seem to have address most of Ben's concerns about
> checkFileVersion.py in this new version.
> 
> How much more work is still to do before you'll be ready for formal review?
> More unittests? Testing?

The patch is ready for prime time.  More sample data files were added for unit tests.  A script was added to download packages in bulk from nightly and verify checking on multiple platforms.

 
> ::: toolkit/locales/l10n.mk
> @@ +53,5 @@
> >  #   for both packages and language packs.
> >  # installer-%
> >  #   This target should list all required targets, a typical rule would be
> > +#	installers-%: clobber-% langpack-% repackage-zip-%\
> > +#         ${ver}^${path}%package-check-version\
> 
> Does this example actually match the rule below, i.e.
> %^package-check-version? (my makefile fu is weak)

Yes, a few make macros have since been added to encapsulate argument handling and make the logic self-documenting.


> ::: toolkit/mozapps/installer/Makefile.in
> @@ +42,5 @@
> > +include $(DEPTH)/config/autoconf.mk
> > +
> > +# MODULE = install
> > +
> > +ifdef ENABLE_TESTS
> 
> So this Makefile is needed just so we can get down into the tests dir when
> required?

Yes, a requirement for recursive makefile traversal.

 
> ::: toolkit/mozapps/installer/checkFileVersion.py
> @@ +513,5 @@
> > +
> > +##############################################################################
> > +# Enhancements:
> > +#   o Support and test on more platforms
> > +#   o gzip and bzip2 tarballs handled, are there others ? [ l7, zip, ... ]
> 
> Those are the only ones we support on linux. We create a 7-zip package on
> Windows, but unpacking the installer(exe) to check is preferred.

The makefile checking target will now accept a list of files to check within an archive.
application.ini is the default.  firefox.exe will be passed in for windows.

Comment 21

[Joey Armstrong [:joey]]

Comment  9 - bug review by Ben H
Comment 11 - new patch submitted
Comment 15 - Pike's question about caret use
Comment 17 - caret use details
Comment 18 - Code review by Coop
Comment 19 - latest patch submission, test data files added along with a script to automate checking application.in/ff.exe version strings in bulk.

Comment 22

[Joey Armstrong [:joey]]

http://hg.mozilla.org/try/pushloghtml?changeset=05bc404a9cdb

Comment 23

[Joey Armstrong [:joey]]

Attachment: Post-repack version check

cosmetic script changes.  Replaced function comment headers with python doc strings. Cleaned up warnings reported by pylint -- long lines, multiple statements on a single line, etc.

Comment 24

[[e-mail] (:bhearsum)]

Mass move of bugs to Release Automation component.

Comment 25

[Joey Armstrong [:joey]]

Attachment: Verify repack version strings.

* * *
try: -e -u all -p all -b do -t none

Comment 26

[Joey Armstrong [:joey]]

Attachment: Verify repack version strings.

* * *
try: -e -u all -p all -b do -t none

Comment 27

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Found in triage.

Comment 28

[Michael Shal [:mshal]]

This may fall under the l10n work that Callek is doing, if it's still relevant.

Comment 29

[Aki Sasaki (not active)]

12 year old bug. I think we haven't hit this sort of issue any time in recent memory; we have additional checks; and ideally repacks will go away before another 12 years pass.