Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.debian.kernel > #57281 > unrolled thread
| Started by | Ben Hutchings <ben@decadent.org.uk> |
|---|---|
| First post | 2017-03-18 23:10 +0100 |
| Last post | 2017-04-04 09:10 +0200 |
| Articles | 6 — 4 participants |
Back to article view | Back to linux.debian.kernel
Bug#856474: [PATCH] Kbuild.include: addtree: Remove quotes before matching path Ben Hutchings <ben@decadent.org.uk> - 2017-03-18 23:10 +0100
Bug#856474: [PATCH] Kbuild.include: addtree: Remove quotes before matching path Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-04-03 10:00 +0200
Bug#856474: [PATCH] Kbuild.include: addtree: Remove quotes before matching path Michal Marek <mmarek@suse.com> - 2017-04-03 15:50 +0200
Bug#856474: [PATCH] Kbuild.include: addtree: Remove quotes before matching path Sam Ravnborg <sam@ravnborg.org> - 2017-04-03 22:40 +0200
Bug#856474: [PATCH] Kbuild.include: addtree: Remove quotes before matching path Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-04-04 06:10 +0200
Bug#856474: [PATCH] Kbuild.include: addtree: Remove quotes before matching path Michal Marek <mmarek@suse.com> - 2017-04-04 09:10 +0200
| From | Ben Hutchings <ben@decadent.org.uk> |
|---|---|
| Date | 2017-03-18 23:10 +0100 |
| Subject | Bug#856474: [PATCH] Kbuild.include: addtree: Remove quotes before matching path |
| Message-ID | <tmuHU-4rH-9@gated-at.bofh.it> |
[Multipart message — attachments visible in raw view] — view raw
systemtap currently fails to build modules when the kernel source and
object trees are separate.
systemtap adds something like -I"/usr/share/systemtap/runtime" to
EXTRA_CFLAGS, and addtree should not adjust this as it's specifying an
absolute directory. But since make has no understanding of shell
quoting, it does anyway.
For a long time this didn't matter, because addtree would still emit
the original -I option after the adjusted one. However, commit
db547ef19064 ("Kbuild: don't add obj tree in additional includes")
changed it to remove the original -I option.
Remove quotes (both double and single) before matching against the
excluded patterns.
References: https://bugs.debian.org/856474
Reported-and-tested-by: Jack Henschel <jackdev@mailbox.org>
Reported-and-tested-by: Ritesh Raj Sarraf <rrs@debian.org>
Fixes: db547ef19064 ("Kbuild: don't add obj tree in additional includes")
Cc: stable@vger.kernel.org # 4.8+
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -207,7 +207,7 @@ hdr-inst := -f $(srctree)/scripts/Makefi
# Prefix -I with $(srctree) if it is not an absolute path.
# skip if -I has no parameter
addtree = $(if $(patsubst -I%,%,$(1)), \
-$(if $(filter-out -I/% -I./% -I../%,$(1)),$(patsubst -I%,-I$(srctree)/%,$(1)),$(1)))
+$(if $(filter-out -I/% -I./% -I../%,$(subst $(quote),,$(subst $(squote),,$(1)))),$(patsubst -I%,-I$(srctree)/%,$(1)),$(1)))
# Find all -I options and call addtree
flags = $(foreach o,$($(1)),$(if $(filter -I%,$(o)),$(call addtree,$(o)),$(o)))
[toc] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-04-03 10:00 +0200 |
| Message-ID | <ts546-3XB-23@gated-at.bofh.it> |
| In reply to | #57281 |
+To Arnd
+To Michal
2017-03-19 6:58 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>:
> systemtap currently fails to build modules when the kernel source and
> object trees are separate.
>
> systemtap adds something like -I"/usr/share/systemtap/runtime" to
> EXTRA_CFLAGS, and addtree should not adjust this as it's specifying an
> absolute directory. But since make has no understanding of shell
> quoting, it does anyway.
>
> For a long time this didn't matter, because addtree would still emit
> the original -I option after the adjusted one. However, commit
> db547ef19064 ("Kbuild: don't add obj tree in additional includes")
> changed it to remove the original -I option.
>
> Remove quotes (both double and single) before matching against the
> excluded patterns.
>
> References: https://bugs.debian.org/856474
> Reported-and-tested-by: Jack Henschel <jackdev@mailbox.org>
> Reported-and-tested-by: Ritesh Raj Sarraf <rrs@debian.org>
> Fixes: db547ef19064 ("Kbuild: don't add obj tree in additional includes")
> Cc: stable@vger.kernel.org # 4.8+
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -207,7 +207,7 @@ hdr-inst := -f $(srctree)/scripts/Makefi
> # Prefix -I with $(srctree) if it is not an absolute path.
> # skip if -I has no parameter
> addtree = $(if $(patsubst -I%,%,$(1)), \
> -$(if $(filter-out -I/% -I./% -I../%,$(1)),$(patsubst -I%,-I$(srctree)/%,$(1)),$(1)))
> +$(if $(filter-out -I/% -I./% -I../%,$(subst $(quote),,$(subst $(squote),,$(1)))),$(patsubst -I%,-I$(srctree)/%,$(1)),$(1)))
>
> # Find all -I options and call addtree
> flags = $(foreach o,$($(1)),$(if $(filter -I%,$(o)),$(call addtree,$(o)),$(o)))
I have been thinking about this patch.
We touched addtree every time a new problem popped up.
It would be easy to pick up this patch,
but it looks like a crazy idea
to play pattern-matching in the addtree even more.
[1] -Ifoo/bar/baz
[2] -I"foo/bar/baz"
[3] -I foo/bar/baz (whitespace(s) after -I)
All of these are valid notation,
and should be handled in the same way.
However, addtree expects only [1],
(then this patch is going to add [2] in the soup).
Each Makefile knows it wants to see
additional headers in the source tree, or objtree.
I am guessing the right approach in a long run is,
we require -I to specify $(srctree) or $(objtree) explicitly.
ccflags-y := -I$(srctree)/foo/bar/baz
or
ccflags-y := -I$(objtree)/foo/bar/baz
(For the latter, we can omit $(objtree)/ as it is ./)
Then, delete $(call flags,_c_flags) after the conversion.
Any comments?
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Michal Marek <mmarek@suse.com> |
|---|---|
| Date | 2017-04-03 15:50 +0200 |
| Message-ID | <tsawO-7yd-21@gated-at.bofh.it> |
| In reply to | #57428 |
On 2017-04-03 09:42, Masahiro Yamada wrote: > Each Makefile knows it wants to see > additional headers in the source tree, or objtree. > > I am guessing the right approach in a long run is, > we require -I to specify $(srctree) or $(objtree) explicitly. > > ccflags-y := -I$(srctree)/foo/bar/baz > > or > > ccflags-y := -I$(objtree)/foo/bar/baz > > > (For the latter, we can omit $(objtree)/ as it is ./) > > > Then, delete $(call flags,_c_flags) after the conversion. Agreed. The addtree function is more of a hack to make things just work with O=, but AFAIK there is no clean way to implement VPATH for -I arguments. So it's sensible to get rid of the hack. It looks like it's going to be lot of work though: $ git grep -e '-I' -- '*Makefile*' | wc -l 732 $ git grep -e '-I *\$(\(src\|obj\)tree)' -- '*Makefile*' | wc -l 166 Michal
[toc] | [prev] | [next] | [standalone]
| From | Sam Ravnborg <sam@ravnborg.org> |
|---|---|
| Date | 2017-04-03 22:40 +0200 |
| Message-ID | <tsgVA-3k5-19@gated-at.bofh.it> |
| In reply to | #57434 |
On Mon, Apr 03, 2017 at 03:25:10PM +0200, Michal Marek wrote: > On 2017-04-03 09:42, Masahiro Yamada wrote: > > Each Makefile knows it wants to see > > additional headers in the source tree, or objtree. > > > > I am guessing the right approach in a long run is, > > we require -I to specify $(srctree) or $(objtree) explicitly. > > > > ccflags-y := -I$(srctree)/foo/bar/baz > > > > or > > > > ccflags-y := -I$(objtree)/foo/bar/baz > > > > > > (For the latter, we can omit $(objtree)/ as it is ./) > > > > > > Then, delete $(call flags,_c_flags) after the conversion. > > Agreed. The addtree function is more of a hack to make things just work > with O=, but AFAIK there is no clean way to implement VPATH for -I > arguments. So it's sensible to get rid of the hack. It looks like it's > going to be lot of work though: > > $ git grep -e '-I' -- '*Makefile*' | wc -l > 732 > $ git grep -e '-I *\$(\(src\|obj\)tree)' -- '*Makefile*' | wc -l > 166 There was a goal long time ago that moving the kernel source should not trigger a rebuild. Any hardcoded path would violate this (like $(srctree), $(objtree)) I dunno if this is really something to aim for today. I have personally from time to time renamed the directory where I have kernel soruce (which is seen like moving the kernel source), and would not be happy if this always triggered a full rebuild. But this is frankly a corner case. Sam
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-04-04 06:10 +0200 |
| Message-ID | <tsnX3-87d-1@gated-at.bofh.it> |
| In reply to | #57443 |
Hi Sam, 2017-04-04 5:20 GMT+09:00 Sam Ravnborg <sam@ravnborg.org>: > On Mon, Apr 03, 2017 at 03:25:10PM +0200, Michal Marek wrote: >> On 2017-04-03 09:42, Masahiro Yamada wrote: >> > Each Makefile knows it wants to see >> > additional headers in the source tree, or objtree. >> > >> > I am guessing the right approach in a long run is, >> > we require -I to specify $(srctree) or $(objtree) explicitly. >> > >> > ccflags-y := -I$(srctree)/foo/bar/baz >> > >> > or >> > >> > ccflags-y := -I$(objtree)/foo/bar/baz >> > >> > >> > (For the latter, we can omit $(objtree)/ as it is ./) >> > >> > >> > Then, delete $(call flags,_c_flags) after the conversion. >> >> Agreed. The addtree function is more of a hack to make things just work >> with O=, but AFAIK there is no clean way to implement VPATH for -I >> arguments. So it's sensible to get rid of the hack. It looks like it's >> going to be lot of work though: >> >> $ git grep -e '-I' -- '*Makefile*' | wc -l >> 732 >> $ git grep -e '-I *\$(\(src\|obj\)tree)' -- '*Makefile*' | wc -l >> 166 > > There was a goal long time ago that moving the kernel source should > not trigger a rebuild. > Any hardcoded path would violate this (like $(srctree), $(objtree)) Even if we avoid hard-coding $(srctree) in each Makefile, "addtree" will modify the -I paths and the modified paths will be recorded in .*.cmd files. If we really want to achieve the goal, my old patch seemed to have a point: https://patchwork.kernel.org/patch/4511991/ This was rejected due to side effects, anyway. -- Best Regards Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Michal Marek <mmarek@suse.com> |
|---|---|
| Date | 2017-04-04 09:10 +0200 |
| Message-ID | <tsqLf-1B3-11@gated-at.bofh.it> |
| In reply to | #57443 |
Dne 3.4.2017 v 22:20 Sam Ravnborg napsal(a): > On Mon, Apr 03, 2017 at 03:25:10PM +0200, Michal Marek wrote: >> On 2017-04-03 09:42, Masahiro Yamada wrote: >>> Each Makefile knows it wants to see >>> additional headers in the source tree, or objtree. >>> >>> I am guessing the right approach in a long run is, >>> we require -I to specify $(srctree) or $(objtree) explicitly. >>> >>> ccflags-y := -I$(srctree)/foo/bar/baz >>> >>> or >>> >>> ccflags-y := -I$(objtree)/foo/bar/baz >>> >>> >>> (For the latter, we can omit $(objtree)/ as it is ./) >>> >>> >>> Then, delete $(call flags,_c_flags) after the conversion. >> >> Agreed. The addtree function is more of a hack to make things just work >> with O=, but AFAIK there is no clean way to implement VPATH for -I >> arguments. So it's sensible to get rid of the hack. It looks like it's >> going to be lot of work though: >> >> $ git grep -e '-I' -- '*Makefile*' | wc -l >> 732 >> $ git grep -e '-I *\$(\(src\|obj\)tree)' -- '*Makefile*' | wc -l >> 166 > > There was a goal long time ago that moving the kernel source should > not trigger a rebuild. > Any hardcoded path would violate this (like $(srctree), $(objtree)) > > I dunno if this is really something to aim for today. > > I have personally from time to time renamed the directory where > I have kernel soruce (which is seen like moving the kernel source), > and would not be happy if this always triggered a full rebuild. > But this is frankly a corner case. This should be working nowadays if you build without O= or if the buildtree points to a subdirectory of the source tree. The srctree variable is set to . or .. in such case. Although when I tried it now, it still does not look perfect: $ cd /dev/shm/mmarek/linux-2.6 $ make O=build -s defconfig $ make O=build -s -j64 Setup is 15500 bytes (padded to 15872 bytes). System is 6530 kB CRC 4bc8f6e2 Kernel: arch/x86/boot/bzImage is ready (#1) $ grep -r $PWD build/ Binary file build/arch/x86/boot/setup.elf matches Binary file build/arch/x86/boot/header.o matches Binary file build/arch/x86/boot/video.o matches Binary file build/arch/x86/boot/cpu.o matches Binary file build/arch/x86/boot/printf.o matches Binary file build/arch/x86/boot/string.o matches Binary file build/arch/x86/boot/video-mode.o matches Binary file build/arch/x86/boot/video-vga.o matches Binary file build/arch/x86/boot/early_serial_console.o matches Binary file build/arch/x86/boot/video-vesa.o matches Binary file build/arch/x86/boot/cpucheck.o matches Binary file build/arch/x86/boot/video-bios.o matches Binary file build/arch/x86/boot/tty.o matches Binary file build/arch/x86/boot/memory.o matches Binary file build/arch/x86/boot/pm.o matches Binary file build/arch/x86/boot/cmdline.o matches Binary file build/arch/x86/boot/main.o matches Binary file build/arch/x86/boot/a20.o matches Binary file build/arch/x86/boot/regs.o matches Binary file build/arch/x86/boot/version.o matches Binary file build/arch/x86/boot/edd.o matches Binary file build/arch/x86/boot/cpuflags.o matches Binary file build/arch/x86/boot/pmjump.o matches Binary file build/arch/x86/boot/copy.o matches Binary file build/arch/x86/boot/bioscall.o matches Binary file build/arch/x86/realmode/rm/video-bios.o matches Binary file build/arch/x86/realmode/rm/video-mode.o matches Binary file build/arch/x86/realmode/rm/video-vga.o matches Binary file build/arch/x86/realmode/rm/video-vesa.o matches Binary file build/arch/x86/realmode/rm/wakemain.o matches Binary file build/arch/x86/realmode/rm/regs.o matches Binary file build/arch/x86/realmode/rm/trampoline_64.o matches Binary file build/arch/x86/realmode/rm/wakeup_asm.o matches Binary file build/arch/x86/realmode/rm/copy.o matches Binary file build/arch/x86/realmode/rm/bioscall.o matches Binary file build/arch/x86/realmode/rm/reboot.o matches All the 32bit object files contain the full path, but none of the .cmd files do?! And we do trigger rebuild of a few files after moving the tree, but we do so even in the original location, so it looks like somebody got a custom rule wrong again. Michal
[toc] | [prev] | [standalone]
Back to top | Article view | linux.debian.kernel
csiph-web