Re: [RFC PATCH 1/4] common/image.c: image_decomp: put IH_COMP_XXX cases inside ifndef USE_HOSTCC
On Wed, Jun 17, 2020 at 09:07:20AM +0200, Rasmus Villemoes wrote: > On 17/06/2020 05.11, Simon Glass wrote: > > Hi Rasmus, > > > > On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes > > wrote: > >> > >> When building host tools, the CONFIG_GZIP etc. symbols are not defined > >> anyway, so this does not (should not) change anything [1]. However, > >> since the host tools also don't include linux/kconfig.h, one cannot > >> use the CONFIG_IS_ENABLED() smartness in a preprocessor conditional, > >> which in turn prevents one from adding, say, an > >> > >> #if CONFIG_IS_ENABLED(ZSTD) > >> > >> case. > >> > >> OTOH, with this, one can do that, and it also makes it possible to > >> convert say, "#ifdef CONFIG_GZIP" to "#if CONFIG_IS_ENABLED(GZIP)" to > >> make those other cases SPL-or-not-SPL-aware. > >> > >> [1] The gzip.h header is only included under !USE_HOSTCC, and removing > >> the CONFIG_GZIP conditional hence both gives an "no declaration of > >> gunzip" warning as well as breaks the link of the host tools, since > >> the gunzip code is indeed not linked in. > >> > >> Signed-off-by: Rasmus Villemoes > >> --- > >> common/image.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/common/image.c b/common/image.c > >> index e1ca1a7905..73f6845274 100644 > >> --- a/common/image.c > >> +++ b/common/image.c > >> @@ -458,6 +458,7 @@ int image_decomp(int comp, ulong load, ulong > >> image_start, int type, > >> else > >> ret = -ENOSPC; > >> break; > >> +#ifndef USE_HOSTCC > > > > In general I'm not a fan of the USE_HOSTCC stuff. I suspect HOST > > should be a Kconfig like anything else, although perhaps magically > > inserted somehow. Then we can do more things at compile-time. > > Yes, as you can see in 2/4, I agree that the hostcc stuff is messy. I > imagine we can introduce a Kconfig.hosttools at some point which can > have a lot of def_bool y symbols (and of course one could eventually > allow them to be actually choosable, e.g. to allow the user to not build > some tool or functionality that he doesn't use and which requires some > external library dependency). > > So for example, to make the fit_check_sign actually also check the > decompression, we'd add a > > CONFIG_HOSTTOOL_GZIP > def_bool y > > move this USE_HOSTCC a bit further down, and change the #ifdef > CONFIG_GZIP to #if CONFIG_IS_ENABLED(GZIP) - all after checking all the > other places CONFIG_GZIP is tested... so not something that's just done > mechanically or over night, but I think it can be done mostly piecemeal. I agree, we can improve this area piecemeal. We need to perhaps give a harder think to what code should be shared directly and where it's more of a pain than it's worth too. -- Tom signature.asc Description: PGP signature
Re: [RFC PATCH 1/4] common/image.c: image_decomp: put IH_COMP_XXX cases inside ifndef USE_HOSTCC
On 17/06/2020 05.11, Simon Glass wrote: > Hi Rasmus, > > On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes > wrote: >> >> When building host tools, the CONFIG_GZIP etc. symbols are not defined >> anyway, so this does not (should not) change anything [1]. However, >> since the host tools also don't include linux/kconfig.h, one cannot >> use the CONFIG_IS_ENABLED() smartness in a preprocessor conditional, >> which in turn prevents one from adding, say, an >> >> #if CONFIG_IS_ENABLED(ZSTD) >> >> case. >> >> OTOH, with this, one can do that, and it also makes it possible to >> convert say, "#ifdef CONFIG_GZIP" to "#if CONFIG_IS_ENABLED(GZIP)" to >> make those other cases SPL-or-not-SPL-aware. >> >> [1] The gzip.h header is only included under !USE_HOSTCC, and removing >> the CONFIG_GZIP conditional hence both gives an "no declaration of >> gunzip" warning as well as breaks the link of the host tools, since >> the gunzip code is indeed not linked in. >> >> Signed-off-by: Rasmus Villemoes >> --- >> common/image.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/common/image.c b/common/image.c >> index e1ca1a7905..73f6845274 100644 >> --- a/common/image.c >> +++ b/common/image.c >> @@ -458,6 +458,7 @@ int image_decomp(int comp, ulong load, ulong >> image_start, int type, >> else >> ret = -ENOSPC; >> break; >> +#ifndef USE_HOSTCC > > In general I'm not a fan of the USE_HOSTCC stuff. I suspect HOST > should be a Kconfig like anything else, although perhaps magically > inserted somehow. Then we can do more things at compile-time. Yes, as you can see in 2/4, I agree that the hostcc stuff is messy. I imagine we can introduce a Kconfig.hosttools at some point which can have a lot of def_bool y symbols (and of course one could eventually allow them to be actually choosable, e.g. to allow the user to not build some tool or functionality that he doesn't use and which requires some external library dependency). So for example, to make the fit_check_sign actually also check the decompression, we'd add a CONFIG_HOSTTOOL_GZIP def_bool y move this USE_HOSTCC a bit further down, and change the #ifdef CONFIG_GZIP to #if CONFIG_IS_ENABLED(GZIP) - all after checking all the other places CONFIG_GZIP is tested... so not something that's just done mechanically or over night, but I think it can be done mostly piecemeal. Rasmus
Re: [RFC PATCH 1/4] common/image.c: image_decomp: put IH_COMP_XXX cases inside ifndef USE_HOSTCC
Hi Rasmus,
On Fri, 12 Jun 2020 at 05:02, Rasmus Villemoes
wrote:
>
> When building host tools, the CONFIG_GZIP etc. symbols are not defined
> anyway, so this does not (should not) change anything [1]. However,
> since the host tools also don't include linux/kconfig.h, one cannot
> use the CONFIG_IS_ENABLED() smartness in a preprocessor conditional,
> which in turn prevents one from adding, say, an
>
> #if CONFIG_IS_ENABLED(ZSTD)
>
> case.
>
> OTOH, with this, one can do that, and it also makes it possible to
> convert say, "#ifdef CONFIG_GZIP" to "#if CONFIG_IS_ENABLED(GZIP)" to
> make those other cases SPL-or-not-SPL-aware.
>
> [1] The gzip.h header is only included under !USE_HOSTCC, and removing
> the CONFIG_GZIP conditional hence both gives an "no declaration of
> gunzip" warning as well as breaks the link of the host tools, since
> the gunzip code is indeed not linked in.
>
> Signed-off-by: Rasmus Villemoes
> ---
> common/image.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/common/image.c b/common/image.c
> index e1ca1a7905..73f6845274 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -458,6 +458,7 @@ int image_decomp(int comp, ulong load, ulong image_start,
> int type,
> else
> ret = -ENOSPC;
> break;
> +#ifndef USE_HOSTCC
In general I'm not a fan of the USE_HOSTCC stuff. I suspect HOST
should be a Kconfig like anything else, although perhaps magically
inserted somehow. Then we can do more things at compile-time.
> #ifdef CONFIG_GZIP
> case IH_COMP_GZIP: {
> ret = gunzip(load_buf, unc_len, image_buf, _len);
> @@ -508,6 +509,7 @@ int image_decomp(int comp, ulong load, ulong image_start,
> int type,
> break;
> }
> #endif /* CONFIG_LZ4 */
> +#endif /* !USE_HOSTCC */
> default:
> printf("Unimplemented compression type %d\n", comp);
> return -ENOSYS;
> --
> 2.23.0
>
Regards,
Simon
