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

[Peter Zijlstra]

On Wed, Mar 14, 2018 at 08:23:16AM +0100, Ingo Molnar wrote:
> 
> * Linus Torvalds  wrote:
> 
> > If Ingo wants to build as root, maybe we could even make him set some
> > environment flag to avoid errors.
> 
> I only build as root infrequently, but I think PeterZ does it more frequently?

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

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

[Peter Zijlstra]

On Wed, Mar 14, 2018 at 08:23:16AM +0100, Ingo Molnar wrote:
> 
> * Linus Torvalds  wrote:
> 
> > If Ingo wants to build as root, maybe we could even make him set some
> > environment flag to avoid errors.
> 
> I only build as root infrequently, but I think PeterZ does it more frequently?

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

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

[Ingo Molnar]

* Linus Torvalds  wrote:

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

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

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

[Ingo Molnar]

* Linus Torvalds  wrote:

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

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

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

[Doug Anderson]

Hi,

On Tue, Mar 13, 2018 at 10:39 AM, Linus Torvalds
 wrote:
> On Tue, Mar 13, 2018 at 9:44 AM, Doug Anderson  wrote:
>>> Is there any issue if I:
>>>
>>> $ make clean
>>> $ make
>>> $ sudo make install
>>> 
>>> $ make
>>
>> I don't personally know of any problems with the above flow.
>
> 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).

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

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

[Doug Anderson]

Hi,

On Tue, Mar 13, 2018 at 10:39 AM, Linus Torvalds
 wrote:
> On Tue, Mar 13, 2018 at 9:44 AM, Doug Anderson  wrote:
>>> Is there any issue if I:
>>>
>>> $ make clean
>>> $ make
>>> $ sudo make install
>>> 
>>> $ make
>>
>> I don't personally know of any problems with the above flow.
>
> 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).

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

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

[Linus Torvalds]

On Tue, Mar 13, 2018 at 9:44 AM, Doug Anderson  wrote:
>> Is there any issue if I:
>>
>> $ make clean
>> $ make
>> $ sudo make install
>> 
>> $ make
>
> I don't personally know of any problems with the above flow.

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

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

[Linus Torvalds]

On Tue, Mar 13, 2018 at 9:44 AM, Doug Anderson  wrote:
>> Is there any issue if I:
>>
>> $ make clean
>> $ make
>> $ sudo make install
>> 
>> $ make
>
> I don't personally know of any problems with the above flow.

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

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

[Doug Anderson]

Hi,

On Tue, Mar 13, 2018 at 9:33 AM, Nick Desaulniers
 wrote:
> On Mon, Mar 12, 2018 at 11:23 PM Doug Anderson 
> wrote:
>
>> Hi,
>
>> On Mon, Mar 12, 2018 at 11:16 PM, Ingo Molnar  wrote:
>> >
>> > * Douglas Anderson  wrote:
>> >
>> >> +# 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
>> >
>> > Please don't do this - many prominent kernel developers build their
> kernels as
>> > root - this makes the build slower for them, and also bifurcates
> testing.
>
>> 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.
>
>>/bin/sh: ./.cache.mk: Permission denied
>
> I feel like I've definitely seen that permission error before.

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.


> Is there any issue if I:
>
> $ make clean
> $ make
> $ sudo make install
> 
> $ make

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


> Or it's only a problem if:
>
> $ make clean
> $ sudo make install
> 
> $ make

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

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

[Doug Anderson]

Hi,

On Tue, Mar 13, 2018 at 9:33 AM, Nick Desaulniers
 wrote:
> On Mon, Mar 12, 2018 at 11:23 PM Doug Anderson 
> wrote:
>
>> Hi,
>
>> On Mon, Mar 12, 2018 at 11:16 PM, Ingo Molnar  wrote:
>> >
>> > * Douglas Anderson  wrote:
>> >
>> >> +# 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
>> >
>> > Please don't do this - many prominent kernel developers build their
> kernels as
>> > root - this makes the build slower for them, and also bifurcates
> testing.
>
>> 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.
>
>>/bin/sh: ./.cache.mk: Permission denied
>
> I feel like I've definitely seen that permission error before.

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.


> Is there any issue if I:
>
> $ make clean
> $ make
> $ sudo make install
> 
> $ make

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


> Or it's only a problem if:
>
> $ make clean
> $ sudo make install
> 
> $ make

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

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

[Nick Desaulniers]

On Mon, Mar 12, 2018 at 11:23 PM Doug Anderson 
wrote:

> Hi,

> On Mon, Mar 12, 2018 at 11:16 PM, Ingo Molnar  wrote:
> >
> > * Douglas Anderson  wrote:
> >
> >> +# 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
> >
> > Please don't do this - many prominent kernel developers build their
kernels as
> > root - this makes the build slower for them, and also bifurcates
testing.

> 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.

>/bin/sh: ./.cache.mk: Permission denied

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

Is there any issue if I:

$ make clean
$ make
$ sudo make install

$ make

Or it's only a problem if:

$ make clean
$ sudo make install

$ make
--
Thanks,
~Nick Desaulniers

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

[Nick Desaulniers]

On Mon, Mar 12, 2018 at 11:23 PM Doug Anderson 
wrote:

> Hi,

> On Mon, Mar 12, 2018 at 11:16 PM, Ingo Molnar  wrote:
> >
> > * Douglas Anderson  wrote:
> >
> >> +# 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
> >
> > Please don't do this - many prominent kernel developers build their
kernels as
> > root - this makes the build slower for them, and also bifurcates
testing.

> 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.

>/bin/sh: ./.cache.mk: Permission denied

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

Is there any issue if I:

$ make clean
$ make
$ sudo make install

$ make

Or it's only a problem if:

$ make clean
$ sudo make install

$ make
--
Thanks,
~Nick Desaulniers

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

[Doug Anderson]

Hi,

On Mon, Mar 12, 2018 at 11:16 PM, Ingo Molnar  wrote:
>
> * Douglas Anderson  wrote:
>
>> +# 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
>
> Please don't do this - many prominent kernel developers build their kernels as
> root - this makes the build slower for them, and also bifurcates testing.

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

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

[Doug Anderson]

Hi,

On Mon, Mar 12, 2018 at 11:16 PM, Ingo Molnar  wrote:
>
> * Douglas Anderson  wrote:
>
>> +# 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
>
> Please don't do this - many prominent kernel developers build their kernels as
> root - this makes the build slower for them, and also bifurcates testing.

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

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

[Ingo Molnar]

* Douglas Anderson  wrote:

> +# 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

Please don't do this - many prominent kernel developers build their kernels as 
root - this makes the build slower for them, and also bifurcates testing.

Thanks,

Ingo

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

[Ingo Molnar]

* Douglas Anderson  wrote:

> +# 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

Please don't do this - many prominent kernel developers build their kernels as 
root - this makes the build slower for them, and also bifurcates testing.

Thanks,

Ingo

[PATCH v3 2/2] kbuild: Don't mess with the .cache.mk when root

[Douglas Anderson]

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.

Fixes: 3298b690b21c ("kbuild: Add a cache for generated variables")
Suggested-by: Masahiro Yamada 
Signed-off-by: Douglas Anderson 
---

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(+)

diff --git a/Makefile b/Makefile
index f1e61470640b..c4d2131831ba 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 065324a8046f..581f93f20119 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -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

[PATCH v3 2/2] kbuild: Don't mess with the .cache.mk when root

[Douglas Anderson]

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.

Fixes: 3298b690b21c ("kbuild: Add a cache for generated variables")
Suggested-by: Masahiro Yamada 
Signed-off-by: Douglas Anderson 
---

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(+)

diff --git a/Makefile b/Makefile
index f1e61470640b..c4d2131831ba 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 065324a8046f..581f93f20119 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -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