| Message ID | 1415366931-6870-2-git-send-email-johan.oudinet@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Johan, All, On 2014-11-07 14:28 +0100, Johan Oudinet spake thusly: > Define common macros only once (instead of as many times as > there are inner-autotools-package calls). > Factorize LIBTOOL_PATCH_HOOK and AUTORECONF_HOOK to avoid duplicated > code. Sorry it takes so long to review this series. Touching the infra is not for the faint of heart, and reviews are really complex and time-consuming. > Signed-off-by: Johan Oudinet <johan.oudinet@gmail.com> > --- > package/pkg-autotools.mk | 119 ++++++++++++++++++++++------------------------- > 1 file changed, 56 insertions(+), 63 deletions(-) > > diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk > index 09f9412..65af6f0 100644 > --- a/package/pkg-autotools.mk > +++ b/package/pkg-autotools.mk > @@ -46,6 +46,57 @@ endef > # $(call AUTOCONF_AC_CHECK_FILE_VAL,/dev/random)=yes > AUTOCONF_AC_CHECK_FILE_VAL = ac_cv_file_$(subst -,_,$(subst /,_,$(subst .,_,$(1)))) > > +# Recipe that patches libtool so it works properly with > +# cross-compilation. > +define PATCH_LIBTOOL > + $(Q)if test "$($(PKG)_LIBTOOL_PATCH)" = "YES"; then \ > + for i in `find $($(PKG)_SRCDIR) -name ltmain.sh`; do \ > + ltmain_version=`sed -n '/^[ ]*VERSION=/{s/^[ ]*VERSION=//;p;q;}' $$$$i | \ > + sed -e 's/\([0-9].[0-9]*\).*/\1/' -e 's/\"//'`; \ > + if test $${ltmain_version} = 1.5; then \ > + $(APPLY_PATCHES) $${i%/*} support/libtool buildroot-libtool-v1.5.patch; \ > + elif test $${ltmain_version} = 2.2; then\ > + $(APPLY_PATCHES) $${i%/*} support/libtool buildroot-libtool-v2.2.patch; \ > + elif test $${ltmain_version} = 2.4; then\ > + $(APPLY_PATCHES) $${i%/*} support/libtool buildroot-libtool-v2.4.patch; \ > + fi \ > + done \ > + fi > +endef > + > +# > +# Hook to update config.sub and config.guess if needed > +# > +define UPDATE_CONFIG_HOOK > + @$(call MESSAGE,"Updating config.sub and config.guess") > + $(call CONFIG_UPDATE,$(@D)) > +endef > + > +# > +# Hook to patch libtool to make it work properly for cross-compilation > +# > +define LIBTOOL_PATCH_HOOK > + @$(call MESSAGE,"Patching libtool") > + $(call PATCH_LIBTOOL) > +endef Hmm... This patch does nore than extract the definitions out of inner-autotools-package; it also reworks the way they are called. I'd rather we get this split in at least two patches: - one that does the move proper - one that reworks the calls It will be easier to review. [--SNIP--] > @@ -265,6 +253,11 @@ $(2)_DEPENDENCIES += host-gettext > endif > $(2)_PRE_CONFIGURE_HOOKS += AUTORECONF_HOOK > $(2)_DEPENDENCIES += host-automake host-autoconf host-libtool > +else > +# default values are not evaluated yet, so don't rely on this defaulting to YES > +ifneq ($$($(2)_LIBTOOL_PATCH),NO) > +$(2)_POST_PATCH_HOOKS += LIBTOOL_PATCH_HOOK > +endif > endif I think you got the order wrong here: we want the libtool patch to be applied _before_ we autoreconf. So, this last if-block should be the very first thing to do right after: ifeq ($$($(2)_AUTORECONF),YES) I'll do some more in-depth review later, presumably after you split the patch. If you don;t have time for that, I can see to get it done tomorrow. Again, sorry for the delay, reviewing infra changes is really tricky... Regards, Yann E. MORIN.
johan, All, On 2014-11-10 23:13 +0100, Yann E. MORIN spake thusly: > On 2014-11-07 14:28 +0100, Johan Oudinet spake thusly: > > Define common macros only once (instead of as many times as > > there are inner-autotools-package calls). > > Factorize LIBTOOL_PATCH_HOOK and AUTORECONF_HOOK to avoid duplicated > > code. [--SNIP--] > I'll do some more in-depth review later, presumably after you split the > patch. If you don;t have time for that, I can see to get it done > tomorrow. So, are you working on this, or can I go hacking this? Regards, Yann E. MORIN.
Hi Johan, I'm mostly going to negate Yann's comments :-) I also have a few additional remarks, but they're not for this patch and probably not even for you. On 10/11/14 23:13, Yann E. MORIN wrote: > Johan, All, > > On 2014-11-07 14:28 +0100, Johan Oudinet spake thusly: >> Define common macros only once (instead of as many times as >> there are inner-autotools-package calls). >> Factorize LIBTOOL_PATCH_HOOK and AUTORECONF_HOOK to avoid duplicated >> code. > > Sorry it takes so long to review this series. Touching the infra is not > for the faint of heart, and reviews are really complex and time-consuming. _This_ is true... > >> Signed-off-by: Johan Oudinet <johan.oudinet@gmail.com> >> --- >> package/pkg-autotools.mk | 119 ++++++++++++++++++++++------------------------- >> 1 file changed, 56 insertions(+), 63 deletions(-) >> >> diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk >> index 09f9412..65af6f0 100644 >> --- a/package/pkg-autotools.mk >> +++ b/package/pkg-autotools.mk >> @@ -46,6 +46,57 @@ endef >> # $(call AUTOCONF_AC_CHECK_FILE_VAL,/dev/random)=yes >> AUTOCONF_AC_CHECK_FILE_VAL = ac_cv_file_$(subst -,_,$(subst /,_,$(subst .,_,$(1)))) >> >> +# Recipe that patches libtool so it works properly with >> +# cross-compilation. >> +define PATCH_LIBTOOL >> + $(Q)if test "$($(PKG)_LIBTOOL_PATCH)" = "YES"; then \ >> + for i in `find $($(PKG)_SRCDIR) -name ltmain.sh`; do \ >> + ltmain_version=`sed -n '/^[ ]*VERSION=/{s/^[ ]*VERSION=//;p;q;}' $$$$i | \ >> + sed -e 's/\([0-9].[0-9]*\).*/\1/' -e 's/\"//'`; \ >> + if test $${ltmain_version} = 1.5; then \ >> + $(APPLY_PATCHES) $${i%/*} support/libtool buildroot-libtool-v1.5.patch; \ >> + elif test $${ltmain_version} = 2.2; then\ >> + $(APPLY_PATCHES) $${i%/*} support/libtool buildroot-libtool-v2.2.patch; \ >> + elif test $${ltmain_version} = 2.4; then\ >> + $(APPLY_PATCHES) $${i%/*} support/libtool buildroot-libtool-v2.4.patch; \ >> + fi \ >> + done \ >> + fi >> +endef >> + >> +# >> +# Hook to update config.sub and config.guess if needed >> +# >> +define UPDATE_CONFIG_HOOK >> + @$(call MESSAGE,"Updating config.sub and config.guess") >> + $(call CONFIG_UPDATE,$(@D)) >> +endef >> + >> +# >> +# Hook to patch libtool to make it work properly for cross-compilation >> +# >> +define LIBTOOL_PATCH_HOOK >> + @$(call MESSAGE,"Patching libtool") This message could be hoisted into PATCH_LIBTOOL itself (or rather, PATCH_LIBTOOL could be moved down here). It makes total sense to display the 'Patching libtool' message even when autoreconf is done (and actually, currently it is displayed, just not at the right time). >> + $(call PATCH_LIBTOOL) >> +endef > > Hmm... This patch does nore than extract the definitions out of > inner-autotools-package; it also reworks the way they are called. > > I'd rather we get this split in at least two patches: > - one that does the move proper > - one that reworks the calls That is true. > > It will be easier to review. > > [--SNIP--] >> @@ -265,6 +253,11 @@ $(2)_DEPENDENCIES += host-gettext >> endif >> $(2)_PRE_CONFIGURE_HOOKS += AUTORECONF_HOOK For a separate patch, it would be nice to turn this into $(2)_PRE_CONFIGURE_HOOKS += AUTORECONF_HOOK ifneq ($$($(2)_LIBTOOL_PATCH),NO) $(2)_PRE_CONFIGURE_HOOKS += LIBTOOL_PATCH_HOOK endif and remove the libtool stuff from AUTORECONF_HOOK. With that done, the shell condition can be removed from the libtool hook as well. >> $(2)_DEPENDENCIES += host-automake host-autoconf host-libtool >> +else >> +# default values are not evaluated yet, so don't rely on this defaulting to YES >> +ifneq ($$($(2)_LIBTOOL_PATCH),NO) >> +$(2)_POST_PATCH_HOOKS += LIBTOOL_PATCH_HOOK >> +endif >> endif > > I think you got the order wrong here: we want the libtool patch to be > applied _before_ we autoreconf. No, we don't. We want it to be done after the autoreconf, because autoreconf generates the libtool scripts. In the original, there was a shell condition that evaluates that AUTORECONF is not YES in the LIBTOOL_PATCH_HOOK. So you would get the message that libtool is patched before the autoreconf, but the actual patching only happens after the reconf. Regards, Arnout > > So, this last if-block should be the very first thing to do right after: > ifeq ($$($(2)_AUTORECONF),YES) > > I'll do some more in-depth review later, presumably after you split the > patch. If you don;t have time for that, I can see to get it done > tomorrow. > > Again, sorry for the delay, reviewing infra changes is really tricky... > > Regards, > Yann E. MORIN. >
Arnout, All, On 2014-11-11 14:03 +0100, Arnout Vandecappelle spake thusly: > I'm mostly going to negate Yann's comments :-) Meh... :-) > >> $(2)_DEPENDENCIES += host-automake host-autoconf host-libtool > >> +else > >> +# default values are not evaluated yet, so don't rely on this defaulting to YES > >> +ifneq ($$($(2)_LIBTOOL_PATCH),NO) > >> +$(2)_POST_PATCH_HOOKS += LIBTOOL_PATCH_HOOK > >> +endif > >> endif > > > > I think you got the order wrong here: we want the libtool patch to be > > applied _before_ we autoreconf. > > No, we don't. We want it to be done after the autoreconf, because autoreconf > generates the libtool scripts. > > In the original, there was a shell condition that evaluates that AUTORECONF is > not YES in the LIBTOOL_PATCH_HOOK. So you would get the message that libtool is > patched before the autoreconf, but the actual patching only happens after the > reconf. Well, both of us are partialy wrong, I think. LIBTOOL_PATCH_HOOK is a post-patch hook, while GETTEXTIZE_HOOK and AUTORECONF_HOOK are pre-configure hooks. So, the order at which they are defined is irrelevant, as it is done right now. But I agree with Arnout, we need to rework this. The issue I can see is that, in case we're not autoreconfiguring, we can only apply the libtool patch once, and that has to be as a post-patch. But if we do autoreconf, it only makes sense to apply it after the autoreconf. One obvious thing to do would be to always apply it at post-patch, whether we autoreconf or not, and if we do autoreconf, also apply it after the autoreconf. After all, patching is not something that takes a lot of time, is it? That way, the code path is a bit more obvious. And move the 13-or-so lines that do the conditional patching out to a single function, so we can share it more easily. Sigh, I need some more coffee... Regards, Yann E. MORIN.
On 11/11/14 14:17, Yann E. MORIN wrote: > Arnout, All, > > On 2014-11-11 14:03 +0100, Arnout Vandecappelle spake thusly: >> I'm mostly going to negate Yann's comments :-) > > Meh... :-) > >>>> $(2)_DEPENDENCIES += host-automake host-autoconf host-libtool >>>> +else >>>> +# default values are not evaluated yet, so don't rely on this defaulting to YES >>>> +ifneq ($$($(2)_LIBTOOL_PATCH),NO) >>>> +$(2)_POST_PATCH_HOOKS += LIBTOOL_PATCH_HOOK >>>> +endif >>>> endif >>> >>> I think you got the order wrong here: we want the libtool patch to be >>> applied _before_ we autoreconf. >> >> No, we don't. We want it to be done after the autoreconf, because autoreconf >> generates the libtool scripts. >> >> In the original, there was a shell condition that evaluates that AUTORECONF is >> not YES in the LIBTOOL_PATCH_HOOK. So you would get the message that libtool is >> patched before the autoreconf, but the actual patching only happens after the >> reconf. > > Well, both of us are partialy wrong, I think. LIBTOOL_PATCH_HOOK is a > post-patch hook, while GETTEXTIZE_HOOK and AUTORECONF_HOOK are pre-configure > hooks. So, the order at which they are defined is irrelevant, as it is done > right now. > > But I agree with Arnout, we need to rework this. > > The issue I can see is that, in case we're not autoreconfiguring, we can > only apply the libtool patch once, and that has to be as a post-patch. > But if we do autoreconf, it only makes sense to apply it after the > autoreconf. > > One obvious thing to do would be to always apply it at post-patch, > whether we autoreconf or not, and if we do autoreconf, also apply it > after the autoreconf. After all, patching is not something that takes a > lot of time, is it? > > That way, the code path is a bit more obvious. > > And move the 13-or-so lines that do the conditional patching out to a > single function, so we can share it more easily. All this makes it pretty clear that reworking the call should be done in a separate patch :-) Regards, Arnout > > Sigh, I need some more coffee... > > Regards, > Yann E. MORIN. >
Arnout, Johan, All, On 2014-11-11 14:33 +0100, Arnout Vandecappelle spake thusly: > On 11/11/14 14:17, Yann E. MORIN wrote: [--SNIP--] > > Well, both of us are partialy wrong, I think. LIBTOOL_PATCH_HOOK is a > > post-patch hook, while GETTEXTIZE_HOOK and AUTORECONF_HOOK are pre-configure > > hooks. So, the order at which they are defined is irrelevant, as it is done > > right now. > > > > But I agree with Arnout, we need to rework this. [--SNIP--] > All this makes it pretty clear that reworking the call should be done in a > separate patch :-) I'm mostly done with just the autotools rework. It's no longer a single patch, and not even two. It's a 7-patch series, now: db9396b pkg-autotools: move common macros aaecaca pkg-autotools: re-order cleaning up the host dependencies aaf7f5a pkg-autotools: commonalise the libtool patching code 54e296d pkg-autotools: move the libtool patching call out of the autoreconf hook 541764e pkg-autotools: remove redundant shell conditional 120fc9c pkg-autotools: fold the libtool patching code directly into the hook 77feedc pkg-autotools: only apply libtool patch at the right moment I'll post those later tonight. I still hope to have time to rework the pkg-rebar patch today... Regards, Yann E. MORIN.
diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk index 09f9412..65af6f0 100644 --- a/package/pkg-autotools.mk +++ b/package/pkg-autotools.mk @@ -46,6 +46,57 @@ endef # $(call AUTOCONF_AC_CHECK_FILE_VAL,/dev/random)=yes AUTOCONF_AC_CHECK_FILE_VAL = ac_cv_file_$(subst -,_,$(subst /,_,$(subst .,_,$(1)))) +# Recipe that patches libtool so it works properly with +# cross-compilation. +define PATCH_LIBTOOL + $(Q)if test "$($(PKG)_LIBTOOL_PATCH)" = "YES"; then \ + for i in `find $($(PKG)_SRCDIR) -name ltmain.sh`; do \ + ltmain_version=`sed -n '/^[ ]*VERSION=/{s/^[ ]*VERSION=//;p;q;}' $$$$i | \ + sed -e 's/\([0-9].[0-9]*\).*/\1/' -e 's/\"//'`; \ + if test $${ltmain_version} = 1.5; then \ + $(APPLY_PATCHES) $${i%/*} support/libtool buildroot-libtool-v1.5.patch; \ + elif test $${ltmain_version} = 2.2; then\ + $(APPLY_PATCHES) $${i%/*} support/libtool buildroot-libtool-v2.2.patch; \ + elif test $${ltmain_version} = 2.4; then\ + $(APPLY_PATCHES) $${i%/*} support/libtool buildroot-libtool-v2.4.patch; \ + fi \ + done \ + fi +endef + +# +# Hook to update config.sub and config.guess if needed +# +define UPDATE_CONFIG_HOOK + @$(call MESSAGE,"Updating config.sub and config.guess") + $(call CONFIG_UPDATE,$(@D)) +endef + +# +# Hook to patch libtool to make it work properly for cross-compilation +# +define LIBTOOL_PATCH_HOOK + @$(call MESSAGE,"Patching libtool") + $(call PATCH_LIBTOOL) +endef + +# +# Hook to gettextize the package if needed +# +define GETTEXTIZE_HOOK + @$(call MESSAGE,"Gettextizing") + $(Q)cd $($(PKG)_SRCDIR) && $(GETTEXTIZE) $($(PKG)_GETTEXTIZE_OPTS) +endef + +# +# Hook to autoreconf the package if needed +# +define AUTORECONF_HOOK + @$(call MESSAGE,"Autoreconfiguring") + $(Q)cd $($(PKG)_SRCDIR) && $($(PKG)_AUTORECONF_ENV) $(AUTORECONF) $($(PKG)_AUTORECONF_OPTS) + $(call PATCH_LIBTOOL) +endef + ################################################################################ # inner-autotools-package -- defines how the configuration, compilation and # installation of an autotools package should be done, implements a @@ -183,71 +234,8 @@ endef endif endif -# -# Hook to update config.sub and config.guess if needed -# -define UPDATE_CONFIG_HOOK - @$$(call MESSAGE,"Updating config.sub and config.guess") - $$(call CONFIG_UPDATE,$$(@D)) -endef - $(2)_POST_PATCH_HOOKS += UPDATE_CONFIG_HOOK -# -# Hook to patch libtool to make it work properly for cross-compilation -# -define LIBTOOL_PATCH_HOOK - @$$(call MESSAGE,"Patching libtool") - $$(Q)if test "$$($$(PKG)_LIBTOOL_PATCH)" = "YES" \ - -a "$$($$(PKG)_AUTORECONF)" != "YES"; then \ - for i in `find $$($$(PKG)_SRCDIR) -name ltmain.sh`; do \ - ltmain_version=`sed -n '/^[ ]*VERSION=/{s/^[ ]*VERSION=//;p;q;}' $$$$i | \ - sed -e 's/\([0-9].[0-9]*\).*/\1/' -e 's/\"//'`; \ - if test $$$${ltmain_version} = '1.5'; then \ - $$(APPLY_PATCHES) $$$${i%/*} support/libtool buildroot-libtool-v1.5.patch; \ - elif test $$$${ltmain_version} = "2.2"; then\ - $$(APPLY_PATCHES) $$$${i%/*} support/libtool buildroot-libtool-v2.2.patch; \ - elif test $$$${ltmain_version} = "2.4"; then\ - $$(APPLY_PATCHES) $$$${i%/*} support/libtool buildroot-libtool-v2.4.patch; \ - fi \ - done \ - fi -endef - -# default values are not evaluated yet, so don't rely on this defaulting to YES -ifneq ($$($(2)_LIBTOOL_PATCH),NO) -$(2)_POST_PATCH_HOOKS += LIBTOOL_PATCH_HOOK -endif - -# -# Hook to gettextize the package if needed -# -define GETTEXTIZE_HOOK - @$$(call MESSAGE,"Gettextizing") - $(Q)cd $$($$(PKG)_SRCDIR) && $$(GETTEXTIZE) $$($$(PKG)_GETTEXTIZE_OPTS) -endef - -# -# Hook to autoreconf the package if needed -# -define AUTORECONF_HOOK - @$$(call MESSAGE,"Autoreconfiguring") - $$(Q)cd $$($$(PKG)_SRCDIR) && $$($$(PKG)_AUTORECONF_ENV) $$(AUTORECONF) $$($$(PKG)_AUTORECONF_OPTS) - $$(Q)if test "$$($$(PKG)_LIBTOOL_PATCH)" = "YES"; then \ - for i in `find $$($$(PKG)_SRCDIR) -name ltmain.sh`; do \ - ltmain_version=`sed -n '/^[ ]*VERSION=/{s/^[ ]*VERSION=//;p;q;}' $$$$i | \ - sed -e 's/\([0-9].[0-9]*\).*/\1/' -e 's/\"//'`; \ - if test $$$${ltmain_version} = "1.5"; then \ - $$(APPLY_PATCHES) $$$${i%/*} support/libtool buildroot-libtool-v1.5.patch; \ - elif test $$$${ltmain_version} = "2.2"; then\ - $$(APPLY_PATCHES) $$$${i%/*} support/libtool buildroot-libtool-v2.2.patch; \ - elif test $$$${ltmain_version} = "2.4"; then\ - $$(APPLY_PATCHES) $$$${i%/*} support/libtool buildroot-libtool-v2.4.patch; \ - fi \ - done \ - fi -endef - # This must be repeated from inner-generic-package, otherwise we get an empty # _DEPENDENCIES if _AUTORECONF is YES. Also filter the result of _AUTORECONF # and _GETTEXTIZE away from the non-host rule @@ -265,6 +253,11 @@ $(2)_DEPENDENCIES += host-gettext endif $(2)_PRE_CONFIGURE_HOOKS += AUTORECONF_HOOK $(2)_DEPENDENCIES += host-automake host-autoconf host-libtool +else +# default values are not evaluated yet, so don't rely on this defaulting to YES +ifneq ($$($(2)_LIBTOOL_PATCH),NO) +$(2)_POST_PATCH_HOOKS += LIBTOOL_PATCH_HOOK +endif endif #
Define common macros only once (instead of as many times as there are inner-autotools-package calls). Factorize LIBTOOL_PATCH_HOOK and AUTORECONF_HOOK to avoid duplicated code. Signed-off-by: Johan Oudinet <johan.oudinet@gmail.com> --- package/pkg-autotools.mk | 119 ++++++++++++++++++++++------------------------- 1 file changed, 56 insertions(+), 63 deletions(-)