| Message ID | 1357596078-7762-2-git-send-email-stefan.froberg@petroprogram.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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
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
Signed-off-by: Stefan Fröberg <stefan.froberg@petroprogram.com> --- package/pkg-generic.mk | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)