Re: [PATCH v2 2/2] kbuild: Don't mess with the .cache.mk when installing

[Masahiro Yamada]

Hi Douglas,

2017-12-23 7:14 GMT+09:00 Douglas Anderson <dianders@xxxxxxxxxxxx>:
> As pointed out by Masahiro Yamada people often run "sudo make install"
> or "sudo make modules_install".  In theory, that could cause a cache
> file (or the directory containing it) to be created by root.  After
> doing this then subsequent invocations to make would yell with a whole
> bunch of warnings like:
>
>   /bin/sh: ./.cache.mk: Permission denied
>
> These yells would be mostly harmless (we'd just go on running without
> the cache), but they're ugly.
>
> The above situation would be fairly unlikely because it should only
> happen if someone does "sudo make install" before doing a normal
> "make", which is an invalid thing to do.  If you did a normal "make"
> before the "sudo make install" then all the cache files and
> directories should have already been created by a normal user and,
> even if the superuser needed to add to the existing files, we wouldn't
> end up with a permission problem.
>
> The above situation would also not have been catastrophic because
> presumably if the user was able to run "sudo make install" then they
> should also be able to run "sudo make clean" to clean things up.
>
> However, even though the problem described is relatively minor, it
> probably makes sense to add a heuristic to avoid creating cache files
> when one of the make goals looks like an install.  This heuristic
> doesn't add a ton of overhead and it might save someone time tracking
> down strange "Permission denied" messages.  We'll consider this
> heuristic good enough because the problem really shouldn't be that
> serious.
>
> Fixes: 3298b690b21c ("kbuild: Add a cache for generated variables")
> Suggested-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
>
> Changes in v2: None
>
>  scripts/Kbuild.include | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index f7efb59d85d1..314585b3efe4 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -124,6 +124,12 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
>  # previous invocation of make!) we'll return the value.  If not, we'll
>  # compute it and store the result for future runs.
>  #
> +# NOTE: we won't ever add to the cache if one of the make goals looks like
> +# it is installing.  Technically there's no reason we can't cache things
> +# related to install but it's likely that 'make install' is called as root.
> +# If we create the cache file as root we might have a hard time adding to it
> +# (or deleting it in make clean).
> +#
>  # This is a bit of voodoo, but basic explanation is that if the variable
>  # was undefined then we'll evaluate the shell command and store the result
>  # into the variable.  We'll then store that value in the cache and finally
> @@ -137,12 +143,14 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
>  define __run-and-store
>  ifeq ($(origin $(1)),undefined)
>    $$(eval $(1) := $$(shell $$(2)))
> +ifeq ($(filter %install,$(MAKECMDGOALS)),)

Hmm, this may not always disable caching when installing something.

The check for %install is only effective at the top-level Makefile.
It may call sub-makefile such as scripts/Makefile.modinst,
scripts/Makefile.dtbinst,
etc. with empty MAKECMDGOALS.


Maybe, introduce a new flag, for example, KBUILD_NOCACHE?

I found KBUILD_NOCMDDEP flag in the same file,
so adding this flag should not be so odd.

(Please feel free to rename it if you have an idea for better naming)



In top Makefile, before including Kbuild.include,
then

ifeq ($(shell id -u),0)
export KBUILD_NOCACHE := 1
endif


I considered this a bit, then I thought
checking user id would have more sense...





>  ifeq ($(create-cache-dir),1)
>    $$(shell mkdir -p $(dir $(make-cache)))
>    $$(eval create-cache-dir :=)
>  endif
>    $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
>  endif
> +endif
>  endef
>  __shell-cached = $(eval $(call __run-and-store,$(1)))$($(1))
>  shell-cached = $(call __shell-cached,__cached_$(call __sanitize-opt,$(1)),$(1))
> --
> 2.15.1.620.gb9897f4670-goog
>



-- 
Best Regards
Masahiro Yamada