2023-12-15 16:08:15

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH v2] 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 /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]>
Reviewed-by: Nicolas Schier <[email protected]>
---

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



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

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

2024-01-22 15:29:17

by Nicolas Schier

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

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


Attachments:
(No filename) (1.09 kB)
signature.asc (849.00 B)
Download all attachments
Subject: Re: [PATCH v2] kbuild: resolve symlinks for O= properly

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

2024-01-22 16:53:56

by Nicolas Schier

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

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

2024-01-22 17:40:30

by David Laight

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

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)

2024-01-26 14:13:23

by Masahiro Yamada

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

On Mon, Jan 22, 2024 at 11:12 PM Sebastian Andrzej Siewior
<[email protected]> 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