[PATCH v2] kbuild: resolve symlinks for O= properly

[PATCH v2] kbuild: resolve symlinks for O= properly

[Masahiro Yamada]

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 /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 <masahiroy@kernel.org>
Reviewed-by: Nicolas Schier <n.schier@avm.de>
---

Changes in v2:
  - Quote $(KBUILD_OUTPUT). If ~ is contained in KBUILD_OUTPUT,
    it should not expanded any further.

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

diff --git a/Makefile b/Makefile
index 04a86c81b5c0..5036edc40f46 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

Re: [PATCH v2] kbuild: resolve symlinks for O= properly

[Sebastian Andrzej Siewior]

On 2023-12-16 01:06:37 [+0900], Masahiro Yamada wrote:
…
> 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.

It might have been expanded, it might have not been expanded. Having a
shell script:
| #!/bin/sh
| 
| exec make O=~/scratch/mk-check defconfig

with bin/sh = dash results in:

| make[1]: Entering directory '/home/bigeasy/linux/~/scratch/mk-check'

while bin/sh = bash expands the ~ properly before for O=. Would it be
too much to ask, to expand the ~?

Sebastian

Re: [PATCH v2] kbuild: resolve symlinks for O= properly

[Nicolas Schier]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

On Mon, Jan 22, 2024 at 03:12:03PM +0100, Sebastian Andrzej Siewior wrote:
> On 2023-12-16 01:06:37 [+0900], Masahiro Yamada wrote:
> …
> > 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.
> 
> It might have been expanded, it might have not been expanded. Having a
> shell script:
> | #!/bin/sh
> | 
> | exec make O=~/scratch/mk-check defconfig
> 
> with bin/sh = dash results in:
> 
> | make[1]: Entering directory '/home/bigeasy/linux/~/scratch/mk-check'
> 
> while bin/sh = bash expands the ~ properly before for O=. Would it be
> too much to ask, to expand the ~?

Expanding tilde expandos is traditionally a shell feature, as you
already mentioned; and bash supports also expandos like '~+' and '~-'.
I think, we should leave the shell things in shells.

Thus, please update your shell scripts to be compliant to their
interpreting shell (e.g. use '$HOME' or switch the shell).

Kind regards,
Nicolas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] kbuild: resolve symlinks for O= properly

[Sebastian Andrzej Siewior]

On 2024-01-22 15:59:37 [+0100], Nicolas Schier wrote:
> Expanding tilde expandos is traditionally a shell feature, as you
> already mentioned; and bash supports also expandos like '~+' and '~-'.
> I think, we should leave the shell things in shells.
> 
> Thus, please update your shell scripts to be compliant to their
> interpreting shell (e.g. use '$HOME' or switch the shell).

I reported that this change silently broke dash users. If you accept
this then you could acknowledge it instead of suggesting workarounds.

> Kind regards,
> Nicolas

Sebastian

Re: [PATCH v2] kbuild: resolve symlinks for O= properly

[Nicolas Schier]

On Mon, Jan 22, 2024 at 04:05:01PM +0100, Sebastian Andrzej Siewior wrote:
> On 2024-01-22 15:59:37 [+0100], Nicolas Schier wrote:
> > Expanding tilde expandos is traditionally a shell feature, as you
> > already mentioned; and bash supports also expandos like '~+' and '~-'.
> > I think, we should leave the shell things in shells.
> > 
> > Thus, please update your shell scripts to be compliant to their
> > interpreting shell (e.g. use '$HOME' or switch the shell).
> 
> I reported that this change silently broke dash users. If you accept
> this then you could acknowledge it instead of suggesting workarounds.

I'm sorry, I didn't mean to upset you.  Yes, we were aware that the
change breaks some usage patterns.  Thanks for pointing that out.

Kind regards,
Nicolas

RE: [PATCH v2] kbuild: resolve symlinks for O= properly

[David Laight]

From: Sebastian Andrzej Siewior
> Sent: 22 January 2024 14:12
> 
> On 2023-12-16 01:06:37 [+0900], Masahiro Yamada wrote:
> …
> > 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.
> 
> It might have been expanded, it might have not been expanded. Having a
> shell script:
> | #!/bin/sh
> |
> | exec make O=~/scratch/mk-check defconfig
> 
> with bin/sh = dash results in:
> 
> | make[1]: Entering directory '/home/bigeasy/linux/~/scratch/mk-check'
> 
> while bin/sh = bash expands the ~ properly before for O=. Would it be
> too much to ask, to expand the ~?

Raise a bug on dash...

Or split the lines:
O=~/xxxx
make O="$O" ...

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2] kbuild: resolve symlinks for O= properly

[Masahiro Yamada]

On Mon, Jan 22, 2024 at 11:12 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2023-12-16 01:06:37 [+0900], Masahiro Yamada wrote:
> …
> > 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.
>
> It might have been expanded, it might have not been expanded. Having a
> shell script:
> | #!/bin/sh
> |
> | exec make O=~/scratch/mk-check defconfig
>
> with bin/sh = dash results in:
>
> | make[1]: Entering directory '/home/bigeasy/linux/~/scratch/mk-check'
>
> while bin/sh = bash expands the ~ properly before for O=. Would it be
> too much to ask, to expand the ~?



Not only O=.


If the shell does not expand the '~' character,
there are more variables that do not work as expected.

For example,


$ make CROSS_COMPILE=~/path/to/compiler/dir

$ make M=~/path/to/external/module/dir'



It is strange to require only O= to expand the '~' character.


So, Kbuild should be agnostic about '~'. This is consistent.




>
> Sebastian
>


-- 
Best Regards
Masahiro Yamada