All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Yann E. MORIN" <yann.morin.1998@free.fr>
To: James Hilliard <james.hilliard1@gmail.com>
Cc: "Anders F Björklund" <anders.f.bjorklund@gmail.com>,
	"Andrey Smirnov" <andrew.smirnov@gmail.com>,
	"Samuel Martin" <s.martin49@gmail.com>,
	"Dominik Michael Rauh" <dmrauh@posteo.de>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	buildroot@buildroot.org,
	"Fabrice Fontaine" <fontaine.fabrice@gmail.com>
Subject: Re: [Buildroot] [PATCH 4/4] package/ccache: bump to version 4.7.4
Date: Sun, 8 Jan 2023 23:29:26 +0100	[thread overview]
Message-ID: <20230108222926.GJ151997@scaer> (raw)
In-Reply-To: <20230107010437.2471513-4-james.hilliard1@gmail.com>

James, All,

+Dominik, +Anders

On 2023-01-06 18:04 -0700, James Hilliard spake thusly:
> Migrate to cmake package infrastructure.
> 
> Add new host-hiredis host-zstd dependencies.
> 
> Add new ccache dependency exclusions to pkg-generic.
> 
> Migrate HOST_CCACHE_PATCH_CONFIGURATION to handle updated
> source files/format.
> 
> License hashes changed due to migrating urls to https:
> https://github.com/ccache/ccache/commit/a0f32f161f1b8b9c5d287ca8abe88e3fd1e940a2
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  package/ccache/ccache.hash |  6 +++---
>  package/ccache/ccache.mk   | 30 +++++++++++-------------------

This misses a critical part: what prevents cmake from using ccache to
build?

Dominik and Anders got that correct in their patch:

    +# we are built before ccache
    +HOST_CMAKE_CONFIGURE_OPTS = \
    +	$(HOST_CONFIGURE_OPTS) \
    +	CC="$(HOSTCC_NOCCACHE)" \
    +	GCC="$(HOSTCC_NOCCACHE)" \
    +	CXX="$(HOSTCXX_NOCCACHE)"
    +
     define HOST_CMAKE_CONFIGURE_CMDS
     	(cd $(@D); \
    -		$(HOST_CONFIGURE_OPTS) \
    +		$(HOST_CMAKE_CONFIGURE_OPTS) \
     		CFLAGS="$(HOST_CMAKE_CFLAGS)" \

(Arguably, CFLAGS assignment should have been moved into
HOST_CMAKE_CONFIGURE_OPTS as well.)

[--SNIP--]
> diff --git a/package/ccache/ccache.mk b/package/ccache/ccache.mk
> index 97aa8b1c75..0c208cb9e6 100644
> --- a/package/ccache/ccache.mk
> +++ b/package/ccache/ccache.mk
> @@ -4,27 +4,19 @@
>  #
>  ################################################################################
>  
> -CCACHE_VERSION = 3.7.12
> +CCACHE_VERSION = 4.7.4
>  CCACHE_SITE = https://github.com/ccache/ccache/releases/download/v$(CCACHE_VERSION)
>  CCACHE_SOURCE = ccache-$(CCACHE_VERSION).tar.xz
>  CCACHE_LICENSE = GPL-3.0+, others
>  CCACHE_LICENSE_FILES = LICENSE.adoc GPL-3.0.txt
> -
> -# Force ccache to use its internal zlib. The problem is that without
> -# this, ccache would link against the zlib of the build system, but we
> -# might build and install a different version of zlib in $(O)/host
> -# afterwards, which ccache will pick up. This might break if there is
> -# a version mismatch. A solution would be to add host-zlib has a
> -# dependency of ccache, but it would require tuning the zlib .mk file
> -# to use HOSTCC_NOCCACHE as the compiler. Instead, we take the easy
> -# path: tell ccache to use its internal copy of zlib, so that ccache
> -# has zero dependency besides the C library.
> -HOST_CCACHE_CONF_OPTS += --with-bundled-zlib
> +HOST_CCACHE_DEPENDENCIES = host-hiredis host-zstd

Dominik and Anders made hiredis an optional dependency. Given that
hiredis builds very, very fast (just about 3s here), I wonder if that
really makes sense to make it optional; hiredis has no dependency that
are not dependencies of ccache, so there is no hidden overhead.

If hiredis support is compiled in, does that make it mandatory to use a
hiredis server, or does ccache still uses local files by default?

If the latter, then I think a mandatory dependency is OK.

Dominik, Anders, did you make it a optional because of a previous review
by Thomas asking so?

[--SNIP--]
> +HOST_CCACHE_CONF_OPTS += \
> +	-DCMAKE_C_COMPILER_LAUNCHER="" \
> +	-DCMAKE_CXX_COMPILER_LAUNCHER="" \

Same as for hiredis: use -U.

[--SNIP--]
> @@ -35,11 +27,11 @@ HOST_CCACHE_CONF_ENV = \
>  #    the need to specify BR_CACHE_DIR when invoking ccache directly.
>  #    CCache replaces "%s" with the home directory of the current user,
>  #    So rewrite BR_CACHE_DIR to take that into consideration for SDK purpose
> -HOST_CCACHE_DEFAULT_CCACHE_DIR = $(patsubst $(HOME)/%,\%s/%,$(BR_CACHE_DIR))
> +HOST_CCACHE_DEFAULT_CCACHE_DIR = $(patsubst $(HOME)/%,%,$(BR_CACHE_DIR))

Err, this is probably bad and the comment above no longer matches the
code.

Did you meant using 'home_dir +' to replace '%s' (see Dominik and
Anders' patch):

    HOST_CCACHE_DEFAULT_CCACHE_DIR = $(patsubst \"$(HOME)/%,home_dir + \"/%,\"$(BR_CACHE_DIR)\")

>  define HOST_CCACHE_PATCH_CONFIGURATION
> -	sed -i 's,getenv("CCACHE_DIR"),getenv("BR_CACHE_DIR"),' $(@D)/src/ccache.c
> -	sed -i 's,"%s/.ccache","$(HOST_CCACHE_DEFAULT_CCACHE_DIR)",' $(@D)/src/conf.c
> +	sed -i 's,getenv("CCACHE_DIR"),getenv("BR_CACHE_DIR"),' $(@D)/src/Config.cpp
> +	sed -i 's,".ccache","$(HOST_CCACHE_DEFAULT_CCACHE_DIR)",' $(@D)/src/Config.cpp

This needs a bit of consolidation and further explanations, becasue
ccache now has more locations where it looks for, and we do not want it
to default to those:

    home_dir + "/.ccache"           -> the legacy location
    home_dir + "/.cache/ccache"     -> new, XDG-based
    home_dir + "/.config/ccache"    -> new, XDG-based

Also, it seems that we need a patch to use BR_CACHE_DIR from the
environment...

[--SNIP--]
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index f24e03a325..0863444221 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -792,7 +792,7 @@ $(2)_EXTRACT_DEPENDENCIES += \
>  endif
>  
>  ifeq ($$(BR2_CCACHE),y)
> -ifeq ($$(filter host-tar host-skeleton host-xz host-lzip host-fakedate host-ccache,$(1)),)
> +ifeq ($$(filter host-tar host-skeleton host-xz host-lzip host-fakedate host-ccache host-cmake host-hiredis host-pkgconf host-zstd,$(1)),)

I've applied that part with just host-hiredis, host-pkgconf, and host-zstd,
and left host-cmake out for now.

Regards,
Yann E. MORIN.

>  $(2)_DEPENDENCIES += host-ccache
>  endif
>  endif
> -- 
> 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

  reply	other threads:[~2023-01-08 22:29 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-07  1:04 [Buildroot] [PATCH 1/4] package/hiredis: enable host package James Hilliard
2023-01-07  1:04 ` [Buildroot] [PATCH 2/4] package/zstd: disable ccache for " James Hilliard
2023-01-08 21:44   ` Yann E. MORIN
2023-01-07  1:04 ` [Buildroot] [PATCH 3/4] package/pkgconf: " James Hilliard
2023-01-08 21:58   ` Yann E. MORIN
2023-01-07  1:04 ` [Buildroot] [PATCH 4/4] package/ccache: bump to version 4.7.4 James Hilliard
2023-01-08 22:29   ` Yann E. MORIN [this message]
2023-01-09  0:22     ` James Hilliard
2023-01-09 19:21     ` Dominik Michael Rauh
2023-01-09 19:55       ` James Hilliard
2023-01-08 21:15 ` [Buildroot] [PATCH 1/4] package/hiredis: enable host package Yann E. MORIN

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230108222926.GJ151997@scaer \
    --to=yann.morin.1998@free.fr \
    --cc=anders.f.bjorklund@gmail.com \
    --cc=andrew.smirnov@gmail.com \
    --cc=buildroot@buildroot.org \
    --cc=dmrauh@posteo.de \
    --cc=fontaine.fabrice@gmail.com \
    --cc=james.hilliard1@gmail.com \
    --cc=s.martin49@gmail.com \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.