Last Comment Bug 734125 - Nonexistent file mochitest/globalstorage/Makefile.in listed in toolkit-makefiles.sh
: Nonexistent file mochitest/globalstorage/Makefile.in listed in toolkit-makefi...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks: 687579
  Show dependency treegraph
 
Reported: 2012-03-08 08:38 PST by Joey Armstrong [:joey]
Modified: 2012-06-25 00:46 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add check to detect non-existent makefiles in *makefiles.sh (11.35 KB, patch)
2012-05-25 08:36 PDT, Joey Armstrong [:joey]
mh+mozilla: review-
Details | Diff | Splinter Review
PoC (1.55 KB, patch)
2012-05-30 10:14 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Fail configure when acoutputfast.pl fails to find one of the input files (6.24 KB, patch)
2012-05-30 10:52 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2012-03-08 08:38:53 PST
1) dom/tests/mochitests/globalstorage/Makefile.in is mia
2) Failure status not reported
3) Setup a check unit test that will exercise all permutations of the makefile-list scripts:
   allmakefiles.sh
   toolkit/toolkit-makefiles.sh
   [snip]

   Under set -e control and will exit with failure when files are missing or commands barf.  This should help ensure the list is changed alongside filesystem deletions or renames.

Try job: 1968a55a70f5
Source changes: config/rules.mk contains nop edits -- comments suffixed at EOF
Failures detected.

https://tbpl.mozilla.org/?tree=Try&rev=1968a55a70f5
  try: -b do -e -p all -u all -t none 

1968a55a70f5/try-android-debug-build739.txt.gz
==============================================
can't read /builds/slave/try-andrd-dbg/build/dom/tests/mochitests/globalstorage/Makefile.in: No such file or directory

1968a55a70f5/try-linux-build795.txt.gz
======================================
can't read /builds/slave/try-lnx/build/dom/tests/mochitest/globalstorage/Makefile.in
Comment 1 Ted Mielczarek [:ted.mielczarek] 2012-03-08 13:18:10 PST
I think we wanted to use "set -e" at some point, but I can't remember why we didn't. Ed has been taking care of most of these recently, maybe he remembers.
Comment 2 Ed Morley [:emorley] 2012-03-08 13:31:36 PST
We |set -o errexit| inside allmakefiles.sh (bug 703930), but that only covers syntax errors in the construction of the MAKEFILES variable in the various sub-scripts.

To prevent the "makefile X not found" type of error, we'd need to |set -o errexit| in configure. In an ideal world, we'd be doing it for all of configure, but that would require fixing up the rest of the file (see bug 698545 comment 1), so perhaps at least for now perhaps just wrapping:
> echo $MAKEFILES | ${PERL} $srcdir/build/autoconf/acoutput-fast.pl > conftest.sh

...might be the quickest solution?
Comment 3 Mike Hommey [:glandium] 2012-03-08 13:46:15 PST
Wouldn't it make sense to have a mercurial hook checking allmakefiles.sh (and friends) don't contain directories that don't exist anymore?
Comment 4 Joey Armstrong [:joey] 2012-03-12 10:53:06 PDT
(In reply to Ed Morley [:edmorley] from comment #2)
> We |set -o errexit| inside allmakefiles.sh (bug 703930), but that only
> covers syntax errors in the construction of the MAKEFILES variable in the
> various sub-scripts.
> 
> To prevent the "makefile X not found" type of error, we'd need to |set -o
> errexit| in configure. In an ideal world, we'd be doing it for all of
> configure, but that would require fixing up the rest of the file (see bug
> 698545 comment 1), so perhaps at least for now perhaps just wrapping:
> > echo $MAKEFILES | ${PERL} $srcdir/build/autoconf/acoutput-fast.pl > conftest.sh
> 
> ...might be the quickest solution?

How about adding new check tests for individual make*.sh scripts and have them validate only paths the script(s) are able to broadcast ?  Ignore conditionals, extract a full list of files, loop and verify existance.  The only caveats should be the test would need to run regularly not just when the dependent script is changed.  Also what to do with unexpanded variables in some of the paths...

The commit hook suggestion would be good as another filter to keep out garbage.  I would also like to see a unit test in place so people have a chance to fix the problem early, prior to a submitting.
Comment 5 Daniel Holbert [:dholbert] 2012-03-22 22:03:08 PDT
FWIW, the cset that removed the globalstorage makefile (w/out updating toolkit-makefiles.sh) was http://hg.mozilla.org/integration/mozilla-inbound/rev/7c62688f88fa for bug 687579.

I took the liberty of updating toolkit-makefiles.sh, with the (last I heard) blanket-rs=ted on removing missing Makefiles from that file:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2296f477cb

It sounds like this bug has partly morphed into "how can we make this sort of thing break the build" -- is that correct?  If so, we can leave this open after merging until that's addressed.
Comment 6 Daniel Holbert [:dholbert] 2012-03-22 22:04:52 PDT
(In reply to Daniel Holbert [:dholbert] from comment #5)
> It sounds like this bug has partly morphed
(er s/has partly morphed/is primarily about/)
Comment 7 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-03-23 05:59:52 PDT
https://hg.mozilla.org/mozilla-central/rev/dc2296f477cb
Comment 8 Joey Armstrong [:joey] 2012-05-25 08:36:49 PDT
Created attachment 627243 [details] [diff] [review]
add check to detect non-existent makefiles in *makefiles.sh
Comment 9 Joey Armstrong [:joey] 2012-05-25 08:41:10 PDT
Comment on attachment 627243 [details] [diff] [review]
add check to detect non-existent makefiles in *makefiles.sh

Added a 'make check' unit test able to search for Makefile.in deletions that were made w/o removing the corresponding entry in *makefiles.sh.

Also repaired the current batch of problem entries reported:
============================================================
ERROR: content/xbl/public/Makefile does not exist
ERROR: dom/system/b2g/Makefile does not exist
ERROR: dom/system/cocoa/Makefile does not exist
ERROR: xulrunner/app/profile/extensions/Makefile does not exist
ERROR: xulrunner/installer/mac/Makefile does not exist
Comment 10 Joey Armstrong [:joey] 2012-05-25 14:38:08 PDT
Try job: https://tbpl.mozilla.org/?tree=Try&rev=f724c409a904
Comment 11 Joey Armstrong [:joey] 2012-05-30 06:11:21 PDT
ping on code review
Comment 12 Mike Hommey [:glandium] 2012-05-30 08:18:04 PDT
Comment on attachment 627243 [details] [diff] [review]
add check to detect non-existent makefiles in *makefiles.sh

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

::: config/tests/makefiles/allmakefiles/checkAllMakefiles.sh
@@ +15,5 @@
> +
> +###########################################################################
> +## Intent: Gather a list of script files to examine
> +###########################################################################
> +function gather_sh

I think both this function and gather_mk are too much of a parsing hack. There are some "makefiles" that aren't Makefiles, and while probably not true now, nothing forces the included shell scripts to match *makefiles.sh.

Something that should work more reliably is something like:
MOZ_BUILD_APP=browser
MOZ_EXTENSIONS=list of extensions (could be automatically gotten from the list of directories under extensions)
alias [=true
alias test=true
. allmakefiles.sh

The Makefile in config/tests/makefiles/allmakefiles seems too complicated, too.
Comment 13 Joey Armstrong [:joey] 2012-05-30 08:31:23 PDT
(In reply to Mike Hommey [:glandium] from comment #12)
> Comment on attachment 627243 [details] [diff] [review]
> add check to detect non-existent makefiles in *makefiles.sh
> 
> Review of attachment 627243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/tests/makefiles/allmakefiles/checkAllMakefiles.sh
> @@ +15,5 @@
> > +
> > +###########################################################################
> > +## Intent: Gather a list of script files to examine
> > +###########################################################################
> > +function gather_sh
> 
> I think both this function and gather_mk are too much of a parsing hack.
> There are some "makefiles" that aren't Makefiles, and while probably not
> true now, nothing forces the included shell scripts to match *makefiles.sh.
> 
> Something that should work more reliably is something like:
> MOZ_BUILD_APP=browser
> MOZ_EXTENSIONS=list of extensions (could be automatically gotten from the
> list of directories under extensions)
> alias [=true
> alias test=true
> . allmakefiles.sh
> 
> The Makefile in config/tests/makefiles/allmakefiles seems too complicated,
> too.

Setting variables and providing a list of extensions requires background knowledge from the user.  If they are not aware of all the little settings or omit entries from the extension(s) list, cases will be missed and latent problems allowed into the system.

This test is setup similar to check-sync-dirs.  *No* extra knowledge or external dependencies are needed, simply run 'make check' and the tool will report on any problem files.


>> There are some "makefiles" that aren't Makefiles

If we have special case files named as a makefile but are not a makefile I think we should be correcting them by renaming rather than force any tools written to be aware of a special case.

Also why would non-makefiles be referenced by *makefiles.sh, that seems a bit confisuing.
Comment 14 Mike Hommey [:glandium] 2012-05-30 09:05:18 PDT
(In reply to Joey Armstrong [:joey] from comment #13)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > Comment on attachment 627243 [details] [diff] [review]
> > add check to detect non-existent makefiles in *makefiles.sh
> > 
> > Review of attachment 627243 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: config/tests/makefiles/allmakefiles/checkAllMakefiles.sh
> > @@ +15,5 @@
> > > +
> > > +###########################################################################
> > > +## Intent: Gather a list of script files to examine
> > > +###########################################################################
> > > +function gather_sh
> > 
> > I think both this function and gather_mk are too much of a parsing hack.
> > There are some "makefiles" that aren't Makefiles, and while probably not
> > true now, nothing forces the included shell scripts to match *makefiles.sh.
> > 
> > Something that should work more reliably is something like:
> > MOZ_BUILD_APP=browser
> > MOZ_EXTENSIONS=list of extensions (could be automatically gotten from the
> > list of directories under extensions)
> > alias [=true
> > alias test=true
> > . allmakefiles.sh
> > 
> > The Makefile in config/tests/makefiles/allmakefiles seems too complicated,
> > too.
> 
> Setting variables and providing a list of extensions requires background
> knowledge from the user.  If they are not aware of all the little settings
> or omit entries from the extension(s) list, cases will be missed and latent
> problems allowed into the system.

Your script won't find the files that depend on these variables either. And the "user" is the build system, it knows these values.

> >> There are some "makefiles" that aren't Makefiles
> 
> If we have special case files named as a makefile but are not a makefile I
> think we should be correcting them by renaming rather than force any tools
> written to be aware of a special case.
> 
> Also why would non-makefiles be referenced by *makefiles.sh, that seems a
> bit confisuing.

Because they are things to be preprocessed by autoconf, and that includes headers. This is a little bit of abuse, but renaming these files is out of the question.
Comment 15 Mike Hommey [:glandium] 2012-05-30 10:14:07 PDT
Created attachment 628372 [details] [diff] [review]
PoC

For instance, I don't think this should be significantly more complicated than this.
<none>
Comment 16 Mike Hommey [:glandium] 2012-05-30 10:17:14 PDT
In fact, I think we should just fail configure when files are missing, and this can be done in build/autoconf/acoutput-fast.pl
Comment 17 Mike Hommey [:glandium] 2012-05-30 10:52:32 PDT
Created attachment 628386 [details] [diff] [review]
Fail configure when acoutputfast.pl fails to find one of the input files
Comment 18 Joey Armstrong [:joey] 2012-05-30 12:23:47 PDT
(In reply to Mike Hommey [:glandium] from comment #14)> > Setting variables and providing a list of extensions requires background
> > knowledge from the user.  If they are not aware of all the little settings
> > or omit entries from the extension(s) list, cases will be missed and latent
> > problems allowed into the system.
> 
> Your script won't find the files that depend on these variables either. And
> the "user" is the build system, it knows these values.

'The build system knows' is not a very user friendly answer.  If I am making changes in my local sandbox and would like to determine if edits may cause breakage or something was missed it would be nice not having to do a full build or waste resources on try to gain a coverage answer.

This checkup script covered the basics, missing files.  If additional coverage were needed it should be easy enough to expand coverage of the checking script.


> > >> There are some "makefiles" that aren't Makefiles
> > 
> > If we have special case files named as a makefile but are not a makefile I
> > think we should be correcting them by renaming rather than force any tools
> > written to be aware of a special case.
> > 
> > Also why would non-makefiles be referenced by *makefiles.sh, that seems a
> > bit confisuing.
> 
> Because they are things to be preprocessed by autoconf, and that includes
> headers. This is a little bit of abuse, but renaming these files is out of
> the question.

Why would correcting a problem like naming convention be an issue, even if this is legacy behavior ?  If pre-processed headers are incorrectly named to resemble a makefile I would think that condition should be corrected to avoid future confusion.  

Allocate a new filename extension .h.preprocess {~whatever} then declare new rules so autoconf can locate and digest the files as headers rather than a mangled makefile name to produce the same behavior.
Comment 19 Ted Mielczarek [:ted.mielczarek] 2012-06-18 12:39:48 PDT
Comment on attachment 628386 [details] [diff] [review]
Fail configure when acoutputfast.pl fails to find one of the input files

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

This seems like a nice simple approach. It would be nice to perhaps expand this to be able to error even if a nonexistent file is stuck inside an if block.
Comment 21 Mike Hommey [:glandium] 2012-06-20 00:25:40 PDT
Backed out. Looks like there were some other removals in the meantime. I'll sort it out before relanding.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0486265b0034
Comment 22 Mike Hommey [:glandium] 2012-06-20 00:46:56 PDT
Relanded
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a83607746e

Note You need to log in before you can comment on or make changes to this bug.