515374 - add a flag to make packager.pm missing file warnings fatal

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

After fixing bug 511642 I cleaned up all the package manifest nonsense, so all of our builds are free of packager.pm warnings. We should make the warnings fatal now so that going forward, people don't forget to update the packaging manifest if files are remove. The leftovers in the package manifest aren't really a problem, but if you removed a file and didn't update the manifet, you probably also forgot to add it to removed-files.in, and will probably break something down the line.

Comment 1

[(not currently active) Ted Mielczarek]

I'm sure Nick would appreciate this, since he bangs his head on it at every release (or maybe just every major update).

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I would love this since the MSI tools die on missing files.  I'll hit this in 511648.

Comment 4

[Joey Armstrong [:joey]]

do_copyfile(/builds/slave/try-osx64/build/obj-firefox/i386/browser/installer/../../dist/bin/chrome/browser.manifest):
 fileparse(src): '/builds/slave/try-osx64/build/obj-firefox/i386/browser/installer/../../dist/bin/chrome/ browser .manifest'
 '/builds/slave/try-osx64/build/obj-firefox/i386/browser/installer/../../dist/bin/chrome/browser.manifest' is not a directory

ERROR: Manifest file was not part of a component: srcname=[browser], srcsuffix=[.manifest], srcpath=[/builds/slave/try-osx64/build/obj-firefox/i386/browser/installer/../../dist/bin/chrome/] at /builds/slave/try-osx64/build/xpi
nstall/packager/Packager.pm line 482.

Comment 5

[Joey Armstrong [:joey]]

grep: components/*.manifest: No such file or directory
sed: can't read components/*.manifest: No such file or directory
Can't find 'chrome.manifest' in ./omni.jar
Can't find 'components/interfaces.manifest' in ./omni.jar
Can't find 'components/browser.xpt' in ./omni.jar
Can't find 'components/components.manifest' in ./omni.jar
Can't find 'greprefs.js' in ./omni.jar
Can't find 'defaults/pref/services-sync.js' in ./omni.jar

==================================================================================
  adding: js.exe        zip warning: name not matched: ../../dist/bin/jemalloc.dll
        zip warning: name not matched: ../../dist/bin/Microsoft.VC90.CRT.manifest
        zip warning: name not matched: ../../dist/bin/msvcr90.dll
 (172 bytes security) (deflated 60%)
  adding: nspr4.dll (172 bytes security) (deflated 59%)
e:/builds/moz2_slave/try-w64/build/obj-firefox/config/nsinstall.exe -D ../../dist/
Compressing...
cd ../../dist && (cd firefox && rm -f omni.jar components/binary.manifest && grep -h '^binary-component' components/*.manifest > binary.manifest ; for m in components/*.manifest; do sed -e 's/^binary-component/#binary-component/' $m > tmp.manifest && mv tmp.manifest $m; done; zip -r9m omni.jar chrome chrome.manifest components/*.js components/*.xpt components/*.manifest modules res defaults greprefs.js jsloader  -x chrome/icons/\* defaults/pref/channel-prefs.js res/cursors/\* res/MainMenu.nib/\*  &&  e:/builds/moz2_slave/try-w64/build/obj-firefox/dist/bin/xpcshell.exe -g "$PWD" -a "$PWD" -f /e/builds/moz2_slave/try-w64/build/toolkit/mozapps/installer/precompile_cache.js -e "populate_startupcache('GreD', 'omni.jar', 'startupCache.zip');" && rm -rf jsloader && unzip startupCache.zip && rm startupCache.zip && zip -r9m omni.jar jsloader/resource/gre && c:/mozilla-build/python/python2.6.exe /e/builds/moz2_slave/try-w64/build/config/optimizejars.py --optimize /e/builds/moz2_slave/try-w64/build/obj-firefox/browser/installer/../../jarlog//en-US ./ ./ && mv binary.manifest components && printf "manifest components/binary.manifest\n" > chrome.manifest) && (cd firefox && c:/mozilla-build/python/python2.6.exe /e/builds/moz2_slave/try-w64/build/config/createprecomplete.py) && zip -r9D firefox-9.0a1.en-US.win64-x86_64.zip firefox
        zip warning: name not matched: jsloader
  adding: chrome/ (260 bytes security) (stored 0%)
  adding: chrome/browser/ (260 bytes security) (stored 0%)

[ SNIP ]

grep: components/*.manifest: No such file or directory
sed: can't read components/*.manifest: No such file or directory
Can't find 'chrome.manifest' in ./omni.jar
Can't find 'components/interfaces.manifest' in ./omni.jar

Comment 6

[Joey Armstrong [:joey]]

Attachment: Handle several packaging problems as an error, exclusion list for legacy warnings.

o Converted several packaging problems reported as warning into an error.

o Added an exclusion list populated with legacy warnings.  New errors will
  force a failure, legacy will continue to warn until cleaned up.

o Std modules added, replaced require.pl with 'use require'.  Added version strings and function export lists.

o Helper functions added to track context while parsing the manifest (abs_path_to_manifest, component, etc) so the information can be displayed when errors are reported -- dieWithContext().

o Report file and directory paths with unlink/rmtree/etc failures to document what element(s) are having a problem and where.

o Minor parser cleanups, skip blank lines and comments that span a line.
  Declare local vars when handling file paths to shorten long lines.
  Begin transitioning from global use to parameter passing in do_context()
  and a few other places.  Display chmod mode in octal rather than decimal.

o Added basic perldoc module documentation.

o Unit tests added for Packager Copy() and a few of the basic functions.
  Use of global vars in the module with recursive functions has made testing tricky.  Added Packager::testhelper() as a hack for explicitly setting package vars to increase coverage.

Comment 7

[Joey Armstrong [:joey]]

(In reply to Joey Armstrong [:joey] from comment #5)
> grep: components/*.manifest: No such file or directory
> sed: can't read components/*.manifest: No such file or directory
> Can't find 'chrome.manifest' in ./omni.jar
> Can't find 'components/interfaces.manifest' in ./omni.jar

https://bugzilla.mozilla.org/show_bug.cgi?id=690337

rm: WARNING: Circular directory structure.
This almost certainly means that you have a corrupted file system.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Not sure if this has been mentioned, but we'll want whether or not errors are fatal to be controllable on a per-application basis (so that Seamonkey et al can turn it off.)

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #8)

Wrt SeaMonkey (at least), the goal should be to enable this new behavior there too.
To be able to disable it couldn't hurt.
But a better/expected solution would be to have the exclusion list configurable at a (Core +) per-app level, just like existing package-manifest.in (and removed-files.in.)

Comment 10

[Joey Armstrong [:joey]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #8)
> Not sure if this has been mentioned, but we'll want whether or not errors
> are fatal to be controllable on a per-application basis (so that Seamonkey
> et al can turn it off.)

The exclusion list was only intended to be temporary so the script changes could be worked in.  If it will be useful longer term for fixing temporary manifest problems that should be easy enough to support.  Are there any existing config files that could be used as a template ( tokens and name case ) ?

--no-exclusions could also be added to make all warnings fatal.

Comment 11

[Joey Armstrong [:joey]]

(In reply to Serge Gautherie (:sgautherie) from comment #9)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #8)
> 
> Wrt SeaMonkey (at least), the goal should be to enable this new behavior
> there too.
> To be able to disable it couldn't hurt.
> But a better/expected solution would be to have the exclusion list
> configurable at a (Core +) per-app level, just like existing
> package-manifest.in (and removed-files.in.)

Scratch my last question if package-manifest can be used as a template.  So should deployment be blocked on configurability or start reporting new errors now and add component based filtering later on ?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The failure scenario here is the following:

fatal packaging errors is turned on for Firefox and Seamonkey.

Somebody makes a change to the packaging of some shared file, and changes the Firefox package-manifest, but forgets to change the Seamonkey package-manifest.

Firefox builds and Seamonkey doesn't.

I don't see how filtering helps, because if you knew which changes to make to Seamonkey's filter list you'd just make those changes to the package-manifest. Seamonkey needs a switch it can set that says "please don't fail the build because some Firefox dev forgot to update my package-manifest".

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to Joey Armstrong [:joey] from comment #10)
> --no-exclusions could also be added to make all warnings fatal.

Or a "--report-as-errors" depending on how you plan to deploy this without breaking any app.


(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)

> Somebody makes a change to the packaging of some shared file, and changes
> the Firefox package-manifest, but forgets to change the Seamonkey
> package-manifest.

I think it's a little more complicated than that:
*I don't expect [thought I'd really love that] Core devs to update (synchronously) all non-Firefox projects (SeaMonkey, Thunderbird, ...).
*A similar case is MailNews Core being updated by Thunderbird before SeaMonkey.

My past experience (with SeaMonkey, and others) is that:
*having error report will be a major help to notice changes.
*but the actual change/port may take some time (find a volunteer, figure out the consequences, wait for review, ...).
*Thus, in that meantime, I would imagine other apps could have a blanket process like "build breaks, someone files a bug and immediately updates exclusion list, then the bug is processed as usual". [And I'm thinking about various cases in which this could help, not just the simple scenario of trivial copy from Firefox to SeaMonkey with everyone around.]

> I don't see how filtering helps, because if you knew which changes to make
> to Seamonkey's filter list you'd just make those changes to the
> package-manifest.

In other words, if even Firefox (or Fennec) needs an exclusion list in order to fix this harness bug, rather than actually fixing all these warnings first, then I don't understand how you can assume other apps will never need (anymore) such a feature.

> Seamonkey needs a switch it can set that says "please
> don't fail the build because some Firefox dev forgot to update my
> package-manifest".

That "switch" should be the exclusion list.

Comment 14

[Joey Armstrong [:joey]]

Attachment: handle packge warnings as an err, updated exclusion list

Added more file exclusions so try builds could succeed.

Comment 15

[Mike Taylor [:bear]]

Comment on attachment 564229 [details] [diff] [review]
handle packge warnings as an err, updated exclusion list

big patch, my first pass isn't finding anything that is making me go WTF! but I'm also seeing a lot of code that I feel needs to be excersized before I feel comfortable with +1'ing it

is the unittest shown at the end functional and has it been run?

Comment 16

[Joey Armstrong [:joey]]

(In reply to Mike Taylor [:bear] from comment #15)
> Comment on attachment 564229 [details] [diff] [review] [diff] [details] [review]
> handle packge warnings as an err, updated exclusion list
> 
> big patch, my first pass isn't finding anything that is making me go WTF!
> but I'm also seeing a lot of code that I feel needs to be excersized before
> I feel comfortable with +1'ing it
> 
> is the unittest shown at the end functional and has it been run?

Yes the unit tests are functional.  You can either call "make check" from the object directory or cd into test/ and run the *.tpl, *.tpm scripts directly.

Comment 17

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

Comment on attachment 564229 [details] [diff] [review]
handle packge warnings as an err, updated exclusion list

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

::: xpinstall/packager/Packager.pm
@@ +106,5 @@
> +    ## Determine path to the exclusion file based on @INC + cwd().
> +    ##
> +    ## This hack is in place work around direct module access by the
> +    ## installer.  Makefiles invoke the Copy subroutine directly as
> +    ## perl -e 'use Packager; Copy(...)' so we have no scirpt init function

Typo: scirpt

@@ +197,5 @@
> +            # FMT: component ^ path
> +            my @fields = split(/\s*\^\s*/o);
> +            next unless ($fields[1]); 
> +            $fields[0] =~ s/^\s+//o;
> +            $fields[1] =~ s%/\?s*$%%o; # snip trailing slash and whitespace

Could we do all this in a single regexp?

@@ +317,5 @@
> +  {
> +    ## Unbuffer output so die/errors string can be displayed
> +    ## alongside processing output that generated them
> +    select((select(STDOUT), $|=1)[0]);
> +    select((select(STDERR), $|=1)[0]);

Is there ever a circumstance where we wouldn't want to set $| > 0, even when not running at a higher debug level?

@@ +948,5 @@
> +* pkg    name of package-manifest file to read in
> +* os     [MSDOS|UNIX] - Path separator type
> +* flat   if set do not copy files into a subdirectory named for the component.
> +* help   display usage
> +* debug  enable debug mode at level [n]

Where are the various debug levels defined?

::: xpinstall/packager/test/Packager.tpm
@@ +307,5 @@
> +check_do_copyfile();
> +check_do_wildcard();
> +check_do_copydir();
> +check_do_component();
> +check_check_arguments();

Do we see adding further checks to this list? If so, just wondering whether we could make it easier to iterate over a list of funcs for this.

Comment 18

[Joey Armstrong [:joey]]

(In reply to Chris Cooper [:coop] from comment #17)
> Comment on attachment 564229 [details] [diff] [review] [diff] [details] [review]
> handle packge warnings as an err, updated exclusion list
> 
> Review of attachment 564229 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> @@ +197,5 @@
> > +            # FMT: component ^ path
> > +            my @fields = split(/\s*\^\s*/o);
> > +            next unless ($fields[1]); 
> > +            $fields[0] =~ s/^\s+//o;
> > +            $fields[1] =~ s%/\?s*$%%o; # snip trailing slash and whitespace
> 
> Could we do all this in a single regexp?

Two distinct fields are having whitespace removed.  If a third delimited field is added in the config file (/^\s*(.+)\s*$/ -- /\s*$/ would no longer be able to prune the 2nd value.


> @@ +317,5 @@
> > +  {
> > +    ## Unbuffer output so die/errors string can be displayed
> > +    ## alongside processing output that generated them
> > +    select((select(STDOUT), $|=1)[0]);
> > +    select((select(STDERR), $|=1)[0]);
> 
> Is there ever a circumstance where we wouldn't want to set $| > 0, even when
> not running at a higher debug level?

There is a small performance hit for unbuffered output but legible output but useful error messages would probably be more helpful here.


> 
> @@ +948,5 @@
> > +* pkg    name of package-manifest file to read in
> > +* os     [MSDOS|UNIX] - Path separator type
> > +* flat   if set do not copy files into a subdirectory named for the component.
> > +* help   display usage
> > +* debug  enable debug mode at level [n]
> 
> Where are the various debug levels defined?

They are simply increasing levels no specific functionality tied to an option.  --debug 99can display command output while --debug 1 would show minimal processing output.


> ::: xpinstall/packager/test/Packager.tpm
> @@ +307,5 @@
> > +check_do_copyfile();
> > +check_do_wildcard();
> > +check_do_copydir();
> > +check_do_component();
> > +check_check_arguments();
> 
> Do we see adding further checks to this list? If so, just wondering whether
> we could make it easier to iterate over a list of funcs for this.

A loop could be setup and eval{ &func } used -- could even take it one step farther and dynamically build the list with grep(/^sub /).  Probably not worth the added complexity since the unit tests are fairly simple.

Comment 19

[Joey Armstrong [:joey]]

Attachment: handle manifest problems as an error. Special case for TB and SM, containue to report warnings.

https://tbpl.mozilla.org/?tree=Try&rev=1c8721bb3159

Unbuffer output per Coop's nit list.

Search for .mozconfig and disable manifest error handling when downstream projects are detected.  This will unblock general error detection, exclusions for thunderbird and seamonkey can be handled separately.

Comment 20

[Joey Armstrong [:joey]]

(In reply to Joey Armstrong [:joey] from comment #19)
> Created attachment 571063 [details] [diff] [review] [diff] [details] [review]
> handle manifest problems as an error.  Special case for TB and SM, containue
> to report warnings.

Unit test updated to verify manifest problems are handled as an error for firefox and warnings for thunderbird and seamonkey.

Comment 21

[Joey Armstrong [:joey]]

Attachment: handle manifest problems as an error. Setup separate exclusion files for SM & TB

Logic from last patch:
  o FF manifest problems will force an error when detected unless files are present on the exclusion list.
  o Detect build type from the sandbox .mozconfig file.  When a SM -or- TB build is detected revert behavior to only report manifest problems as a warning rather than error condition.
  o A few file changes made in Packages.excl.

New addition:
  o Added build specific exclusion files for Seamonkey and Thunderbird.
  o Files currently are empty but should be populated for the next patch.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 574341 [details] [diff] [review]
handle manifest problems as an error.  Setup separate exclusion files for SM & TB

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

This is far too complex.  Sid has a better approach.

Comment 23

[Rain [:rain1] (inactive)]

Attachment: [checked in] add a flag to make file missing warnings fatal

Does what it says. I tested this with trying to build the Thunderbird installer with the var set to 1. It errored out, so looks like it worked great.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 583931 [details] [diff] [review]
[checked in] add a flag to make file missing warnings fatal

r=me

Comment 25

[Rain [:rain1] (inactive)]

Waiting for buildbot issues to be resolved before pushing this to b-s.

Comment 26

[Rain [:rain1] (inactive)]

Comment on attachment 583931 [details] [diff] [review]
[checked in] add a flag to make file missing warnings fatal

https://hg.mozilla.org/projects/build-system/rev/cc4d4c230ec1

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/cc4d4c230ec1