Created attachment 99470 [details] [review]
gconf-ldap+beautify.patch

This patch does 2 things:
* make ldap detection support yes/no/auto (auto being default) via --with-openldap
* cosmetic changes for ./configure users

 configure.in  |   80 +++++++++++++++++++++++++++++++++++++---------------------
 gconf-2.m4.in |   10 +++----
 2 files changed, 57 insertions(+), 33 deletions(-)

Thanks for considering

and relevant downstream bug: https://bugs.gentoo.org/show_bug.cgi?id=193442

can we get a review here ? I'd be happy to commit this myself but I'd like peer review first :)

Ping...

Created attachment 119053 [details] [review]
gconf-HEAD-autofoo.patch

 * fix whitespace policy in configure.in
 * make openldap support optional and auto-detected
 * raise AC_PREREQ to be able to use AS_HELP_STRING and other improvments of autoconf
 * add a configure summary at the end of configure
 * fix "make distcheck"

 Makefile.am  |    2
 configure.in |  167 +++++++++++++++++++++++++++++++++++++----------------------
 2 files changed, 106 insertions(+), 63 deletions(-)

this patch bundles a lot more changes but I'm confident it doesn't brake anything. I'll commit it in the coming two weeks maximum.

Created attachment 119056 [details] [review]
gconf-HEAD-autofoo.patch

little update, set a default value to enable_defaults_service and HAVE_POLKIT

I'd like to have the "make ldap detection support yes/no/auto (auto being default) via --with-openldap" part separated from the cosmetic fixes.

A few notes on the patch:

+AM_CONFIG_HEADER([config.h])

Deprecated. Use AC_CONFIG_HEADERS.

+AM_INIT_AUTOMAKE([1.9 no-dist-gzip dist-bzip2 -Wall])

I don't think we should use -Wall here.

+AM_CONDITIONAL(OS_WIN32, [test "x$os_win32" = "xyes"])
[...]
+if test "x$os_win32" = "xyes"; then
+  if test "x$enable_static" = "xyes" -o "x$enable_static" = "x"; then
[...]
+  if test "x$enable_shared" = "xno"; then
[etc etc]

It's totally unnecessary to add those exes.

+AC_ARG_WITH(sysconfsubdir,
+  AS_HELP_STRING([--with-sysconfsubdir],
+    [directory name used under sysconfdir @<:@default=gconf@:>@]),
+  sysconfsubdir=${withval}, sysconfsubdir=gconf)

More [] quoting please, and no need for the 3rd arg, just do [] there.

+AC_ARG_ENABLE(debug,
+  AS_HELP_STRING([--enable-debug=@<:@no/yes/minimum@:>@],
+    [Compile with debug checks.]),
+  ,enable_debug=minimum)

More quoting. Also please use standard on AS_HELP_STRING, putting the available values in the 2nd arg.

+  AC_FYI("Will build with debugging spew and checks")

I can find no documentation on AC_FYI (on autoconf 2.61); where does it come from? Do you meant to use AC_MSG_NOTICE? Also wrong quoting style, use [] or even [[]] not "".

+AC_ARG_ENABLE(gtk,
+  AS_HELP_STRING([--enable-gtk],
+    [Enable GTK+ support (for gconf-sanity-check) @<:@default=auto@:>@]),
+  enable_gtk="$enableval", enable_gtk=auto)
 
More quoting, and no need for 3rd arg.

 AC_ARG_ENABLE(defaults-service,
-              [AC_HELP_STRING([--enable-defaults-service],
-                              [build the defaults DBus service [default=auto]])],
-              [enable_defaults_service="$enableval"],
-              [enable_defaults_service=auto])
+  AS_HELP_STRING([--enable-defaults-service],
+    [build the defaults DBus service @<:@default=auto@:>@])],
+  [enable_defaults_service="$enableval"],
+  [enable_defaults_service=auto])

And here. And in all the other AC_ARG_ENABLE/WITH.

(In reply to comment #8)
> I'd like to have the "make ldap detection support yes/no/auto (auto being
> default) via --with-openldap" part separated from the cosmetic fixes.

sure, will do asap

for the rest, please note that I didn't try to fix all issues, mostly fix consistency.

> +AM_INIT_AUTOMAKE([1.9 no-dist-gzip dist-bzip2 -Wall])
> 
> I don't think we should use -Wall here.

I think it is useful to be notified when things start being deprecated and it doesn't stop autogen so no big deal imho.
 
> +AM_CONDITIONAL(OS_WIN32, [test "x$os_win32" = "xyes"])
> [...]
> +if test "x$os_win32" = "xyes"; then
> +  if test "x$enable_static" = "xyes" -o "x$enable_static" = "x"; then
> [...]
> +  if test "x$enable_shared" = "xno"; then
> [etc etc]
> 
> It's totally unnecessary to add those exes.

that was there before and I have no idea what it's there so I didn't touched it.

> +AC_ARG_WITH(sysconfsubdir,
> +  AS_HELP_STRING([--with-sysconfsubdir],
> +    [directory name used under sysconfdir @<:@default=gconf@:>@]),
> +  sysconfsubdir=${withval}, sysconfsubdir=gconf)
>
> More [] quoting please, and no need for the 3rd arg, just do [] there.
>

why more [], if 3rd argument is omitted, there's nothing expandable that would require [] or am I missing something ? AS_HELP_STRING don't need [] afaik.

> +  AC_FYI("Will build with debugging spew and checks")
> 
> I can find no documentation on AC_FYI (on autoconf 2.61); where does it come
> from? Do you meant to use AC_MSG_NOTICE? Also wrong quoting style, use [] or
> even [[]] not "".

no idea either, AC_MSG_NOTICE is probably good too for the job.

(In reply to comment #9)
> > +AC_ARG_WITH(sysconfsubdir,
> > +  AS_HELP_STRING([--with-sysconfsubdir],
> > +    [directory name used under sysconfdir @<:@default=gconf@:>@]),
> > +  sysconfsubdir=${withval}, sysconfsubdir=gconf)
> >
> > More [] quoting please, and no need for the 3rd arg, just do [] there.
> >
> 
> why more [], if 3rd argument is omitted, there's nothing expandable that would
> require [] or am I missing something ? AS_HELP_STRING don't need [] afaik.
You're right here. It's a bit confusing that it's --with-sysconfsubdir while the main one is --sysconfdir ... 

Created attachment 119952 [details] [review]
gconf-HEAD-automagic-ldap.patch

this patch makes LDAP optional support configurable via a proper configure switch and not just automagic. Like I said in an earlier comment, the win32 specific part is left as is because I can't test it and I don't want to break it.

Created attachment 119953 [details] [review]
gconf-HEAD-make-distcheck.patch

this patch fixes make distcheck problem with the new defaults service.

Created attachment 119954 [details] [review]
gconf-HEAD-autofoo.patch

this patch aligns whitespaces, quoting, ... pratices.
I've removed AC_FYI which was in fact defined in the beginning of the configure.in file.

I also removed AWK/PERL checks because reading the sources, I couldn't find where it's used at all.

Created attachment 119955 [details] [review]
gconf-HEAD-macro.patch

This patch makes sure the gconf-m4 macro is consistent with itself (AC_HELP_STRING and quoting). I didn't change AC_HELP_STRING to AS_HELP_STRING because it would force other project to raise their toolchain requirements (which is not bad but, well, it can be changed later).

Created attachment 119956 [details] [review]
gconf-HEAD-include-config.h.patch

this patch changes all #include "config.h" to #include <config.h> as instructed on 

http://www.gnu.org/software/autoconf/manual/autoconf-2.57/html_node/autoconf_27.html

btw, is there a reason runtests.sh is not included in EXTRA_DIST while tests are still distributed. It stops us from running tests to check the builded gconf is functional.

ping ?

ping ?

if there's no objection, I'll try to rebase my work against current HEAD and start commiting those patches as we've been having them (at least the ldap one) in gentoo since 2.16 or so.

done. Thanks for reviewing.

Does this mean that this bug is closed?

(Before I embark on a patch to add AC_CHECK_PROG gconftool-2 before trying to use
it in AM_GCONF_SOURCE_2)

(In reply to comment #21)
> Does this mean that this bug is closed?

It is, sorry I seem to have forgotten to close it.