kbuild: Fix corner caches with .cache.mk

This two-patches fixes two corner cases with .cache.mk that have been reported. Neither problem was catastrophic, but certainly several people ran into the problem solved by the first patch (can't build after gcc upgrade) and wasted time debugging, so it's really a good idea to fix.

Sorry for the big delay between v2 and v3. I never quite caught up with email after the holidays, but hopefully better late than never...

Changes in v3:
- Fix as per Masahiro Yamada (move change to main Makefile)
- Use "uid 0" as the heuristic instead of install
- Do the checking in the main Makefile instead of Kbuild.include

Changes in v2:
- Don't error if MAKECMDGOALS is blank.

Douglas Anderson (2):
kbuild: Require a 'make clean' if we detect gcc changed underneath us
kbuild: Don't mess with the .cache.mk when root

 Makefile               | 15 +++++++++++++++
 scripts/Kbuild.include |  2 ++
 2 files changed, 17 insertions(+)

2.16.2.660.g709887971b-goog

Several people reported that the commit 3298b690b21c ("kbuild: Add a cache for generated variables") caused them problems when they updated gcc versions. Specifically the reports all looked something similar to this:

Masahiro Yamada determined that the problem was with:

  NOSTDINC_FLAGS += -nostdinc -isystem $(call shell-cached,$(CC)
  -print-file-name=include)

Specifically that the stale result of -print-file-name is stored in the cache file. It was determined that a "make clean" fixed the problems in all cases.

In this particular case we could certainly try to clean just the cache when we detect a gcc update, but it seems like overall it's a bad idea to do an incremental build when gcc changes. We should warn the user and tell them that they need a 'make clean'.

---

Changes in v3:
- Fix as per Masahiro Yamada (move change to main Makefile)

Changes in v2:
- Don't error if MAKECMDGOALS is blank.

 Makefile | 9 +++++++++
 1 file changed, 9 insertions(+)

index c4322dea3ca2..f1e61470640b 100644
@@ -573,6 +573,15 @@ virt-y		:= virt/
 endif # KBUILD_EXTMOD
 
 ifeq ($(dot-config),1)
+# Require a 'make clean' if the compiler changed; not only does the .cache.mk
+# need to be thrown out but we should also start with fresh object files.
+cc-fullversion-uncached := \
+	$(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-version.sh -p $(CC))
+
+ifneq ($(cc-fullversion-uncached),$(cc-fullversion))
+   $(error Detected new CC version ($(cc-fullversion-uncached) vs $(cc-fullversion)).  Please 'make clean')
+endif
+
 # Read in config
 -include include/config/auto.conf
 

2.16.2.660.g709887971b-goog

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 we're running as root. 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.

---

Changes in v3:
- Use "uid 0" as the heuristic instead of install
- Do the checking in the main Makefile instead of Kbuild.include

Changes in v2: None

 Makefile               | 6 ++++++
 scripts/Kbuild.include | 2 ++
 2 files changed, 8 insertions(+)

index f1e61470640b..c4d2131831ba 100644
@@ -268,6 +268,12 @@ __build_one_by_one:
 
 else
 
+# Don't create Makefile caches if running as root since they can't be deleted
+# easily; in the real world we might be root when doing "sudo make install"
+ifeq ($(shell id -u),0)
+export KBUILD_NOCACHE := 1
+endif
+
 # We need some generic definitions (do not try to remake the file).
 scripts/Kbuild.include: ;
 include scripts/Kbuild.include

index 065324a8046f..581f93f20119 100644
@@ -137,12 +137,14 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
 define __run-and-store
 ifeq ($(origin $(1)),undefined)
   $$(eval $(1) := $$(shell $$(2)))
+ifneq ($(KBUILD_NOCACHE),1)
 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.16.2.660.g709887971b-goog

Hi,

Ah, interesting. I hadn't realized that!

I'm OK with dropping this patch myself. It was mostly addressing a potential problem pointed out by Masahiro Yamada when we were talking about .cache.mk, but I don't think anyone has actually experienced the problems listed in the CL description. I suppose the other option would be to go back to keying off the "install" target (like v2 of this patch did), but it kinda feels like if someone did "sudo make install" and then followed up by a non-sudo "make" they probably would be able to figure out how to fix it without much trouble (just do a "sudo make clean").

In case it's not obvious, though, we should still try to land patch #1 of this series. That's a real problems that people reported when the .cache.mk stuff first landed and then folks tried to upgrade their gcc.

-Doug

deleted

install"

kernels as

testing.

I feel like I've definitely seen that permission error before.

Is there any issue if I:

$ make clean
$ make
$ sudo make install
<hack around>
$ make

Or it's only a problem if:

$ make clean
$ sudo make install
<hack around>
$ make
--
Thanks,
~Nick Desaulniers

Hi,

Hopefully the error message was obvious enough that it didn't take you too long to think to type "sudo make clean"? I know it's better for people not to have to figure this out, but my hope is at least it's not too arcane of an errror message.

I don't personally know of any problems with the above flow.

Yeah, just that one.

NOTE: as Masahiro noted in response to the cover letter [1], he's come up with a better solution to the problem that was solved by the .cache.mk and he's planning to remove it shortly.

[1] https://lkml.org/lkml/2018/3/13/100

This is my workflow, and what I suggest people do - only do "make install" as root, everything else as a normal user.

And in fact, it occasionally _has_ broken, when "make install" has done more than just install things, and created root-owned files and directories (particularly the generated headers).

And then I complain to people, because then things like "make clean" as a normal user ends up breaking too when there's some root-owned file.

So it can break, but it's pretty rare. Usually it's something like "people didn't use the proper sequence to update a file only if it changed, and just blindly over-wrote a new version.

Generally, I prefer "make install" not even checking dependencies at all, and just blindly copy things. And I'd definitely be ok with an error rather than root generating files, although then I'd not special-case mkcache, but all the *other* random file changes we do.

If Ingo wants to build as root, maybe we could even make him set some environment flag to avoid errors.

(But I don't actually like the patch in question, because I really think KBUILD_NOCACHE is much too specific a special case, and it should be way more generic).

Linus

Hi,

Please consider the patch abandoned. As per other threads pointed to in this conversation, Masahiro Yamada is planning to fully gut the ".cache.mk" from the build system anyway. :-)

-Doug

I only build as root infrequently, but I think PeterZ does it more frequently?

Distro package builds are also often done as root.

I don't mind warnings, etc. - I only objected to the workaround of silently turning off a build optimization when root.

Thanks,

Ingo

Yeah, a bunch of my testboxes don't even have a regular user account.

Hi Douglas,

2018-03-13 15:11 GMT+09:00 Douglas Anderson <[email protected]>:

If v3 had been sent in time for v4.15
I would have immediately queued it up.

But, the situation has changed.

The direction of the development is
to move the compiler flags evaluation to the Kconfig phase, and remove the Kbuild cache.

If you are interested in the background,
here are some of resources:

Linus' comment that finally moved this work forward:
https://lkml.org/lkml/2018/2/7/527

Tons of discussion about the initial version:
https://patchwork.kernel.org/patch/10207385/

The core patch in the latest version:
https://patchwork.kernel.org/patch/10225355/

And, I am removing the Kbuild cache entirely:
https://patchwork.kernel.org/patch/10225437/

I do not know how to handle your patches since I am removing the addressed code for 4.17-rc1, which is comming in a month or so.

If this is a fatal problem, we can queue it up
only for 4.16, but I do not hear problem reports these days...

Best Regards
Masahiro Yamada

Hi,

Sigh. My apologies again for sitting on this for so long. :(

Oh, that's great!

Just drop them. Ingo hated patch 2/2, and (like you) I haven't heard any complaints about the problems described by patch 1/2 in the last few months. I got back to this because I told myself I would try to re-focus on upstream stuff while I was at ELC, not because I suddenly got another stream of complaints. ;)

Anyway, glad to here you've come up with a better solution for the future and we can get rid of the cache. Thank you for all your hard work!

-Doug