Hello,
I noticed a global misuse / misunderstanding of the Autotools (mostly about the dist mechanism) among the geany-plugins.
Attached a patch to correct the Autotools build system. I can split this commit into several ones if needed (one for each plugin plus some more global ones). I just need a bit of time to do so (fortunately, git helps a lot).
Cheers
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.
Attached a patch to correct the Autotools build system. I can split this commit into several ones if needed (one for each plugin plus some more global ones). I just need a bit of time to do so (fortunately, git helps a lot).
Cheers
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.”
And here’s the diff of the "tar tf" on the make dist tarball:
--- before-patch 2012-04-22 18:33:34.080440186 +0200 +++ after-patch 2012-04-22 18:33:48.168440399 +0200 @@ -127,0 +128 @@ +geany-plugins-1.22/debugger/wscript_configure @@ -201,0 +203 @@ +geany-plugins-1.22/debugger/wscript_build @@ -219,0 +222 @@ +geany-plugins-1.22/multiterm/wscript_configure @@ -243,0 +247 @@ +geany-plugins-1.22/multiterm/wscript_build @@ -310,0 +315 @@ +geany-plugins-1.22/devhelp/wscript_configure @@ -367,0 +373 @@ +geany-plugins-1.22/devhelp/wscript_build @@ -695,0 +702 @@ +geany-plugins-1.22/geanylua/wscript_configure @@ -749,0 +757 @@ +geany-plugins-1.22/geanylua/wscript_build @@ -914,0 +923 @@ +geany-plugins-1.22/webhelper/wscript_configure @@ -934,0 +944 @@ +geany-plugins-1.22/webhelper/wscript_build
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.
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.
On 23/04/2012 02:14, Chow Loong Jin wrote:
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.
One more issue:
hyperair@thinkpwn:~/src/geany-plugins/multiterm/src [master]% make V=1 distdir
[ 2:17AM] rm -f multiterm_la_vala.stamp && echo stamp > multiterm_la_vala.stamp-t CDPATH="${ZSH_VERSION+.}:" && cd . && --vapidir ../../multiterm/src/vapi --pkg gtk+-2.0 --pkg geany --pkg vte --header multiterm.h --use-header -C config.vala context-menu.vala defconf.vala notebook.vala plugin.vala shell-config.vala tab-label.vala terminal.vala /bin/bash: --vapidir: command not found
I think we have a bug in Automake. You can reproduce this using: ./configure --disable-addons --disable-autom4te.cache --disable-build --disable-codenav --disable-debugger --disable-devhelp --disable-geanydoc --disable-geanyextrasel --disable-geanygdb --disable-geanygendoc --disable-geanyinsertnum --disable-geanylatex --disable-geanylipsum --disable-geanylua --disable-geanymacro --disable-geanyminiscript --disable-geanynumberedbookmarks --disable-geanypg --disable-geany-plugins-1.22 --disable-geanyprj --disable-geanysendmail --disable-geanyvc --disable-gproject --disable-multiterm --disable-po --disable-pretty-printer --disable-shiftcolumn --disable-spellcheck --disable-tableconvert --disable-treebrowser --disable-updatechecker --disable-webhelper --disable-xmlsnippets
Cheers
On 22/04/2012 20:20, Chow Loong Jin wrote:
On 23/04/2012 02:14, Chow Loong Jin wrote:
Could you include that explanation into the commit message of that patch, please?
Done.
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.
No, removed.
- "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)
That was just a question about dist it or not.
- What's up with the FIXME in geanyvc/src/Makefile.am? I don't think "…" as a FIXME message is particularly descriptive.
It has its own commit now (5/9).
- The following hunk is really unnecessary. I personally prefer spaces between # and the actual comment.
Reflex…
And finally... please separate the patch a little. Specifically, I think the unittest-related changes should go into their own commit.
No problem, planed and done now!
One more issue:
hyperair@thinkpwn:~/src/geany-plugins/multiterm/src [master]% make V=1 distdir
[ 2:17AM]rm -f multiterm_la_vala.stamp && echo stamp > multiterm_la_vala.stamp-t CDPATH="${ZSH_VERSION+.}:" && cd . && --vapidir ../../multiterm/src/vapi --pkg gtk+-2.0 --pkg geany --pkg vte --header multiterm.h --use-header -C config.vala context-menu.vala defconf.vala notebook.vala plugin.vala shell-config.vala tab-label.vala terminal.vala /bin/bash: --vapidir: command not found
You have to set VALAC when using Vala, and you can’t dist without Vala, it’s not a real issue but it’s still annoying.
Attached the patch suite (9 patches). The last one may be removed if it’s not yet desirable.
Cheers
Any news about this patch set?
Sorry for the delay, I completely missed this mail.
Le 22/04/2012 20:14, Chow Loong Jin a écrit :
[...]
- "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)
Actually there is a bug in the current build system, because the html4css1 stylesheet should either be distributed or embedded, which isn't currently the case.
Attached are a few patches updating the build system to properly embed both stylesheets, and a few other updates. I didn't commit them/made a PR yet because #2 is likely to conflict.
Regards, Colomban