[Geany-devel] geany-plugins: Autotools usage

[Chow Loong Jin]

On 23/04/2012 00:55, Quentin Glidic wrote:
> On 22/04/2012 18:21, Chow Loong Jin wrote:
>> On 22/04/2012 20:09, Quentin Glidic wrote:
>>> Hello,
>>>
>>> I noticed a global misuse / misunderstanding of the Autotools 
>>> (mostly about the dist mechanism) among the geany-plugins.
>>
>> Could you elaborate, please?
>>
>> I noticed that you have shifted all the ENABLE_$PLUGIN conditionals 
>> to the top-level Makefile.am instead, by modifying the SUBDIRS 
>> variable there. However, this is not desirable as disabled plugins 
>> will be excluded from the release tarball generated by `make dist'.
>>
>> --enable/disable-$plugin should only toggle the building of the 
>> actual plugin.
> 
> That’s exactly the misunderstanding I was speaking about. From the
> Automake manual:
> 
> “If SUBDIRS is defined conditionally using Automake conditionals,
> Automake will define DIST_SUBDIRS automatically from the possible values
> of SUBDIRS in all conditions.”

Great! I hadn't known of the existence of the DIST_SUBDIRS variable. I do recall
running into issues where whole plugins went missing from the release tarball
using that approach though.

Could you include that explanation into the commit message of that patch, please?

> [...]
> Automake handles the conditionnal perfectly by itself, and just need
> some "dist_" prefixes here and there, while EXTRA_DIST should be
> reserved for files that are used outside of automake scope.

Of course. I believe they were mostly used in the past for the AM_CONDITIONAL
separation.

Apart from that.. it looks mostly good, but here are a couple of questions/issues:
- Is there a reason you defined plugin = geanydoc in geanydoc/src/Makefile.am?
  It doesn't look like it's needed there.

- "FIXME: CSS?" doesn't look like it's needed in geanygendoc -- there's a rule
  to generate manual.html from manual.rst and manual.css. (This probably
  shouldn't be dist'd, but Colomban would probably be in a better position to
  answer that)

- What's up with the FIXME in geanyvc/src/Makefile.am? I don't think "…" as a
  FIXME message is particularly descriptive.

- The following hunk is really unnecessary. I personally prefer spaces between
  # and the actual comment.

> diff --git a/pretty-printer/Makefile.am b/pretty-printer/Makefile.am
> index 8c643f8..a82e819 100644
> --- a/pretty-printer/Makefile.am
> +++ b/pretty-printer/Makefile.am
> @@ -1,4 +1,4 @@
> -# include $(top_srcdir)/build/vars.auxfiles.mk
> +#include $(top_srcdir)/build/vars.auxfiles.mk
> 
>  SUBDIRS = src
>  plugin = codenav


And finally... please separate the patch a little. Specifically, I think the
unittest-related changes should go into their own commit.

-- 
Loong Jin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 900 bytes
Desc: OpenPGP digital signature
URL: <http://lists.geany.org/pipermail/devel/attachments/20120423/f69e1e6d/attachment.pgp>