[Buildroot] [PATCH] package/qoriq-cadence-dp-firmware: refactor with FREESCALE_IMX_EXTRACT_HELPER

[Buildroot] [PATCH] package/qoriq-cadence-dp-firmware: refactor with FREESCALE_IMX_EXTRACT_HELPER

[Francois Perrad]

avoid code duplication

Signed-off-by: Francois Perrad <francois.perrad@gadz.org>
---
 .../qoriq-cadence-dp-firmware.mk              | 27 +------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk b/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
index 23643d24e..1a97c5803 100644
--- a/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
+++ b/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
@@ -13,33 +13,8 @@ QORIQ_CADENCE_DP_FIRMWARE_REDISTRIBUTE = NO
 QORIQ_CADENCE_DP_FIRMWARE_INSTALL_IMAGES = YES
 QORIQ_CADENCE_DP_FIRMWARE_INSTALL_TARGET = NO
 
-# Helper for self-extracting binaries distributed by NXP.
-#
-# The --force option makes sure it doesn't fail if the source
-# directory already exists. The --auto-accept skips the license check,
-# as it is not needed in Buildroot because we have legal-info. Since
-# there's a EULA in the binary file, we extract it in this macro, and
-# it should therefore be added to the LICENSE_FILES variable of
-# packages using this macro. Also, remember to set REDISTRIBUTE to
-# "NO". Indeed, this is a legal minefield: the EULA specifies that the
-# Board Support Package includes software and hardware (sic!) for
-# which a separate license is needed...
-#
-# $(1): full path to the archive file
-#
-define QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_HELPER
-	awk 'BEGIN      { start = 0; } \
-	     /^EOEULA/  { start = 0; } \
-	                { if (start) print; } \
-	     /<<EOEULA/ { start = 1; }' \
-	    $(1) > $(@D)/EULA
-	cd $(@D) && sh $(1) --force --auto-accept
-	find $(@D)/$(basename $(notdir $(1))) -mindepth 1 -maxdepth 1 -exec mv {} $(@D) \;
-	rmdir $(@D)/$(basename $(notdir $(1)))
-endef
-
 define QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_CMDS
-	$(call QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_HELPER,$(QORIQ_CADENCE_DP_FIRMWARE_DL_DIR)/$(QORIQ_CADENCE_DP_FIRMWARE_SOURCE))
+	$(call FREESCALE_IMX_EXTRACT_HELPER,$(QORIQ_CADENCE_DP_FIRMWARE_DL_DIR)/$(QORIQ_CADENCE_DP_FIRMWARE_SOURCE))
 endef
 
 define QORIQ_CADENCE_DP_FIRMWARE_INSTALL_IMAGES_CMDS
-- 
2.34.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/qoriq-cadence-dp-firmware: refactor with FREESCALE_IMX_EXTRACT_HELPER

[Yann E. MORIN]

François, All,

+Arnout would applied qoriq-cadence-dp-firmware.

On 2022-08-14 14:56 +0200, Francois Perrad spake thusly:
> avoid code duplication

I understand the udnerlying reason, but I am not too fond of it.

The FREESCALE_IMX_EXTRACT_HELPER really bears the name "Freescale" in
it, and if we need to change it for Freescale-related packages, it will
not be obvious that it can also be used by other, non-Freescale
packages.

Also, when we have packages that share some infrastructure, we like them
to be in a sub-directory of a common dir (like qt5 and all the Qt 5.x
packages, or like indeed freescale-imx and all relatedpackages). But
this qoriq-cadence-dp-firmware is not part of that freescale-imx
collection.

So, in this case, I prefer we keep the duplication.

Yeah, Freescale is now part of NXP, but this is not a reason yet.

Also, note that the extract helper does extract a fie named EULA, but it
it not present in LICENSE_FILES, even though the comment states it
should be.

Additionally, one extracted, there is also an EULA.txt file that is not
in LICENSE_FILES wither, which I think it should be.

Can you address those last two bits in a separate patch, please?

Regards,
Yann E. MORIN.

> Signed-off-by: Francois Perrad <francois.perrad@gadz.org>
> ---
>  .../qoriq-cadence-dp-firmware.mk              | 27 +------------------
>  1 file changed, 1 insertion(+), 26 deletions(-)
> 
> diff --git a/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk b/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
> index 23643d24e..1a97c5803 100644
> --- a/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
> +++ b/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
> @@ -13,33 +13,8 @@ QORIQ_CADENCE_DP_FIRMWARE_REDISTRIBUTE = NO
>  QORIQ_CADENCE_DP_FIRMWARE_INSTALL_IMAGES = YES
>  QORIQ_CADENCE_DP_FIRMWARE_INSTALL_TARGET = NO
>  
> -# Helper for self-extracting binaries distributed by NXP.
> -#
> -# The --force option makes sure it doesn't fail if the source
> -# directory already exists. The --auto-accept skips the license check,
> -# as it is not needed in Buildroot because we have legal-info. Since
> -# there's a EULA in the binary file, we extract it in this macro, and
> -# it should therefore be added to the LICENSE_FILES variable of
> -# packages using this macro. Also, remember to set REDISTRIBUTE to
> -# "NO". Indeed, this is a legal minefield: the EULA specifies that the
> -# Board Support Package includes software and hardware (sic!) for
> -# which a separate license is needed...
> -#
> -# $(1): full path to the archive file
> -#
> -define QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_HELPER
> -	awk 'BEGIN      { start = 0; } \
> -	     /^EOEULA/  { start = 0; } \
> -	                { if (start) print; } \
> -	     /<<EOEULA/ { start = 1; }' \
> -	    $(1) > $(@D)/EULA
> -	cd $(@D) && sh $(1) --force --auto-accept
> -	find $(@D)/$(basename $(notdir $(1))) -mindepth 1 -maxdepth 1 -exec mv {} $(@D) \;
> -	rmdir $(@D)/$(basename $(notdir $(1)))
> -endef
> -
>  define QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_CMDS
> -	$(call QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_HELPER,$(QORIQ_CADENCE_DP_FIRMWARE_DL_DIR)/$(QORIQ_CADENCE_DP_FIRMWARE_SOURCE))
> +	$(call FREESCALE_IMX_EXTRACT_HELPER,$(QORIQ_CADENCE_DP_FIRMWARE_DL_DIR)/$(QORIQ_CADENCE_DP_FIRMWARE_SOURCE))
>  endef
>  
>  define QORIQ_CADENCE_DP_FIRMWARE_INSTALL_IMAGES_CMDS
> -- 
> 2.34.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/qoriq-cadence-dp-firmware: refactor with FREESCALE_IMX_EXTRACT_HELPER

[Arnout Vandecappelle]

On 14/08/2022 17:04, Yann E. MORIN wrote:
> François, All,
> 
> +Arnout would applied qoriq-cadence-dp-firmware.
> 
> On 2022-08-14 14:56 +0200, Francois Perrad spake thusly:
>> avoid code duplication
> 
> I understand the udnerlying reason, but I am not too fond of it.
> 
> The FREESCALE_IMX_EXTRACT_HELPER really bears the name "Freescale" in
> it, and if we need to change it for Freescale-related packages, it will
> not be obvious that it can also be used by other, non-Freescale
> packages.

  Well, the name Freescale in there is legacy, it should really be called 
NXP_IMX_EXTRACT_HELPER - or even NXP_EXTRACT_HELPER (it's not just IMX 
platforms, since it's apparently also used for QorIQ). Basically everything that 
currently has some form of freescale in the name should be converted to NXP. But 
we don't like that kind of useless churn, so things stay as they are.


> Also, when we have packages that share some infrastructure, we like them
> to be in a sub-directory of a common dir (like qt5 and all the Qt 5.x
> packages, or like indeed freescale-imx and all relatedpackages). But
> this qoriq-cadence-dp-firmware is not part of that freescale-imx
> collection.

  This, on the other hand, is pretty essential IMHO.

  The solution in this case may be to move it to a global place, e.g. 
package/pkg-utils. With the number of places where we need this crappy 
extractor, it can be considered infra on the level of the gitlab helper.

  So, I propose 4 patches.

1. Fix license files as described below by Yann.
2. Move FREESCALE_IMX_EXTRACT_HELPER to package/pkg-utils.mk.
3. Rename FREESCALE_IMX_EXTRACT_HELPER to NXP_EXTRACT_HELPER.
4. Use NXP_EXTRACT_HELPER in qoriq-cadence-dp-firmware.

  Yann, do you agree?

  Regards,
  Arnout

> 
> So, in this case, I prefer we keep the duplication.
> 
> Yeah, Freescale is now part of NXP, but this is not a reason yet.
> 
> Also, note that the extract helper does extract a fie named EULA, but it
> it not present in LICENSE_FILES, even though the comment states it
> should be.
> 
> Additionally, one extracted, there is also an EULA.txt file that is not
> in LICENSE_FILES wither, which I think it should be.
> 
> Can you address those last two bits in a separate patch, please?
> 
> Regards,
> Yann E. MORIN.
> 
>> Signed-off-by: Francois Perrad <francois.perrad@gadz.org>
>> ---
>>   .../qoriq-cadence-dp-firmware.mk              | 27 +------------------
>>   1 file changed, 1 insertion(+), 26 deletions(-)
>>
>> diff --git a/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk b/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
>> index 23643d24e..1a97c5803 100644
>> --- a/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
>> +++ b/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
>> @@ -13,33 +13,8 @@ QORIQ_CADENCE_DP_FIRMWARE_REDISTRIBUTE = NO
>>   QORIQ_CADENCE_DP_FIRMWARE_INSTALL_IMAGES = YES
>>   QORIQ_CADENCE_DP_FIRMWARE_INSTALL_TARGET = NO
>>   
>> -# Helper for self-extracting binaries distributed by NXP.
>> -#
>> -# The --force option makes sure it doesn't fail if the source
>> -# directory already exists. The --auto-accept skips the license check,
>> -# as it is not needed in Buildroot because we have legal-info. Since
>> -# there's a EULA in the binary file, we extract it in this macro, and
>> -# it should therefore be added to the LICENSE_FILES variable of
>> -# packages using this macro. Also, remember to set REDISTRIBUTE to
>> -# "NO". Indeed, this is a legal minefield: the EULA specifies that the
>> -# Board Support Package includes software and hardware (sic!) for
>> -# which a separate license is needed...
>> -#
>> -# $(1): full path to the archive file
>> -#
>> -define QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_HELPER
>> -	awk 'BEGIN      { start = 0; } \
>> -	     /^EOEULA/  { start = 0; } \
>> -	                { if (start) print; } \
>> -	     /<<EOEULA/ { start = 1; }' \
>> -	    $(1) > $(@D)/EULA
>> -	cd $(@D) && sh $(1) --force --auto-accept
>> -	find $(@D)/$(basename $(notdir $(1))) -mindepth 1 -maxdepth 1 -exec mv {} $(@D) \;
>> -	rmdir $(@D)/$(basename $(notdir $(1)))
>> -endef
>> -
>>   define QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_CMDS
>> -	$(call QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_HELPER,$(QORIQ_CADENCE_DP_FIRMWARE_DL_DIR)/$(QORIQ_CADENCE_DP_FIRMWARE_SOURCE))
>> +	$(call FREESCALE_IMX_EXTRACT_HELPER,$(QORIQ_CADENCE_DP_FIRMWARE_DL_DIR)/$(QORIQ_CADENCE_DP_FIRMWARE_SOURCE))
>>   endef
>>   
>>   define QORIQ_CADENCE_DP_FIRMWARE_INSTALL_IMAGES_CMDS
>> -- 
>> 2.34.1
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@buildroot.org
>> https://lists.buildroot.org/mailman/listinfo/buildroot
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/qoriq-cadence-dp-firmware: refactor with FREESCALE_IMX_EXTRACT_HELPER

[Yann E. MORIN]

Arnout, All,

On 2022-08-14 17:56 +0200, Arnout Vandecappelle spake thusly:
> On 14/08/2022 17:04, Yann E. MORIN wrote:
> >On 2022-08-14 14:56 +0200, Francois Perrad spake thusly:
> >>avoid code duplication
[--SNIP--]
> >Also, when we have packages that share some infrastructure, we like them
> >to be in a sub-directory of a common dir (like qt5 and all the Qt 5.x
> >packages, or like indeed freescale-imx and all relatedpackages). But
> >this qoriq-cadence-dp-firmware is not part of that freescale-imx
> >collection.
> 
>  This, on the other hand, is pretty essential IMHO.
> 
>  The solution in this case may be to move it to a global place, e.g.
> package/pkg-utils. With the number of places where we need this crappy
> extractor, it can be considered infra on the level of the gitlab helper.
> 
>  So, I propose 4 patches.
> 
> 1. Fix license files as described below by Yann.
> 2. Move FREESCALE_IMX_EXTRACT_HELPER to package/pkg-utils.mk.
> 3. Rename FREESCALE_IMX_EXTRACT_HELPER to NXP_EXTRACT_HELPER.
> 4. Use NXP_EXTRACT_HELPER in qoriq-cadence-dp-firmware.
> 
>  Yann, do you agree?

Yes, this is fine with me.

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> >
> >So, in this case, I prefer we keep the duplication.
> >
> >Yeah, Freescale is now part of NXP, but this is not a reason yet.
> >
> >Also, note that the extract helper does extract a fie named EULA, but it
> >it not present in LICENSE_FILES, even though the comment states it
> >should be.
> >
> >Additionally, one extracted, there is also an EULA.txt file that is not
> >in LICENSE_FILES wither, which I think it should be.
> >
> >Can you address those last two bits in a separate patch, please?
> >
> >Regards,
> >Yann E. MORIN.
> >
> >>Signed-off-by: Francois Perrad <francois.perrad@gadz.org>
> >>---
> >>  .../qoriq-cadence-dp-firmware.mk              | 27 +------------------
> >>  1 file changed, 1 insertion(+), 26 deletions(-)
> >>
> >>diff --git a/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk b/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
> >>index 23643d24e..1a97c5803 100644
> >>--- a/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
> >>+++ b/package/qoriq-cadence-dp-firmware/qoriq-cadence-dp-firmware.mk
> >>@@ -13,33 +13,8 @@ QORIQ_CADENCE_DP_FIRMWARE_REDISTRIBUTE = NO
> >>  QORIQ_CADENCE_DP_FIRMWARE_INSTALL_IMAGES = YES
> >>  QORIQ_CADENCE_DP_FIRMWARE_INSTALL_TARGET = NO
> >>-# Helper for self-extracting binaries distributed by NXP.
> >>-#
> >>-# The --force option makes sure it doesn't fail if the source
> >>-# directory already exists. The --auto-accept skips the license check,
> >>-# as it is not needed in Buildroot because we have legal-info. Since
> >>-# there's a EULA in the binary file, we extract it in this macro, and
> >>-# it should therefore be added to the LICENSE_FILES variable of
> >>-# packages using this macro. Also, remember to set REDISTRIBUTE to
> >>-# "NO". Indeed, this is a legal minefield: the EULA specifies that the
> >>-# Board Support Package includes software and hardware (sic!) for
> >>-# which a separate license is needed...
> >>-#
> >>-# $(1): full path to the archive file
> >>-#
> >>-define QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_HELPER
> >>-	awk 'BEGIN      { start = 0; } \
> >>-	     /^EOEULA/  { start = 0; } \
> >>-	                { if (start) print; } \
> >>-	     /<<EOEULA/ { start = 1; }' \
> >>-	    $(1) > $(@D)/EULA
> >>-	cd $(@D) && sh $(1) --force --auto-accept
> >>-	find $(@D)/$(basename $(notdir $(1))) -mindepth 1 -maxdepth 1 -exec mv {} $(@D) \;
> >>-	rmdir $(@D)/$(basename $(notdir $(1)))
> >>-endef
> >>-
> >>  define QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_CMDS
> >>-	$(call QORIQ_CADENCE_DP_FIRMWARE_EXTRACT_HELPER,$(QORIQ_CADENCE_DP_FIRMWARE_DL_DIR)/$(QORIQ_CADENCE_DP_FIRMWARE_SOURCE))
> >>+	$(call FREESCALE_IMX_EXTRACT_HELPER,$(QORIQ_CADENCE_DP_FIRMWARE_DL_DIR)/$(QORIQ_CADENCE_DP_FIRMWARE_SOURCE))
> >>  endef
> >>  define QORIQ_CADENCE_DP_FIRMWARE_INSTALL_IMAGES_CMDS
> >>-- 
> >>2.34.1
> >>
> >>_______________________________________________
> >>buildroot mailing list
> >>buildroot@buildroot.org
> >>https://lists.buildroot.org/mailman/listinfo/buildroot
> >
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot