Closed Bug 734125 Opened 12 years ago Closed 12 years ago

Nonexistent file mochitest/globalstorage/Makefile.in listed in toolkit-makefiles.sh

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla16

People

(Reporter: joey, Assigned: glandium)

References

Details

Attachments

(2 files, 1 obsolete file)

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
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.
Summary: config/rules.mk: nop job failure - mochitest/globalstorage/Makefile.in → Nonexistent file mochitest/globalstorage/Makefile.in listed in toolkit-makefiles.sh
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?
Wouldn't it make sense to have a mercurial hook checking allmakefiles.sh (and friends) don't contain directories that don't exist anymore?
(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.
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.
Blocks: 687579
Whiteboard: [leave open after merging]
Version: unspecified → Trunk
(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 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
Attachment #627243 - Flags: review?(ted.mielczarek)
ping on code review
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.
Attachment #627243 - Flags: review?(ted.mielczarek) → review-
(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.
(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.
Attached patch PoC (obsolete) — Splinter Review
For instance, I don't think this should be significantly more complicated than this.
<none>
In fact, I think we should just fail configure when files are missing, and this can be done in build/autoconf/acoutput-fast.pl
Attachment #628372 - Attachment is obsolete: true
(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 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.
Attachment #628386 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → mh+mozilla
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [leave open after merging]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.