Re: [Buildroot] [PATCH 1/4] package/hiredis: enable host package

["Yann E. MORIN" <yann.morin.1998@free.fr>]

James, All,

There's this already pending, old-ish patch from Dominik and
Anders:
    https://patchwork.ozlabs.org/project/buildroot/patch/20220807093853.15579-1-dmrauh@posteo.de/

that tries to bump ccache.

I prefer your series, because everything is split as much as possible,
but I am looking at both patches to see if we can pick the best of the
two. And indeed, see below...

On 2023-01-06 18:04 -0700, James Hilliard spake thusly:
> The host-hiredis package is needed for the upcoming ccache update.
> 
> Note that we must disable ccache when building host-hiredis itself
> as host-ccache depends on host-hiredis.
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  package/hiredis/hiredis.mk | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/package/hiredis/hiredis.mk b/package/hiredis/hiredis.mk
> index f79b6c757a..f2a4836310 100644
> --- a/package/hiredis/hiredis.mk
> +++ b/package/hiredis/hiredis.mk
> @@ -22,4 +22,13 @@ else
>  HIREDIS_CONF_OPTS += -DENABLE_SSL=OFF
>  endif
>  
> +# We are a ccache dependency, so we can't use ccache
> +HOST_HIREDIS_CONF_OPTS += \

This is the first assignment, so no need to use append-assignment, jsut
use a simple assignemnt.

> +	-DCMAKE_C_COMPILER_LAUNCHER="" \
> +	-DCMAKE_CXX_COMPILER_LAUNCHER="" \

The patch from Dominik and Anders uses -U to "unset" the variables,
while you set them to empty. I think unsetting is semantically more
correct, and I checked: it works.

Also, the following options are not related to being build before
ccache, but the comment makes it confusing, so I prefer we split the
assignment when only parts of it is covered by a coment:

    HOST_HIREDIS_CONF_OPTS = \
        -DDISABLE_TESTS=ON \
        -DENABLE_SSL=OFF

    # Set CMAKE_BUILD_TYPE to Release or the libraries will be suffixed with "d"
    HOST_HIREDIS_CONF_OPTS += -DCMAKE_BUILD_TYPE=Release

    # We are a ccache dependency, so we can't use ccache; reset the
    # options set by the cmake infra
    HOST_HIREDIS_CONF_OPTS += \
        -UCMAKE_C_COMPILER_LAUNCHER \
        -UCMAKE_CXX_COMPILER_LAUNCHER

However, I wonder whether the -DCMAKE_BUILD_TYPE=Release makes sense.
Ideed, we need to set it for the target variant, because we can do a
debug build or not, and we set CMAKE_BUILD_TYPE in toolchainfile.cmake.
But for the host variant, we do not use toolchainfile.cmake, and we
never explicitly pass the build type on the command line either, so we
especially never set it to "Debug", and so the 'd' suffix should not be
added.

Ah, but CMAKE_BUILD_TYPE can come from the environment... Damn.

OK, so I did a few changes, and applied to master, thanks:

  - add Dominik and Anders in Cc to ack they provided inspiration
  - use -Ufoo instead of -Dfoo=""  (as per Dominik & Anders)
  - reorder CONF_OPTS assignments

Regards,
Yann E. MORIN.

> +	-DCMAKE_BUILD_TYPE=Release \
> +	-DDISABLE_TESTS=ON \
> +	-DENABLE_SSL=OFF
> +
>  $(eval $(cmake-package))
> +$(eval $(host-cmake-package))
> -- 
> 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