Patchwork new variable <pkg>_CONFIG_FIXUP

login
register
mail settings
Submitter Stefan Fröberg
Date Jan. 7, 2013, 10:01 p.m.
Message ID <1357596078-7762-2-git-send-email-stefan.froberg@petroprogram.com>
Download mbox | patch
Permalink /patch/210249/
State Superseded
Headers show

Comments

Stefan Fröberg - Jan. 7, 2013, 10:01 p.m.
Signed-off-by: Stefan Fröberg <stefan.froberg@petroprogram.com>
---
 package/pkg-generic.mk |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
Thomas Petazzoni - Jan. 7, 2013, 10:10 p.m.
Dear Stefan Fröberg,

On Tue,  8 Jan 2013 00:01:18 +0200, Stefan Fröberg wrote:
> Signed-off-by: Stefan Fröberg <stefan.froberg@petroprogram.com>

Please add more details to your commit log. I.e, basically, most of your
introduction e-mail contents should go here.

Remember: the contents of your introduction e-mail are not preserved in
the Git history. On the opposite, the commit log is preserved forever
in the Git history, so we want the details to be here.

> ---
>  package/pkg-generic.mk |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index a570ad7..a410ad1 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -121,6 +121,14 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
>  	@$(call MESSAGE,"Installing to staging directory")
>  	$($(PKG)_INSTALL_STAGING_CMDS)
>  	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
> +	@$(call MESSAGE,"Fixing package configuration files")

Please show the message only if there are actually files to fixup. Or
maybe just don't display any message at all. Fixing those files is kind
of a detail.

> +	for file in $($(PKG)_CONFIG_FIXUP); do \
> +		if [ -e $(STAGING_DIR)/usr/bin/$${file} ]; then \

No, we should not silently error out if the file doesn't exist, because
it's hiding a mistake of package .mk file. Instead, if the file doesn't
exist, we should loudly error out.

Also, please include an update to the Buildroot manual, either as part
of this patch, or as a separate patch.

Thanks for your work!

Thomas
Stefan Fröberg - Jan. 7, 2013, 10:23 p.m.
Hi Thomas

8.1.2013 0:10, Thomas Petazzoni kirjoitti:
> Dear Stefan Fröberg,
>
> On Tue,  8 Jan 2013 00:01:18 +0200, Stefan Fröberg wrote:
>> Signed-off-by: Stefan Fröberg <stefan.froberg@petroprogram.com>
> Please add more details to your commit log. I.e, basically, most of your
> introduction e-mail contents should go here.
Okay
> Remember: the contents of your introduction e-mail are not preserved in
> the Git history. On the opposite, the commit log is preserved forever
> in the Git history, so we want the details to be here.
>
>> ---
>>  package/pkg-generic.mk |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>> index a570ad7..a410ad1 100644
>> --- a/package/pkg-generic.mk
>> +++ b/package/pkg-generic.mk
>> @@ -121,6 +121,14 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
>>  	@$(call MESSAGE,"Installing to staging directory")
>>  	$($(PKG)_INSTALL_STAGING_CMDS)
>>  	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
>> +	@$(call MESSAGE,"Fixing package configuration files")
> Please show the message only if there are actually files to fixup. Or
> maybe just don't display any message at all. Fixing those files is kind
> of a detail.

You mean checking first in if-statement if  $($(PKG)_CONFIG_FIXUP exist ?
What is the best way to detect if it exist ?

>> +	for file in $($(PKG)_CONFIG_FIXUP); do \
>> +		if [ -e $(STAGING_DIR)/usr/bin/$${file} ]; then \
> No, we should not silently error out if the file doesn't exist, because
> it's hiding a mistake of package .mk file. Instead, if the file doesn't
> exist, we should loudly error out.

Okay

> Also, please include an update to the Buildroot manual, either as part
> of this patch, or as a separate patch.

Oh my, I really suck when doing any documentation stuff.
But ill try.....

> Thanks for your work!
>
> Thomas

Thank you!

Stefan
Thomas Petazzoni - Jan. 7, 2013, 10:42 p.m.
Dear Stefan Fröberg,

On Tue, 08 Jan 2013 00:23:33 +0200, Stefan Fröberg wrote:

> You mean checking first in if-statement if  $($(PKG)_CONFIG_FIXUP exist ?
> What is the best way to detect if it exist ?

Just test if the variable is not empty, i.e test -n.

Like (untested):

	if test -n $($(PKG)_CONFIG_FIXUP) ; then \
		$(call MESSAGE, "Blabla") ; \
		for file in ...
			...
		done
	fi

Thomas
Arnout Vandecappelle - Jan. 8, 2013, 8:53 p.m.
On 01/07/13 23:42, Thomas Petazzoni wrote:
> Dear Stefan Fröberg,
>
> On Tue, 08 Jan 2013 00:23:33 +0200, Stefan Fröberg wrote:
>
>> You mean checking first in if-statement if  $($(PKG)_CONFIG_FIXUP exist ?
>> What is the best way to detect if it exist ?
>
> Just test if the variable is not empty, i.e test -n.
>
> Like (untested):
>
> 	if test -n $($(PKG)_CONFIG_FIXUP) ; then \
> 		$(call MESSAGE, "Blabla") ; \
> 		for file in ...
> 			...
> 		done
> 	fi

  The test isn't necessary, the loop will not execute if it is empty.

  Stefan, sed will give an appropriate error if the file doesn't exist.

  Regards,
  Arnout
Arnout Vandecappelle - Jan. 8, 2013, 9:10 p.m.
On 01/07/13 23:01, Stefan Fröberg wrote:
> Signed-off-by: Stefan Fröberg<stefan.froberg@petroprogram.com>
> ---
>   package/pkg-generic.mk |    8 ++++++++
>   1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index a570ad7..a410ad1 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -121,6 +121,14 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
>   	@$(call MESSAGE,"Installing to staging directory")
>   	$($(PKG)_INSTALL_STAGING_CMDS)
>   	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
> +	@$(call MESSAGE,"Fixing package configuration files")
> +	for file in $($(PKG)_CONFIG_FIXUP); do \
> +		if [ -e $(STAGING_DIR)/usr/bin/$${file} ]; then \
> +			$(SED) "s,^prefix=.*,prefix=\'$(STAGING_DIR)/usr\',g" \

 The \' are redundant, just put ' (it's already between double quotes).

> +				-e "s,^exec_prefix=.*,exec_prefix=\'$(STAGING_DIR)/usr\',g" \
> +				$(STAGING_DIR)/usr/bin/$${file} ;\
> +		fi \
> +	done

 Since sed -i is used, you can actually pass all files together. 
Something like:

if [ "$($(PKG)_CONFIG_FIXUP)" ]; then \
	$(SED) ... \
		$(addprefix $(STAGING_DIR)/usr/bin,$($(PKG)_CONFIG_FIXUP))); \
fi


 Regards,
 Arnout

>   	$(Q)touch $@
> 
>   # Install to images dir
Thomas Petazzoni - Jan. 8, 2013, 9:10 p.m.
Dear Arnout Vandecappelle,

On Tue, 08 Jan 2013 21:53:45 +0100, Arnout Vandecappelle wrote:

> > Like (untested):
> >
> > 	if test -n $($(PKG)_CONFIG_FIXUP) ; then \
> > 		$(call MESSAGE, "Blabla") ; \
> > 		for file in ...
> > 			...
> > 		done
> > 	fi
> 
>   The test isn't necessary, the loop will not execute if it is empty.

See the original discussion. The test is necessary to not show the
message "Fixing package configuration files" if there are no files to
fix, i.e if the <PKG>_CONFIG_FIXUP variable is empty.

Best regards,

Thomas

Patch

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index a570ad7..a410ad1 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -121,6 +121,14 @@  $(BUILD_DIR)/%/.stamp_staging_installed:
 	@$(call MESSAGE,"Installing to staging directory")
 	$($(PKG)_INSTALL_STAGING_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
+	@$(call MESSAGE,"Fixing package configuration files")
+	for file in $($(PKG)_CONFIG_FIXUP); do \
+		if [ -e $(STAGING_DIR)/usr/bin/$${file} ]; then \
+			$(SED) "s,^prefix=.*,prefix=\'$(STAGING_DIR)/usr\',g" \
+				-e "s,^exec_prefix=.*,exec_prefix=\'$(STAGING_DIR)/usr\',g" \
+				$(STAGING_DIR)/usr/bin/$${file} ;\
+		fi \
+	done
 	$(Q)touch $@
 
 # Install to images dir