Closed
Bug 661855
Opened 13 years ago
Closed 13 years ago
build/unix/uniq.pl - escape regexpr chars in directory paths to avoid exclusion by wildcard (dot)
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla8
People
(Reporter: joey, Assigned: joey)
References
Details
(Whiteboard: fixed-in-bs)
Attachments
(1 file, 5 obsolete files)
18.22 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → joey
Assignee | ||
Updated•13 years ago
|
Attachment #537159 -
Flags: review?(coop)
Assignee | ||
Updated•13 years ago
|
Attachment #537159 -
Flags: review?(khuey)
Attachment #537159 -
Flags: review?(coop)
Attachment #537159 -
Flags: review?(benjamin)
Comment 1•13 years ago
|
||
Comment on attachment 537159 [details] fix for uniq.pl + unit test We already have a unit testing infrastructure for build scripts, and we'd like any new tests to run under "make check". See http://mxr.mozilla.org/mozilla-central/source/build/Makefile.in#126 or http://mxr.mozilla.org/mozilla-central/source/config/Makefile.in#194 Also, there's a /build/unix/test/.hgignore file, but .hgignore is only relevant at the root directory: it is ignored in other directories. It shouldn't be necessary to add that hgignore file, though, since we should be running the test in the objdir, not the srcdir.
Attachment #537159 -
Flags: review?(khuey)
Attachment #537159 -
Flags: review?(benjamin)
Attachment #537159 -
Flags: review-
Assignee | ||
Comment 2•13 years ago
|
||
build/unix/uniq.pl - escape dots to handle regexpr patterns properly. build/unix/test/* - unit test setup to verify behavior. Makefile.in added to allow launching the test via "gmake check".
Attachment #537159 -
Attachment is obsolete: true
Attachment #539612 -
Flags: review?(benjamin)
Assignee | ||
Comment 3•13 years ago
|
||
Comment on attachment 539612 [details] [diff] [review] fix for uniq.pl + unit test Review of attachment 539612 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #539612 -
Flags: review?(khuey)
Comment on attachment 539612 [details] [diff] [review] fix for uniq.pl + unit test Review of attachment 539612 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, a number of comments, but most are nits. I only skimmed the perl test code, but it looks good. In general, things in the build system only require one review. My review is sufficient here, and bsmedberg is very busy, so I've cancelled the other review request. ::: build/unix/Makefile.in @@ +54,5 @@ > +ifdef ENABLE_TESTS > + ifeq ($(NULL),$(filter WINNT OS2,$(OS_ARCH))) > + DIRS += test > + endif # WIN > +endif # ENABLE_TESTS I wish we did this everywhere ;-) Small nit, use ifeq (,$(filter ...)) rather than ifeq ($(NULL),$(filter ...)) @@ +61,5 @@ > > libs:: $(srcdir)/run-mozilla.sh > $(INSTALL) $< $(DIST)/bin > + > +check:: Why is there an empty target here? ::: build/unix/test/Makefile.in @@ +16,5 @@ > +# The Original Code is mozilla.org code. > +# > +# The Initial Developer of the Original Code is > +# Netscape Communications Corporation. > +# Portions created by the Initial Developer are Copyright (C) 1998 Nits: developer is the Mozilla Foundation, copyright year is 2011 @@ +43,5 @@ > +VPATH = @srcdir@ > + > +include $(DEPTH)/config/autoconf.mk > + > +# MODULE = build Just remove this line, it doesn't actually do anything or give any useful information @@ +51,5 @@ > +################################################## > +## Gather a list of tests, generate timestamp deps > +################################################## > +TS=.ts > +ifneq ($(NULL),$(findstring check,$(MAKECMDGOALS))) ifneq (,$(findstring ...)) @@ +52,5 @@ > +## Gather a list of tests, generate timestamp deps > +################################################## > +TS=.ts > +ifneq ($(NULL),$(findstring check,$(MAKECMDGOALS))) > + check_targets =$(null) If you're going to use a null variable, use $(NULL). This assignment isn't necessary though, and can be dropped. @@ +60,5 @@ > +endif > + > +check:: $(TS) $(check_targets) > +clean:: > + $(RM) -r $(TS) We have infrastructure for these types of clean targets. Just say GARBAGE_DIRS += $(TS). @@ +65,5 @@ > + > +############################################# > +# Only invoke tests when sources have changed > +############################################# > +$(TS)/%: $(srcdir)/% Doesn't this need to depend explicitly on $(TS) to avoid breaking a parallel build? @@ +71,5 @@ > + echo "TEST-UNEXPECTED-FAIL | build/unix/test | unify failed to unify files with differing line ordering!"; \ > + /bin/false; \ > + else \ > + echo "TEST-PASS | build/ | unify unified files with differing line ordering!"; \ > + fi I would prefer that we moved the logic to print TEST-[UNEXPECTED-FAIL|PASS] into runtest and just made this a call into runtest. Also, I'm not sure where these messages came from ...
Attachment #539612 -
Flags: review?(khuey)
Attachment #539612 -
Flags: review?(benjamin)
Attachment #539612 -
Flags: review+
Assignee | ||
Comment 5•13 years ago
|
||
> ::: build/unix/Makefile.in > @@ +54,5 @@ > > +ifdef ENABLE_TESTS > > + ifeq ($(NULL),$(filter WINNT OS2,$(OS_ARCH))) > > + DIRS += test > > + endif # WIN > > +endif # ENABLE_TESTS > > I wish we did this everywhere ;-) If there are good practices/suggestions like this to follow maybe it would be worth starting a wiki page to document them for posterity. Might come in handy later with re-factoring. I'll look into setting one up. For this particular case filtering by '/unix/' for non-unix might be a way to somewhat automate traversal exclusions in the sandbox. > Small nit, use ifeq (,$(filter ...)) rather than ifeq ($(NULL),$(filter ...)) I normally add $(NULL) just so the conditional easier to see { would be nice if make supported "iftrue" to simplify }. Is use of (,x) the preferred convention ? > ::: build/unix/test/Makefile.in > @@ +16,5 @@ > > +# The Original Code is mozilla.org code. > > +# > > +# The Initial Developer of the Original Code is > > +# Netscape Communications Corporation. > > +# Portions created by the Initial Developer are Copyright (C) 1998 > > Nits: developer is the Mozilla Foundation, copyright year is 2011 Hmmm, there may be more in the sandbox to update. The block was copied verbatim from either client.mk or rules.mk. > @@ +65,5 @@ > > + > > +############################################# > > +# Only invoke tests when sources have changed > > +############################################# > > +$(TS)/%: $(srcdir)/% > > Doesn't this need to depend explicitly on $(TS) to avoid breaking a parallel > build? Hmmm good question. Dependencies on a directory are bad in general because any inode change will perpetually create a stale target. Each new test run would invalidate all of the tests run earlier. If a target like init:, once:, serial:, etc exists that could be processed on the master before slave machines are able to thread the build that would help eliminate the race condition. > > @@ +71,5 @@ > > + echo "TEST-UNEXPECTED-FAIL | build/unix/test | unify failed to unify files with differing line ordering!"; \ > > + /bin/false; \ > > + else \ > > + echo "TEST-PASS | build/ | unify unified files with differing line ordering!"; \ > > + fi > > I would prefer that we moved the logic to print TEST-[UNEXPECTED-FAIL|PASS] > into runtest and just made this a call into runtest. Also, I'm not sure > where these messages came from ... The syntax was taken from another makefile configured with check test. What would you think about moving the status display logic into a standalone script that was setup to pay attention to and report status rather than embedding it directly ? runtest() use could remain generic while messages are reported by a library routine rather than inline: Something like this: runtest; reportstatus --syntax check_test Thanks for the review
(In reply to comment #5) > > ::: build/unix/Makefile.in > > @@ +54,5 @@ > > > +ifdef ENABLE_TESTS > > > + ifeq ($(NULL),$(filter WINNT OS2,$(OS_ARCH))) > > > + DIRS += test > > > + endif # WIN > > > +endif # ENABLE_TESTS > > > > I wish we did this everywhere ;-) > > If there are good practices/suggestions like this to follow maybe it would > be worth starting a wiki page to document them for posterity. Might come in > handy later with re-factoring. I'll look into setting one up. That would be fantastic. > > Small nit, use ifeq (,$(filter ...)) rather than ifeq ($(NULL),$(filter ...)) > > I normally add $(NULL) just so the conditional easier to see { would be nice > if make supported "iftrue" to simplify }. Is use of (,x) the preferred > convention ? Yeah, that's what we use pretty much everywhere. > > ::: build/unix/test/Makefile.in > > @@ +16,5 @@ > > > +# The Original Code is mozilla.org code. > > > +# > > > +# The Initial Developer of the Original Code is > > > +# Netscape Communications Corporation. > > > +# Portions created by the Initial Developer are Copyright (C) 1998 > > > > Nits: developer is the Mozilla Foundation, copyright year is 2011 > > Hmmm, there may be more in the sandbox to update. The block was copied > verbatim from either client.mk or rules.mk. Well, those existing files were actually developed by Netscape in 1998. This one wasn't though ;-) > > @@ +65,5 @@ > > > + > > > +############################################# > > > +# Only invoke tests when sources have changed > > > +############################################# > > > +$(TS)/%: $(srcdir)/% > > > > Doesn't this need to depend explicitly on $(TS) to avoid breaking a parallel > > build? > > Hmmm good question. > > Dependencies on a directory are bad in general because any inode change will > perpetually create a stale target. Each new test run would invalidate all > of the tests run earlier. Right. I had forgotten about this. We handle this elsewhere by creating a .done file in the directory e.g. http://hg.mozilla.org/mozilla-central/annotate/60182c83c925/config/rules.mk#l1527 > > > > @@ +71,5 @@ > > > + echo "TEST-UNEXPECTED-FAIL | build/unix/test | unify failed to unify files with differing line ordering!"; \ > > > + /bin/false; \ > > > + else \ > > > + echo "TEST-PASS | build/ | unify unified files with differing line ordering!"; \ > > > + fi > > > > I would prefer that we moved the logic to print TEST-[UNEXPECTED-FAIL|PASS] > > into runtest and just made this a call into runtest. Also, I'm not sure > > where these messages came from ... > > The syntax was taken from another makefile configured with check test. > > What would you think about moving the status display logic into a standalone > script that was setup to pay attention to and report status rather than > embedding it directly ? runtest() use could remain generic while messages > are reported by a library routine rather than inline: > > Something like this: > runtest; reportstatus --syntax check_test That sounds like a good idea.
Assignee | ||
Comment 7•13 years ago
|
||
Added $(TS)/.done as a dependency for $(TS) to prevent a race condition during parallel traversal. Updated makefile copyright message and date. Removed stray targets and changed $(NULL) case. Replaced clean:: target with GARBAGE_DIRS += Removed formatted PASS/FAIL status messages from the makefile as they do not appear to be used. runtest will already report "Returning: [PASS|FAIL]" so rely on that.
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #540488 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #539612 -
Attachment is obsolete: true
Comment 9•13 years ago
|
||
Comment on attachment 540563 [details] [diff] [review] nit fixes for earlier checkin, prune checkin msg, hg export format for patch Checked in to build-system. http://hg.mozilla.org/projects/build-system/rev/ac9450e70e57
Attachment #540563 -
Flags: checkin+
Updated•13 years ago
|
Whiteboard: [fixed in build-system]
Updated•13 years ago
|
Whiteboard: [fixed in build-system] → fixed-in-bs
Comment 10•13 years ago
|
||
This caused orange on b-s because it didn't change the copies in js/src, FYI.
Assignee | ||
Comment 11•13 years ago
|
||
cp build/unix/uniq.pl js/src/build/unix/uniq.pl to get the scripts back in sync.
Attachment #540863 -
Flags: review?(khuey)
Comment on attachment 540863 [details] [diff] [review] unique.pl regexpr fix + unit test + cp into js/src/build/unix Ordinarily for this kind of fix we'd just push the fix without review ... but since you don't have L2 commit access that would be ... difficult. I landed the cp bit of this on b-s. http://hg.mozilla.org/projects/build-system/rev/ac9450e70e57
Attachment #540863 -
Flags: review?(khuey) → review+
And then I backed it all out because it turns out this was breaking our windows builds. http://hg.mozilla.org/projects/build-system/rev/61e9e59c8cca The uniq.pl changes are causing NECKO_PROTOCOLS to end up empty after configure ... and turning off things like http have understandably bad results.
Whiteboard: fixed-in-bs → [needs new patch]
Assignee | ||
Comment 14•13 years ago
|
||
Patch contains script edits + unit test. Corruption problem from the last submission was due to an old perl module from the build.mozilla mingw installation. Getopt::Long v2.25 is not able to parse arguments formatted with an added default value (arg_long|arg_short:arg_default). Script problems were noted when configure ran but they were not enough to cause an early failure. Added a conditional in uniq.pl to not pass a default arg when the old module is in play. Changes built cleanly on a windows desktop and the try server.
Attachment #540563 -
Attachment is obsolete: true
Attachment #540863 -
Attachment is obsolete: true
Attachment #547484 -
Flags: review?(khuey)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs new patch]
Attachment #547484 -
Flags: review?(khuey) → review+
Comment 15•13 years ago
|
||
http://hg.mozilla.org/projects/build-system/rev/4aa92c4a5f13
Whiteboard: fixed-in-bs
http://hg.mozilla.org/mozilla-central/rev/4aa92c4a5f13
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Comment 17•13 years ago
|
||
You seem to be adding tabs.
Comment 18•13 years ago
|
||
(In reply to Ms2ger from comment #17) > You seem to be adding tabs. And adding |# EOF| which iirc ted asked not to add in new files for us, since hg makes "no newline at EOF" obvious on diffs
Assignee | ||
Comment 19•13 years ago
|
||
(In reply to Justin Wood (:Callek) from comment #18) > (In reply to Ms2ger from comment #17) > > You seem to be adding tabs. > > And adding |# EOF| which iirc ted asked not to add in new files for us, > since hg makes "no newline at EOF" obvious on diffs Just a timing problem, Ted commented about #EOF in bug 662833 and the line was removed in that patch. The uniq.pl has been in the queue/landing for a while.
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
•