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)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla8

People

(Reporter: joey, Assigned: joey)

References

Details

(Whiteboard: fixed-in-bs)

Attachments

(1 file, 5 obsolete files)

Attached file fix for uniq.pl + unit test (obsolete) —
      No description provided.
Assignee: nobody → joey
Attachment #537159 - Flags: review?(coop)
Attachment #537159 - Flags: review?(khuey)
Attachment #537159 - Flags: review?(coop)
Attachment #537159 - Flags: review?(benjamin)
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-
Attached patch fix for uniq.pl + unit test (obsolete) — Splinter Review
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)
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+
> ::: 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.
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.
Attachment #539612 - Attachment is obsolete: true
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+
Whiteboard: [fixed in build-system]
Whiteboard: [fixed in build-system] → fixed-in-bs
This caused orange on b-s because it didn't change the copies in js/src, FYI.
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]
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)
Whiteboard: [needs new patch]
http://hg.mozilla.org/mozilla-central/rev/4aa92c4a5f13
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You seem to be adding tabs.
(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
(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.
Blocks: 682897
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: