Bug 69216

Created attachment 85615 [details] [review]
.gitignore: automake 1.13 copies in /test-driver

Created attachment 85616 [details] [review]
configure: use same debug check code than Gabble

Created attachment 85617 [details] [review]
configure: add summary

Created attachment 85618 [details] [review]
move sidecars.py to TWISTED_AVAHI_TESTS

Created attachment 85619 [details] [review]
Import run-test.sh.in from Gabble master

Created attachment 85620 [details] [review]
Run regression tests under the run-test.sh "driver"

Created attachment 85621 [details] [review]
always enable Avahi tests when building with the Avahi backend

Comment on attachment 85616 [details] [review]
configure: use same debug check code than Gabble

Review of attachment 85616 [details] [review]:
-----------------------------------------------------------------

Did we not have AC_ARG_ENABLE(debug) somewhere else already?

Strictly speaking, this is underquoted (AC_ARG_ENABLE([debug], ...) - one level of [] per level of ()).

The if/fi should remain an AS_IF, which doesn't matter except for when it does (Bug #53445, <https://bugzilla.gnome.org/show_bug.cgi?id=681413>).

Comment on attachment 85617 [details] [review]
configure: add summary

Review of attachment 85617 [details] [review]:
-----------------------------------------------------------------

If you like. I find these summaries a bit spammy - they're working around the amount of noise configure generates by adding more noise - but whatever.

Comment on attachment 85620 [details] [review]
Run regression tests under the run-test.sh "driver"

Review of attachment 85620 [details] [review]:
-----------------------------------------------------------------

::: tests/twisted/Makefile.am
@@ +115,5 @@
> +# so far - but I'm substituting it to keep the script more similar to Gabble's.
> +# ${pkglibexecdir}/tests is what GNOME's InstalledTests goal recommends.
> +#
> +# Similarly, Gabble supports TEST_PYTHON differing from PYTHON for historical
> +# reasons, but we don't do that here.

Are you sure about that? configure mentions TEST_PYTHON.

All good except where noted.

(In reply to comment #8)
> Comment on attachment 85616 [details] [review] [review]
> configure: use same debug check code than Gabble
> 
> Review of attachment 85616 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Did we not have AC_ARG_ENABLE(debug) somewhere else already?

No, actually that's the thing I meant to fix. Patch updated.

(In reply to comment #9)
> Comment on attachment 85617 [details] [review] [review]
> configure: add summary
> 
> Review of attachment 85617 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> If you like. I find these summaries a bit spammy - they're working around
> the amount of noise configure generates by adding more noise - but whatever.

I actually like them, if only when debugging/hacking on configure.ac.

(In reply to comment #10)
> Comment on attachment 85620 [details] [review] [review]
> Run regression tests under the run-test.sh "driver"
> 
> Review of attachment 85620 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: tests/twisted/Makefile.am
> @@ +115,5 @@
> > +# so far - but I'm substituting it to keep the script more similar to Gabble's.
> > +# ${pkglibexecdir}/tests is what GNOME's InstalledTests goal recommends.
> > +#
> > +# Similarly, Gabble supports TEST_PYTHON differing from PYTHON for historical
> > +# reasons, but we don't do that here.
> 
> Are you sure about that? configure mentions TEST_PYTHON.

You're right, I just re-used your file. I removed this comment.

Created attachment 85626 [details] [review]
configure: add AC_ARG_ENABLE(debug)

Created attachment 85627 [details] [review]
Run regression tests under the run-test.sh "driver"

Comment on attachment 85627 [details] [review]
Run regression tests under the run-test.sh "driver"

Review of attachment 85627 [details] [review]:
-----------------------------------------------------------------

::: tests/twisted/Makefile.am
@@ +116,5 @@
> +# ${pkglibexecdir}/tests is what GNOME's InstalledTests goal recommends.
> +run-test.sh: run-test.sh.in Makefile
> +	$(AM_V_GEN)sed \
> +			-e 's![@]saluttestsdir[@]!${pkglibexecdir}/tests!' \
> +			-e 's![@]TEST_PYTHON[@]!$(PYTHON)!' \

Replace with $(TEST_PYTHON) please? Other than that, ++

Comment on attachment 85626 [details] [review]
configure: add AC_ARG_ENABLE(debug)

Review of attachment 85626 [details] [review]:
-----------------------------------------------------------------

++ with comments:

::: configure.ac
@@ +89,4 @@
>     unused-parameter])
>  AC_SUBST([ERROR_CFLAGS])
>  
> +AC_ARG_ENABLE(debug,

[debug] would be better, but not very important

@@ +90,5 @@
>  AC_SUBST([ERROR_CFLAGS])
>  
> +AC_ARG_ENABLE(debug,
> +  AC_HELP_STRING([--disable-debug],[compile without debug code]),
> +    enable_debug=$enableval, enable_debug=yes )

Would be better as "[enable_debug=$enableval], [enable_debug=yes])" or even "[], [enable_debug=yes])" although it isn't important.

(Look at the generated code - AC_ARG_ENABLE([foo]) always sets enable_foo in the "user-specified" case anyway.)

(In reply to comment #18)
> > +AC_ARG_ENABLE(debug,
> > +  AC_HELP_STRING([--disable-debug],[compile without debug code]),
> > +    enable_debug=$enableval, enable_debug=yes )

Less misleading indentation would also be nice: enable_debug=... is at the same level as AC_HELP_STRING.

Strictly speaking, the whole AC_HELP_STRING call should also be inside [] to avoid underquoting:

    AC_ARG_ENABLE([debug],
      [AC_HELP_STRING([--disable-debug], [compile without debug code])],
      [], [enable_debug=yes])

(In reply to comment #17)
> Comment on attachment 85627 [details] [review] [review]
> Run regression tests under the run-test.sh "driver"
> 
> Review of attachment 85627 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: tests/twisted/Makefile.am
> @@ +116,5 @@
> > +# ${pkglibexecdir}/tests is what GNOME's InstalledTests goal recommends.
> > +run-test.sh: run-test.sh.in Makefile
> > +	$(AM_V_GEN)sed \
> > +			-e 's![@]saluttestsdir[@]!${pkglibexecdir}/tests!' \
> > +			-e 's![@]TEST_PYTHON[@]!$(PYTHON)!' \
> 
> Replace with $(TEST_PYTHON) please? Other than that, ++

done.

In reply to comment #19)
> (In reply to comment #18)
> > > +AC_ARG_ENABLE(debug,
> > > +  AC_HELP_STRING([--disable-debug],[compile without debug code]),
> > > +    enable_debug=$enableval, enable_debug=yes )
> 
> Less misleading indentation would also be nice: enable_debug=... is at the
> same level as AC_HELP_STRING.
> 
> Strictly speaking, the whole AC_HELP_STRING call should also be inside [] to
> avoid underquoting:
> 
>     AC_ARG_ENABLE([debug],
>       [AC_HELP_STRING([--disable-debug], [compile without debug code])],
>       [], [enable_debug=yes])

changed.

Merged to master; thanks!