Created attachment 35366 [details] [review]
patch for autogen.sh and acinclude.m4

Here is the patch.  The contents of gtk-doc.m4 appended to acinclude.m4 are
from
gtk-doc 1.2.

Please tell me if I can commit the patch to HEAD (and maybe to the stable
branch
as well).

The configure option for enabling or disabling the build of the docs is
--enable-gtk-doc or --disable-gtk-doc. Please change the patch accordingly.

I also think that your patch makes autogen.sh unnecessarily complex. While it is
probably correct to adapt the command-line parser from configure, it is IMO just
plain overkill here. It should be sufficient to check if --enable-gtk-doc was
passed to autogen.sh or is in AUTOGEN_CONFIGURE_ARGS.

Well, the option for the configure script is --disable-devel-docs (or --enable...)
and there are some lines in configure.in that will set --disable-gtk-doc if the
devel docs are disabled.  That's why I thought that it would be better to test the
"main" feature (devel-docs) instead of testing only the one that depends on it
(gtk-doc).  But I should probably check for both and include the same logic as in
the configure script.

If I have to check for both options, I think that it makes sense to keep the
command-line parsing code as it is now, even if it is unfortunately a bit complex.
Do you agree, or would you prefer to have explicit tests for --enable-gtk-doc,
--enable-devel-docs and the corresponding --disable... options?

Created attachment 35383 [details] [review]
revised patch for autogen.sh and acinclude.m4

This new version of the patch checks for both --disable-devel-docs
and --disable-gtk-doc (devel-docs=no overrides gtk-doc, as in the
configure script).

--disable-devel-docs would better be removed from your patch and from the
configure script. It was a bad idea to add it in the first place.

I used this patch on my cvs checkout.  

It has a problem.  I run ./autogen.sh --help to get information like this:
  --disable-gtk-doc and
  --disable-gtk-doc 
which, to the best of my knowledge is where you get this sort of necessary
information.  The patch does not fix autogen.sh enough that it can tell you
important information -- or it fixes it incompletely or in the wrong place.

If these tools are as well designed as everyone said they are, it seems like it
would be more difficult to make them work this way than to make them work
properly.  Then again, I might be reading more into that situation than actually
exists as well.

Can a patch be made that works so you can get through the initial parts and
enough into the configuration to get this information? 

Carol, despite of your useless ramblings, it had already been stated that the
patch needs work. And no, autogen.sh --help is not going to tell you everything
about the build process. There is no guarantee whatsoever that autogen.sh does
the right thing as it's just a stupid convenience wrapper which is explained in
the file HACKING and doesn't need to document itself.

The change to autogen.sh shouldn't be more than a few lines. We are not going to
duplicate autoconf in what's supposed to be a simple convenience script. It
would be completely sufficient if autogen.sh checked for --disable-gtk-doc.

i will try in the future to remember that "your patch makes autogen.sh
unnecessarily complex" is more useful than being specific with exactly what the
problem is.

is there an easy solution for this patch that is "unnecessarily complex"?

Ok, I will try to solve this.  Here is my proposal:
- Remove the option --disable-devel-docs from configure.in.  This may require
  some changes in some Makefiles or other files; I haven't checked yet.
- Change both autogen.sh so that the error message displayed when gtkdocize is
  not found suggests to use --disable-gtk-doc.
- Change my patch to autogen.sh so that it only checks for --enable-gtk-doc,
  --enable-gtk-doc=no and --disable-gtk-doc.  Note that I still have to have a
  loop and check for these three options because --disable* may be set in
  AUTOGEN_CONFIGURE_ARGS and then overriden by --enable* in the command-line
  arguments.
- Update the "Compilation" section in HACKING and mention that --enable-gtk-doc
  is the default for CVS builds (in the current version, the last paragraph
  incorrectly implies the contrary).

I will try to do that this evening.  Should I submit a patch here, or commit it
directly?

A couple of corrections:

(1) --enable-gtk-doc is not the default, stating that would be wrong.
(2) your patch needs to check for --disable-gtk-doc and nothing else
(3) we need to add a note to HACKING that explains that passing --disable-gtk-doc
    to autogen.sh has the effect of completely disabling gtk-doc support

Created attachment 36113 [details] [review]
Updated patch for autogen.sh, HACKING and acinclude.m4

OK, here is a new version of the patch.  It is shorter now.  I haven't removed
--disable-devel-docs from configure.in because I would like to be really sure
that nobody uses it.

Regarding your comments:
(1) OK, I have left that paragraph unchanged in the HACKING file.
(2) I prefer to add a test for --enable-*.  This only adds 3 lines so that's
    not a big deal.
(3) OK, I have added a paragraph about that to the HACKING file.  I also
    display a warning in autogen.sh if gtk-doc is disabled.

Can I commit this patch?

I haven't yet found time to review it but will definitely apply it before the
next 2.2 release.

Applied to HEAD with some modifications:

2005-01-20  Sven Neumann  <sven@gimp.org>

        * HACKING
        * Makefile.am
        * acinclude.m4
        * autogen.sh: applied (modified) patch from Raphaël Quinet that
        allows to build GIMP from CVS without having gtk-doc installed.
        If you need to do this, pass --disable-gtk-doc to autogen.sh.

        * configure.in: removed --disable-devel-docs option since it has
        become obsolete now.

        * devel-docs/Makefile.am: require gtk-doc when running 'make dist'.

I am reluctant to do this change in the stable branch since it bears the risk of
breaking the build. Will think about it and keep the report open until this has
been figured out.

Created attachment 36302 [details] [review]
patch as applied to HEAD branch

for reference, here's the patch as applied to the HEAD branch

I have now merged these changes into the stable branch. Closing as FIXED.