Last Comment Bug 661855 - build/unix/uniq.pl - escape regexpr chars in directory paths to avoid exclusion by wildcard (dot)
: build/unix/uniq.pl - escape regexpr chars in directory paths to avoid exclusi...
Status: RESOLVED FIXED
fixed-in-bs
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla8
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on:
Blocks: 682897
  Show dependency treegraph
 
Reported: 2011-06-03 09:28 PDT by Joey Armstrong [:joey]
Modified: 2011-08-29 21:57 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix for uniq.pl + unit test (11.64 KB, text/plain)
2011-06-03 09:28 PDT, Joey Armstrong [:joey]
benjamin: review-
Details
fix for uniq.pl + unit test (14.40 KB, patch)
2011-06-15 12:16 PDT, Joey Armstrong [:joey]
khuey: review+
Details | Diff | Review
nit fixes for earlier patch submission (14.25 KB, patch)
2011-06-20 09:14 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Review
nit fixes for earlier checkin, prune checkin msg, hg export format for patch (13.94 KB, patch)
2011-06-20 12:44 PDT, Joey Armstrong [:joey]
justin.lebar+bug: checkin+
Details | Diff | Review
unique.pl regexpr fix + unit test + cp into js/src/build/unix (16.78 KB, patch)
2011-06-21 13:31 PDT, Joey Armstrong [:joey]
khuey: review+
Details | Diff | Review
uniq.pl - escape regex char dot in paths + unit test (18.22 KB, patch)
2011-07-21 13:17 PDT, Joey Armstrong [:joey]
khuey: review+
Details | Diff | Review

Description Joey Armstrong [:joey] 2011-06-03 09:28:28 PDT
Created attachment 537159 [details]
fix for uniq.pl + unit test
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-06-13 09:32:43 PDT
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.
Comment 2 Joey Armstrong [:joey] 2011-06-15 12:16:47 PDT
Created attachment 539612 [details] [diff] [review]
fix for uniq.pl + unit test

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".
Comment 3 Joey Armstrong [:joey] 2011-06-16 14:22:00 PDT
Comment on attachment 539612 [details] [diff] [review]
fix for uniq.pl + unit test

Review of attachment 539612 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-17 09:18:20 PDT
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 ...
Comment 5 Joey Armstrong [:joey] 2011-06-17 10:41:53 PDT
> ::: 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
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-17 11:08:50 PDT
(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.
Comment 7 Joey Armstrong [:joey] 2011-06-20 09:14:33 PDT
Created attachment 540488 [details] [diff] [review]
nit fixes for earlier patch submission

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.
Comment 8 Joey Armstrong [:joey] 2011-06-20 12:44:09 PDT
Created attachment 540563 [details] [diff] [review]
nit fixes for earlier checkin, prune checkin msg, hg export format for patch
Comment 9 Justin Lebar (not reading bugmail) 2011-06-20 12:53:57 PDT
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
Comment 10 Ted Mielczarek [:ted.mielczarek] 2011-06-21 12:39:20 PDT
This caused orange on b-s because it didn't change the copies in js/src, FYI.
Comment 11 Joey Armstrong [:joey] 2011-06-21 13:31:32 PDT
Created attachment 540863 [details] [diff] [review]
unique.pl regexpr fix + unit test + cp into js/src/build/unix

cp build/unix/uniq.pl js/src/build/unix/uniq.pl to get the scripts back in sync.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-24 09:28:40 PDT
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
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-24 10:20:33 PDT
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.
Comment 14 Joey Armstrong [:joey] 2011-07-21 13:17:51 PDT
Created attachment 547484 [details] [diff] [review]
uniq.pl - escape regex char dot in paths + unit test

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.
Comment 15 Ted Mielczarek [:ted.mielczarek] 2011-08-09 07:30:36 PDT
http://hg.mozilla.org/projects/build-system/rev/4aa92c4a5f13
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-10 08:13:57 PDT
http://hg.mozilla.org/mozilla-central/rev/4aa92c4a5f13
Comment 17 :Ms2ger 2011-08-10 12:12:05 PDT
You seem to be adding tabs.
Comment 18 Justin Wood (:Callek) 2011-08-10 12:16:25 PDT
(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
Comment 19 Joey Armstrong [:joey] 2011-08-11 03:28:43 PDT
(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.

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