Hi,
On Thu, Nov 9, 2017 at 7:41 AM, Masahiro Yamada
<yamada.masahiro@xxxxxxxxxxxxx> wrote:
> Currently, the existence of $(dir $(make-cache)) is always checked,
> and created if it is missing.
>
> We can avoid unnecessary system calls by some tricks.
>
> [1] If KBUILD_SRC is unset, we are building in the source tree.
> The output directory checks can be entirely skipped.
> [2] If at least one cache data is found, it means the cache file
> was included. Obiously its directory exists. Skip "mkdir -p".
> [3] If Makefile does not contain any call of __run-and-store, it will
> not create a cache file. No need to create its directory.
> [4] The "mkdir -p" should be only invoked by the first call of
> __run-and-store
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> ---
>
> scripts/Kbuild.include | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index be1c9d6..4fb1be1 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -99,18 +99,19 @@ cc-cross-prefix = \
>
> # Include values from last time
> make-cache := $(if $(KBUILD_EXTMOD),$(KBUILD_EXTMOD)/,$(if $(obj),$(obj)/)).cache.mk
> -ifeq ($(wildcard $(dir $(make-cache))),)
> -$(shell mkdir -p '$(dir $(make-cache))')
> -endif
> $(make-cache): ;
> -include $(make-cache)
>
> +cached-data := $(filter __cached_%, $(.VARIABLES))
> +
> # If cache exceeds 1000 lines, shrink it down to 500.
> -ifneq ($(word 1000,$(filter __cached_%, $(.VARIABLES))),)
> +ifneq ($(word 1000,$(cached-data)),)
> $(shell tail -n 500 $(make-cache) > $(make-cache).tmp; \
> mv $(make-cache).tmp $(make-cache))
> endif
>
> +cache-dir := $(if $(KBUILD_SRC),$(if $(cache-data),,$(dir $(make-cache))))
It wouldn't hurt to add a comment that cache-dir will be blank if we
don't need to make the cache dir and will contain a directory path
only if the dir doesn't exist. Without a comment it could take
someone quite a while to realize that...
> +
> # Usage: $(call __sanitize-opt,Hello=Hola$(comma)Goodbye Adios)
> #
> # Convert all '$', ')', '(', '\', '=', ' ', ',', ':' to '_'
> @@ -136,6 +137,10 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
> define __run-and-store
> ifeq ($(origin $(1)),undefined)
> $$(eval $(1) := $$(shell $$(2)))
> +ifneq ($(cache-dir),)
> + $$(shell mkdir -p $(cache-dir))
I _think_ you want some single quotes in there. AKA:
$$(shell mkdir -p '$(cache-dir)')
That at least matches what the "old" code used to do. Specifically if
'cache-dir' happens to have a space in it then it won't work right
without the single quotes. There may be other symbols that your shell
might interpret in interesting ways, too.
NOTE: I have no idea if the kernel Makefiles work if paths like
KBUILD_SRC have spaces in them to begin with, but it seems wise to add
the quotes here anyway.
ALSO NOTE: I think you could still confuse the kernel Makefiles if
somehow you had a single quote in your path somehow. I assume we
don't care?
> + $$(eval cache-dir :=)
> +endif
> $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
> endif
> endef
Other than the single quote problem and the suggested comment, this
seems like a sane optimization to me. Feel free to add my Reviewed-by
once those fixes are in place.
-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
