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
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
Any news about this patch set?
On 16/06/2012 05:47, Quentin Glidic wrote:
Any news about this patch set?
Committed and pushed. Thanks for the reminder and sorry for the delay. Your previous email slipped my notice.