2023-12-14 14:03:35

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH] kbuild: resolve symlinks for O= properly

Currently, Kbuild follows the logical chain of directories for the O=
option, just like 'cd' (or 'realpath --logical') does.

Example:

$ mkdir -p /tmp/a /tmp/x/y
$ ln -s /tmp/x/y /tmp/a/b
$ realpath /tmp/a/b/..
/tmp/x
$ realpath --logical /tmp/a/b/..
/tmp/a
$ make O=/tmp/a/b/.. defconfig
make[1]: Entering directory '/tmp/a'
[snip]
make[1]: Leaving directory '/tmp/a'

'make O=/tmp/a/b/.. defconfig' creates the kernel configuration in
/tmp/a instead of /tmp/x despite the directory path /tmp/a/b/..
resolves to the physical directory path /tmp/x.

This is because Kbuild internally uses the 'cd ... && pwd' for the
path resolution, but this behavior is not predictable for users.
Additionally, it is not consistent with how the Kbuild handles the
M= option or GNU Make works with 'make -C /tmp/a/b/..'.

Using the physical directory structure for the O= option seems more
reasonable.

The comment says "expand a shell special character '~'", but it has
already been expanded to the home directory in the command line.

Signed-off-by: Masahiro Yamada <[email protected]>
---

Makefile | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index 24fac1889997..a05f0f7c99e0 100644
--- a/Makefile
+++ b/Makefile
@@ -190,14 +190,11 @@ ifeq ("$(origin O)", "command line")
endif

ifneq ($(KBUILD_OUTPUT),)
-# Make's built-in functions such as $(abspath ...), $(realpath ...) cannot
-# expand a shell special character '~'. We use a somewhat tedious way here.
-abs_objtree := $(shell mkdir -p $(KBUILD_OUTPUT) && cd $(KBUILD_OUTPUT) && pwd)
-$(if $(abs_objtree),, \
- $(error failed to create output directory "$(KBUILD_OUTPUT)"))
-
+# $(realpath ...) gets empty if the path does not exist. Run 'mkdir -p' first.
+$(shell mkdir -p $(KBUILD_OUTPUT))
# $(realpath ...) resolves symlinks
-abs_objtree := $(realpath $(abs_objtree))
+abs_objtree := $(realpath $(KBUILD_OUTPUT))
+$(if $(abs_objtree),,$(error failed to create output directory "$(KBUILD_OUTPUT)"))
endif # ifneq ($(KBUILD_OUTPUT),)

ifneq ($(words $(subst :, ,$(abs_srctree))), 1)
--
2.40.1


2023-12-15 09:37:29

by Nicolas Schier

[permalink] [raw]
Subject: Re: [PATCH] kbuild: resolve symlinks for O= properly

On Thu, Dec 14, 2023 at 11:03:05PM +0900, Masahiro Yamada wrote:
> Currently, Kbuild follows the logical chain of directories for the O=
> option, just like 'cd' (or 'realpath --logical') does.
>
> Example:
>
> $ mkdir -p /tmp/a /tmp/x/y
> $ ln -s /tmp/x/y /tmp/a/b
> $ realpath /tmp/a/b/..
> /tmp/x
> $ realpath --logical /tmp/a/b/..
> /tmp/a
> $ make O=/tmp/a/b/.. defconfig
> make[1]: Entering directory '/tmp/a'
> [snip]
> make[1]: Leaving directory '/tmp/a'
>
> 'make O=/tmp/a/b/.. defconfig' creates the kernel configuration in
> /tmp/a instead of /tmp/x despite the directory path /tmp/a/b/..
> resolves to the physical directory path /tmp/x.
>
> This is because Kbuild internally uses the 'cd ... && pwd' for the
> path resolution, but this behavior is not predictable for users.
> Additionally, it is not consistent with how the Kbuild handles the
> M= option or GNU Make works with 'make -C /tmp/a/b/..'.
>
> Using the physical directory structure for the O= option seems more
> reasonable.
>
> The comment says "expand a shell special character '~'", but it has
> already been expanded to the home directory in the command line.

I minor change in behaviour is that 'make O="~/..."' (=quoted '~) will
not work any more. But I think this actually the way it should be.
Thanks!

Reviewed-by: Nicolas Schier <[email protected]>


> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> Makefile | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 24fac1889997..a05f0f7c99e0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -190,14 +190,11 @@ ifeq ("$(origin O)", "command line")
> endif
>
> ifneq ($(KBUILD_OUTPUT),)
> -# Make's built-in functions such as $(abspath ...), $(realpath ...) cannot
> -# expand a shell special character '~'. We use a somewhat tedious way here.
> -abs_objtree := $(shell mkdir -p $(KBUILD_OUTPUT) && cd $(KBUILD_OUTPUT) && pwd)
> -$(if $(abs_objtree),, \
> - $(error failed to create output directory "$(KBUILD_OUTPUT)"))
> -
> +# $(realpath ...) gets empty if the path does not exist. Run 'mkdir -p' first.
> +$(shell mkdir -p $(KBUILD_OUTPUT))
> # $(realpath ...) resolves symlinks
> -abs_objtree := $(realpath $(abs_objtree))
> +abs_objtree := $(realpath $(KBUILD_OUTPUT))
> +$(if $(abs_objtree),,$(error failed to create output directory "$(KBUILD_OUTPUT)"))
> endif # ifneq ($(KBUILD_OUTPUT),)
>
> ifneq ($(words $(subst :, ,$(abs_srctree))), 1)
> --
> 2.40.1
>


Attachments:
(No filename) (2.55 kB)
signature.asc (849.00 B)
Download all attachments

2023-12-15 15:59:45

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH] kbuild: resolve symlinks for O= properly

On Fri, Dec 15, 2023 at 6:29 PM Nicolas Schier <[email protected]> wrote:
>
> On Thu, Dec 14, 2023 at 11:03:05PM +0900, Masahiro Yamada wrote:
> > Currently, Kbuild follows the logical chain of directories for the O=
> > option, just like 'cd' (or 'realpath --logical') does.
> >
> > Example:
> >
> > $ mkdir -p /tmp/a /tmp/x/y
> > $ ln -s /tmp/x/y /tmp/a/b
> > $ realpath /tmp/a/b/..
> > /tmp/x
> > $ realpath --logical /tmp/a/b/..
> > /tmp/a
> > $ make O=/tmp/a/b/.. defconfig
> > make[1]: Entering directory '/tmp/a'
> > [snip]
> > make[1]: Leaving directory '/tmp/a'
> >
> > 'make O=/tmp/a/b/.. defconfig' creates the kernel configuration in
> > /tmp/a instead of /tmp/x despite the directory path /tmp/a/b/..
> > resolves to the physical directory path /tmp/x.
> >
> > This is because Kbuild internally uses the 'cd ... && pwd' for the
> > path resolution, but this behavior is not predictable for users.
> > Additionally, it is not consistent with how the Kbuild handles the
> > M= option or GNU Make works with 'make -C /tmp/a/b/..'.
> >
> > Using the physical directory structure for the O= option seems more
> > reasonable.
> >
> > The comment says "expand a shell special character '~'", but it has
> > already been expanded to the home directory in the command line.
>
> I minor change in behaviour is that 'make O="~/..."' (=quoted '~) will
> not work any more. But I think this actually the way it should be.
> Thanks!


A good catch. This bug should be fixed by quoting $(KBUILD_OUTPUT).


-$(shell mkdir -p $(KBUILD_OUTPUT))
+$(shell mkdir -p "$(KBUILD_OUTPUT)")



When ~ is quoted, it loses a special meaning.


masahiro@zoe:~$ echo ~
/home/masahiro
masahiro@zoe:~$ echo "~"
~



So, the right behaviour of 'make O="~/foo"' is
to create "~/foo" under the current directory.


If you try to test it, please be careful to
not delete your home directory.

(I did "rm -r ./~" just in case)











> Reviewed-by: Nicolas Schier <[email protected]>
>
>
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > ---
> >
> > Makefile | 11 ++++-------
> > 1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 24fac1889997..a05f0f7c99e0 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -190,14 +190,11 @@ ifeq ("$(origin O)", "command line")
> > endif
> >
> > ifneq ($(KBUILD_OUTPUT),)
> > -# Make's built-in functions such as $(abspath ...), $(realpath ...) cannot
> > -# expand a shell special character '~'. We use a somewhat tedious way here.
> > -abs_objtree := $(shell mkdir -p $(KBUILD_OUTPUT) && cd $(KBUILD_OUTPUT) && pwd)
> > -$(if $(abs_objtree),, \
> > - $(error failed to create output directory "$(KBUILD_OUTPUT)"))
> > -
> > +# $(realpath ...) gets empty if the path does not exist. Run 'mkdir -p' first.
> > +$(shell mkdir -p $(KBUILD_OUTPUT))
> > # $(realpath ...) resolves symlinks
> > -abs_objtree := $(realpath $(abs_objtree))
> > +abs_objtree := $(realpath $(KBUILD_OUTPUT))
> > +$(if $(abs_objtree),,$(error failed to create output directory "$(KBUILD_OUTPUT)"))
> > endif # ifneq ($(KBUILD_OUTPUT),)
> >
> > ifneq ($(words $(subst :, ,$(abs_srctree))), 1)
> > --
> > 2.40.1
> >



--
Best Regards
Masahiro Yamada