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)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla16
People
(Reporter: joey, Assigned: glandium)
References
Details
Attachments
(2 files, 1 obsolete file)
11.35 KB,
patch
|
glandium
:
review-
|
Details | Diff | Splinter Review |
6.24 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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
Comment 2•12 years ago
|
||
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?
Assignee | ||
Comment 3•12 years ago
|
||
Wouldn't it make sense to have a mercurial hook checking allmakefiles.sh (and friends) don't contain directories that don't exist anymore?
Reporter | ||
Comment 4•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dc2296f477cb
Reporter | ||
Comment 8•12 years ago
|
||
Reporter | ||
Comment 9•12 years ago
|
||
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)
Reporter | ||
Comment 10•12 years ago
|
||
Try job: https://tbpl.mozilla.org/?tree=Try&rev=f724c409a904
Reporter | ||
Comment 11•12 years ago
|
||
ping on code review
Assignee | ||
Comment 12•12 years ago
|
||
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-
Reporter | ||
Comment 13•12 years ago
|
||
(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.
Assignee | ||
Comment 14•12 years ago
|
||
(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.
Assignee | ||
Comment 15•12 years ago
|
||
For instance, I don't think this should be significantly more complicated than this. <none>
Assignee | ||
Comment 16•12 years ago
|
||
In fact, I think we should just fail configure when files are missing, and this can be done in build/autoconf/acoutput-fast.pl
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #628386 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•12 years ago
|
Attachment #628372 -
Attachment is obsolete: true
Reporter | ||
Comment 18•12 years ago
|
||
(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•12 years ago
|
||
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+
Updated•12 years ago
|
Assignee: nobody → mh+mozilla
Assignee | ||
Comment 20•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/efc2f07a829b
Target Milestone: --- → mozilla16
Assignee | ||
Comment 21•12 years ago
|
||
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
Assignee | ||
Comment 22•12 years ago
|
||
Relanded https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a83607746e
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [leave open after merging]
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•