Message ID | 1358725943-31485-6-git-send-email-thomas.petazzoni@free-electrons.com |
---|---|
State | Changes Requested |
Headers | show |
On 01/21/13 00:52, Thomas Petazzoni wrote: > Doing the autoreconf step as a pre-configure hook doesn't work > properly, because the source directory is read-only during the > configure step. And in fact, the autoreconf process modifies the > source code, so it is quite logical to do it as part of the patching > process rather than the configuration process. It was in fact moved from POST_PATCH to PRE_CONFIGURE in commit 89d1ad91fe4b1b65f0e902f94aba99a6cefbf631, and the PRE_CONFIGURE_HOOKS were added specifically for this purpose. Unfortunately, the commit gives no explanation as to why it was needed. Regards, Arnout > > Signed-off-by: Thomas Petazzoni<thomas.petazzoni@free-electrons.com> > --- > package/pkg-autotools.mk | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk > index bd87253..ec394fd 100644 > --- a/package/pkg-autotools.mk > +++ b/package/pkg-autotools.mk > @@ -214,7 +214,7 @@ $(2)_DEPENDENCIES ?= $(filter-out host-automake host-autoconf host-libtool $(1), > > > ifeq ($$($(2)_AUTORECONF),YES) > -$(2)_PRE_CONFIGURE_HOOKS += AUTORECONF_HOOK > +$(2)_POST_PATCH_HOOKS += AUTORECONF_HOOK > $(2)_DEPENDENCIES += host-automake host-autoconf host-libtool > endif >
Dear Arnout Vandecappelle, On Sat, 26 Jan 2013 23:03:21 +0100, Arnout Vandecappelle wrote: > On 01/21/13 00:52, Thomas Petazzoni wrote: > > Doing the autoreconf step as a pre-configure hook doesn't work > > properly, because the source directory is read-only during the > > configure step. And in fact, the autoreconf process modifies the > > source code, so it is quite logical to do it as part of the patching > > process rather than the configuration process. > > It was in fact moved from POST_PATCH to PRE_CONFIGURE in commit > 89d1ad91fe4b1b65f0e902f94aba99a6cefbf631, and the PRE_CONFIGURE_HOOKS > were added specifically for this purpose. Unfortunately, the commit > gives no explanation as to why it was needed. Hum, right. Interesting. I've looked at the commits around this one, and also the e-mails around the one through which this patch was submitted, but it didn't give any clue. The other patches around are version bumps and other, seemingly unrelated things. It's even more disappointing that I was amongst the people giving a Acked-by on this patch... One reason that might explain this is the lack of ordering guarantees on post patch hooks. For example, if you have a post patch hook that applies Debian patches, and a post patch hook to do the autoreconf, you quite certainly want the Debian patches hook to be executed before the autoreconf hook. But I have absolutely no idea if this is the problem that we were trying to fix here. That said, I still believe that the autoreconf thing belongs to the patch step. It is really a modification of the source code itself, and it is common to both the target build and host build. So maybe the autotools infrastructure needs a special hook in the generic infrastructure (rather than a normal post patch hook), to ensure that the autoreconf step gets executed after all post patch hooks? Thomas
On 27/01/13 17:27, Thomas Petazzoni wrote: > One reason that might explain this is the lack of ordering guarantees > on post patch hooks. For example, if you have a post patch hook that > applies Debian patches, and a post patch hook to do the autoreconf, you > quite certainly want the Debian patches hook to be executed before the > autoreconf hook. But I have absolutely no idea if this is the problem > that we were trying to fix here. That sounds like a likely explanation... > That said, I still believe that the autoreconf thing belongs to the > patch step. It is really a modification of the source code itself, and > it is common to both the target build and host build. I completely agree. > So maybe the > autotools infrastructure needs a special hook in the generic > infrastructure (rather than a normal post patch hook), to ensure that > the autoreconf step gets executed after all post patch hooks? Or maybe (just thinking out loud here) there could be the possibility for the package infrastructure to add additional steps in the dependency chain. Something like ifeq ($$($(2)_AUTORECONF),YES) $(2)_TARGET_AUTORECONFIGURE = $$($(3)_SRCDIR)/.stamp_autoreconfigured $(1)-autoreconfigure: $(1)-patch $$($(2)_TARGET_AUTORECONFIGURE) $(1)-configure: $(1)-autoreconfigure endif Regards, Arnout
Dear Arnout Vandecappelle, On Sun, 27 Jan 2013 23:18:39 +0100, Arnout Vandecappelle wrote: > > So maybe the > > autotools infrastructure needs a special hook in the generic > > infrastructure (rather than a normal post patch hook), to ensure > > that the autoreconf step gets executed after all post patch hooks? > > Or maybe (just thinking out loud here) there could be the > possibility for the package infrastructure to add additional steps in > the dependency chain. Something like > > ifeq ($$($(2)_AUTORECONF),YES) > $(2)_TARGET_AUTORECONFIGURE = $$($(3)_SRCDIR)/.stamp_autoreconfigured > $(1)-autoreconfigure: $(1)-patch $$($(2)_TARGET_AUTORECONFIGURE) > $(1)-configure: $(1)-autoreconfigure > endif Sounds interesting. I'll try to experiment around this idea. Thanks, Thomas
diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk index bd87253..ec394fd 100644 --- a/package/pkg-autotools.mk +++ b/package/pkg-autotools.mk @@ -214,7 +214,7 @@ $(2)_DEPENDENCIES ?= $(filter-out host-automake host-autoconf host-libtool $(1), ifeq ($$($(2)_AUTORECONF),YES) -$(2)_PRE_CONFIGURE_HOOKS += AUTORECONF_HOOK +$(2)_POST_PATCH_HOOKS += AUTORECONF_HOOK $(2)_DEPENDENCIES += host-automake host-autoconf host-libtool endif
Doing the autoreconf step as a pre-configure hook doesn't work properly, because the source directory is read-only during the configure step. And in fact, the autoreconf process modifies the source code, so it is quite logical to do it as part of the patching process rather than the configuration process. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- package/pkg-autotools.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)