diff mbox

[1/2] Makefile: don't rely on linux-tools being sorted alphabetically

Message ID 20170704235445.35255-2-code@mmayer.net
State Changes Requested
Headers show

Commit Message

Markus Mayer July 4, 2017, 11:54 p.m. UTC 
From: Markus Mayer <mmayer@broadcom.com>

The Linux tools master makefile (linux-tools.mk) must be included after
all Linux tools sub-makefiles (linux-tool-*.mk). This is currently
ensured by sorting all package makefiles alphabetically.

In preparation for adding linux-tool-tmon, we cannot keep relying on
this mechanism. In the C locale, linux-tool-tmon.mk will be sorted
behind linux-tools.mk, which would break the build.

Therefore, we ensure that linux-tools.mk always comes after
linux-tool-*.mk by explicitly including it after the other package
makefiles.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 Makefile                           | 6 +++++-
 package/linux-tools/linux-tools.mk | 8 +++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

Comments

Yann E. MORIN July 5, 2017, 7:25 a.m. UTC | #1 
Markus, all,

On 2017-07-04 16:54 -0700, Markus Mayer spake thusly:
> From: Markus Mayer <mmayer@broadcom.com>
> 
> The Linux tools master makefile (linux-tools.mk) must be included after
> all Linux tools sub-makefiles (linux-tool-*.mk). This is currently
> ensured by sorting all package makefiles alphabetically.
> 
> In preparation for adding linux-tool-tmon, we cannot keep relying on
> this mechanism. In the C locale, linux-tool-tmon.mk will be sorted
> behind linux-tools.mk, which would break the build.

That is not the case for me:

    $ touch package/linux-tools/linux-tool-tmon.mk
    $ LC_ALL=C ls -lh package/linux-tools/
    total 28K
    -rw-r--r-- 1 ymorin ymorin 2.8K Jul  3 23:55 Config.in
    -rw-r--r-- 1 ymorin ymorin 1.1K Jul  3 23:55 linux-tool-cpupower.mk
    -rw-r--r-- 1 ymorin ymorin  721 Jul  3 23:55 linux-tool-gpio.mk
    -rw-r--r-- 1 ymorin ymorin  730 Jul  3 23:55 linux-tool-iio.mk
    -rw-r--r-- 1 ymorin ymorin 4.0K Jul  3 23:55 linux-tool-perf.mk
    -rw-r--r-- 1 ymorin ymorin 1.5K Jul  3 23:55 linux-tool-selftests.mk
    -rw-r--r-- 1 ymorin ymorin    0 Jul  5 09:17 linux-tool-tmon.mk
    -rw-r--r-- 1 ymorin ymorin 2.0K Jul  3 23:55 linux-tools.mk

However, I am totally in favour of fixing it once and for all.

But rather than do a hack in the main Makefile as you suggest, I would
just ensure the ordering of the files.

I think we should just rneame the individual tool files to remove
the .mk extension, and then exp[licitly source them from
package/linux-tools/linux-tools.mk

    include $(wildcard package/linux-tools/linux-tool-*)

and be done with that once and for all.

Regards,
Yann E. MORIN.

> Therefore, we ensure that linux-tools.mk always comes after
> linux-tool-*.mk by explicitly including it after the other package
> makefiles.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  Makefile                           | 6 +++++-
>  package/linux-tools/linux-tools.mk | 8 +++-----
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index b37171fcf..b5596444d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -495,7 +495,11 @@ ifneq ($(PACKAGE_OVERRIDE_FILE),)
>  -include $(PACKAGE_OVERRIDE_FILE)
>  endif
>  
> -include $(sort $(wildcard package/*/*.mk))
> +# We need to insure that linux-tools.mk is included after all linux-tool-*.mk
> +# makefiles, and we can't rely on alphabetical sort order for this.
> +LINUX_TOOLS_MK = package/linux-tools/linux-tools.mk
> +include $(sort $(filter-out $(LINUX_TOOLS_MK),$(wildcard package/*/*.mk)))
> +include $(LINUX_TOOLS_MK)
>  
>  include boot/common.mk
>  include linux/linux.mk
> diff --git a/package/linux-tools/linux-tools.mk b/package/linux-tools/linux-tools.mk
> index 7fa8d194c..3d7a0bd92 100644
> --- a/package/linux-tools/linux-tools.mk
> +++ b/package/linux-tools/linux-tools.mk
> @@ -14,11 +14,9 @@
>  # to guarantee that each tool has a chance to register itself before we build
>  # the list of build and install hooks, below.
>  #
> -# This is currently guaranteed by the naming of each file:
> -# - they get included by the top-level Makefile, with $(sort $(wildcard ...))
> -# - make's $(sort) function will aways sort in the C locale
> -# - the files names correctly sort out in the C locale so that each tool's
> -#   .mk file is included before this one.
> +# This is ensured by the main buildroot makefile, which explicitly includes
> +# linux-tools.mk after all linux-tool*.mk makefiles. Look for LINUX_TOOLS_MK
> +# in the top-level makefile to see where this is happening.
>  
>  # We only need the kernel to be extracted, not actually built
>  LINUX_TOOLS_PATCH_DEPENDENCIES = linux
> -- 
> 2.13.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Yann E. MORIN July 5, 2017, 7:38 a.m. UTC | #2 
Markus, All,

On 2017-07-05 09:25 +0200, Yann E. MORIN spake thusly:
> On 2017-07-04 16:54 -0700, Markus Mayer spake thusly:
> > From: Markus Mayer <mmayer@broadcom.com>
> > 
> > The Linux tools master makefile (linux-tools.mk) must be included after
> > all Linux tools sub-makefiles (linux-tool-*.mk). This is currently
> > ensured by sorting all package makefiles alphabetically.
> > 
> > In preparation for adding linux-tool-tmon, we cannot keep relying on
> > this mechanism. In the C locale, linux-tool-tmon.mk will be sorted
> > behind linux-tools.mk, which would break the build.
[--SNIP--]
> However, I am totally in favour of fixing it once and for all.
> 
> But rather than do a hack in the main Makefile as you suggest, I would
> just ensure the ordering of the files.
> 
> I think we should just rneame the individual tool files to remove
> the .mk extension, and then exp[licitly source them from
> package/linux-tools/linux-tools.mk
> 
>     include $(wildcard package/linux-tools/linux-tool-*)
> 
> and be done with that once and for all.

But don't hurry in doing so, we've been discussing this IRL with Thomas,
and he is not happy with that proposal (but he agrees that touching the
main Makefile is not acceptable).

We're looking to find an alternative solution...

Regards,
Yann E. MORIN.
Arnout Vandecappelle July 5, 2017, 8:09 a.m. UTC | #3 
On 05-07-17 09:25, Yann E. MORIN wrote:
> Markus, all,
> 
> On 2017-07-04 16:54 -0700, Markus Mayer spake thusly:
>> From: Markus Mayer <mmayer@broadcom.com>
>>
>> The Linux tools master makefile (linux-tools.mk) must be included after
>> all Linux tools sub-makefiles (linux-tool-*.mk). This is currently
>> ensured by sorting all package makefiles alphabetically.
>>
>> In preparation for adding linux-tool-tmon, we cannot keep relying on
>> this mechanism. In the C locale, linux-tool-tmon.mk will be sorted
>> behind linux-tools.mk, which would break the build.
> That is not the case for me:

 Indeed, but it's just the commit message that is wrong. The sort is not done by
the sort program, but by make, and make doesn't get the LC_ALL=C passed to it.
So we indeed need to find a robust solution.

 Regards,
 Arnout
Arnout Vandecappelle July 5, 2017, 8:15 a.m. UTC | #4 
On 05-07-17 09:38, Yann E. MORIN wrote:
> Markus, All,
> 
> On 2017-07-05 09:25 +0200, Yann E. MORIN spake thusly:
>> On 2017-07-04 16:54 -0700, Markus Mayer spake thusly:
>>> From: Markus Mayer <mmayer@broadcom.com>
>>>
>>> The Linux tools master makefile (linux-tools.mk) must be included after
>>> all Linux tools sub-makefiles (linux-tool-*.mk). This is currently
>>> ensured by sorting all package makefiles alphabetically.
>>>
>>> In preparation for adding linux-tool-tmon, we cannot keep relying on
>>> this mechanism. In the C locale, linux-tool-tmon.mk will be sorted
>>> behind linux-tools.mk, which would break the build.
> [--SNIP--]
>> However, I am totally in favour of fixing it once and for all.
>>
>> But rather than do a hack in the main Makefile as you suggest, I would
>> just ensure the ordering of the files.
>>
>> I think we should just rneame the individual tool files to remove
>> the .mk extension, and then exp[licitly source them from
>> package/linux-tools/linux-tools.mk
>>
>>     include $(wildcard package/linux-tools/linux-tool-*)
>>
>> and be done with that once and for all.
> 
> But don't hurry in doing so, we've been discussing this IRL with Thomas,
> and he is not happy with that proposal (but he agrees that touching the
> main Makefile is not acceptable).
> 
> We're looking to find an alternative solution...

 The proposal is to move all the linux-tool-*.mk files to subdirectories of
package/linux-tools, and include them from the linux-tool.mk file at the
appropriate place. This is more in-line with how we do it for other packages,
e.g. qt5 or barebox. Ideally, the Config.in would also be split up into the
individual subpackages.

 Could you prepare a patch for that?

 Regards,
 Arnout
Arnout Vandecappelle July 5, 2017, 8:18 a.m. UTC | #5 
On 05-07-17 10:09, Arnout Vandecappelle wrote:
> 
> 
> On 05-07-17 09:25, Yann E. MORIN wrote:
>> Markus, all,
>>
>> On 2017-07-04 16:54 -0700, Markus Mayer spake thusly:
>>> From: Markus Mayer <mmayer@broadcom.com>
>>>
>>> The Linux tools master makefile (linux-tools.mk) must be included after
>>> all Linux tools sub-makefiles (linux-tool-*.mk). This is currently
>>> ensured by sorting all package makefiles alphabetically.
>>>
>>> In preparation for adding linux-tool-tmon, we cannot keep relying on
>>> this mechanism. In the C locale, linux-tool-tmon.mk will be sorted
>>> behind linux-tools.mk, which would break the build.
>> That is not the case for me:
> 
>  Indeed, but it's just the commit message that is wrong. The sort is not done by
> the sort program, but by make, and make doesn't get the LC_ALL=C passed to it.
> So we indeed need to find a robust solution.

 Hm, Yann tells me that make always uses the C locale for sorting. So why does
it fail for you? The C locale for sure sorts correctly.

 Also, the only thing that is sensitive to the order in this case is
LINUX_TOOLS_DEPENDENCIES. So you would miss the dependency on ncurses if it is
in the wrong order. Is that what happens for you? Could you tell us what error
you get?

 Regards,
 Arnout
Markus Mayer July 5, 2017, 5:19 p.m. UTC | #6 
On 5 July 2017 at 08:31, Markus Mayer <markus.mayer@broadcom.com> wrote:

> I got the error, "Your kernel version is too old and does not have the
> tmon tool.", because it tried building linux-tool-tmon before
> extracting the kernel. Running make a second time succeed, because it
> extracted the kernel right away. This was an Ubuntu box running 16.04.
>
> I will try again to see if I can still reproduce it.
>
> Also, I did figure out why it is sorted differently by "ls". The
> locale is set to en_CA.UTF-8. I thought I had reset the locale.
> Apparently not.
>
> $ echo $LANG
> en_CA.UTF-8
>
> $ ls -l
> total 32
> -rw-rw-r-- 1 mmayer mmayer 2965 Jul  4 11:13 Config.in
> -rw-rw-r-- 1 mmayer mmayer 1084 May  9 10:46 linux-tool-cpupower.mk
> -rw-rw-r-- 1 mmayer mmayer  721 May  9 10:46 linux-tool-gpio.mk
> -rw-rw-r-- 1 mmayer mmayer  730 May  9 10:46 linux-tool-iio.mk
> -rw-rw-r-- 1 mmayer mmayer 4015 May  9 10:46 linux-tool-perf.mk
> -rw-rw-r-- 1 mmayer mmayer 1469 May  9 10:46 linux-tool-selftests.mk
> -rw-rw-r-- 1 mmayer mmayer 1861 Jul  4 11:13 linux-tools.mk
> -rw-rw-r-- 1 mmayer mmayer  832 Jul  4 11:13 linux-tool-tmon.mk
>
> $ LANG=C ls -l
> total 32
> -rw-rw-r-- 1 mmayer mmayer 2965 Jul  4 11:13 Config.in
> -rw-rw-r-- 1 mmayer mmayer 1084 May  9 10:46 linux-tool-cpupower.mk
> -rw-rw-r-- 1 mmayer mmayer  721 May  9 10:46 linux-tool-gpio.mk
> -rw-rw-r-- 1 mmayer mmayer  730 May  9 10:46 linux-tool-iio.mk
> -rw-rw-r-- 1 mmayer mmayer 4015 May  9 10:46 linux-tool-perf.mk
> -rw-rw-r-- 1 mmayer mmayer 1469 May  9 10:46 linux-tool-selftests.mk
> -rw-rw-r-- 1 mmayer mmayer  832 Jul  4 11:13 linux-tool-tmon.mk
> -rw-rw-r-- 1 mmayer mmayer 1861 Jul  4 11:13 linux-tools.mk
>
> I'll experiment more with this and see what I can find out.

Looks like the build is actually succeeding for me now. Not sure what
happened originally when it bailed the way it did. My build must have
been in a weird state. My apologies for the false alarm. I
successfully built arm and arm64.

This means, we don't need patch 1/2 after all.

That said, and while I am a complete buildroot newbie, I do like the
suggestion of creating sub-directories for the tools and including the
makefiles from linux-tools.mk rather than relying on alphabetical
sorting.

Regards,
-Markus
Arnout Vandecappelle July 8, 2017, 9:16 p.m. UTC | #7 
On 05-07-17 19:19, Markus Mayer wrote:
> That said, and while I am a complete buildroot newbie, I do like the
> suggestion of creating sub-directories for the tools and including the
> makefiles from linux-tools.mk rather than relying on alphabetical
> sorting.

 If you're willing to upgrade from newbie to contributor, feel free to send a
patch that does exactly this!

 When you do, don't forget to update
docs/manual/adding-packages-linux-kernel-spec-infra.txt
as well.

 Regards,
 Arnout
Markus Mayer July 9, 2017, midnight UTC | #8 
On 8 July 2017 at 14:16, Arnout Vandecappelle <arnout@mind.be> wrote:
>
>
> On 05-07-17 19:19, Markus Mayer wrote:
>> That said, and while I am a complete buildroot newbie, I do like the
>> suggestion of creating sub-directories for the tools and including the
>> makefiles from linux-tools.mk rather than relying on alphabetical
>> sorting.
>
>  If you're willing to upgrade from newbie to contributor, feel free to send a
> patch that does exactly this!
>
>  When you do, don't forget to update
> docs/manual/adding-packages-linux-kernel-spec-infra.txt
> as well.

Sounds good. Will do. :-)

-Markus
Yann E. MORIN July 9, 2017, 8:13 a.m. UTC | #9 
Markus, All,

On 2017-07-05 10:19 -0700, Markus Mayer spake thusly:
[--SNIP--]
> That said, and while I am a complete buildroot newbie, I do like the
> suggestion of creating sub-directories for the tools and including the
> makefiles from linux-tools.mk rather than relying on alphabetical
> sorting.

To be sure we're on the same line: one single sub-directory with all tools
in there, not one sub-dir per tool.

Regards,
Yann E. MORIN.
Arnout Vandecappelle July 9, 2017, 11:10 a.m. UTC | #10 
On 09-07-17 10:13, Yann E. MORIN wrote:
> Markus, All,
> 
> On 2017-07-05 10:19 -0700, Markus Mayer spake thusly:
> [--SNIP--]
>> That said, and while I am a complete buildroot newbie, I do like the
>> suggestion of creating sub-directories for the tools and including the
>> makefiles from linux-tools.mk rather than relying on alphabetical
>> sorting.
> 
> To be sure we're on the same line: one single sub-directory with all tools
> in there, not one sub-dir per tool.

 Really? One sub-dir per tool would be more in-line with how we handle normal
packages.

 Regards,
 Arnout
Yann E. MORIN July 9, 2017, 12:41 p.m. UTC | #11 
Arnout, All,

On 2017-07-09 13:10 +0200, Arnout Vandecappelle spake thusly:
> On 09-07-17 10:13, Yann E. MORIN wrote:
> > Markus, All,
> > 
> > On 2017-07-05 10:19 -0700, Markus Mayer spake thusly:
> > [--SNIP--]
> >> That said, and while I am a complete buildroot newbie, I do like the
> >> suggestion of creating sub-directories for the tools and including the
> >> makefiles from linux-tools.mk rather than relying on alphabetical
> >> sorting.
> > 
> > To be sure we're on the same line: one single sub-directory with all tools
> > in there, not one sub-dir per tool.
> 
>  Really? One sub-dir per tool would be more in-line with how we handle normal
> packages.

Well, I would find it a bit over-blown to have many sub-dirs with a
single file (the .mk) in each.

Unless you also expected that the big Config.in file be split too?

I would argue against, because its fate is already a bit special: it is
not sourced from package/Config.in but from linux/Config.in. and I would
prefer we avoid a bit more complexity...

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> -- 
> Arnout Vandecappelle                          arnout at mind be
> Senior Embedded Software Architect            +32-16-286500
> Essensium/Mind                                http://www.mind.be
> G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
> LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
> GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
Markus Mayer July 10, 2017, 3:49 a.m. UTC | #12 
On 9 July 2017 at 05:41, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Arnout, All,
>
> On 2017-07-09 13:10 +0200, Arnout Vandecappelle spake thusly:
>> On 09-07-17 10:13, Yann E. MORIN wrote:
>> > Markus, All,
>> >
>> > On 2017-07-05 10:19 -0700, Markus Mayer spake thusly:
>> > [--SNIP--]
>> >> That said, and while I am a complete buildroot newbie, I do like the
>> >> suggestion of creating sub-directories for the tools and including the
>> >> makefiles from linux-tools.mk rather than relying on alphabetical
>> >> sorting.
>> >
>> > To be sure we're on the same line: one single sub-directory with all tools
>> > in there, not one sub-dir per tool.
>>
>>  Really? One sub-dir per tool would be more in-line with how we handle normal
>> packages.
>
> Well, I would find it a bit over-blown to have many sub-dirs with a
> single file (the .mk) in each.
>
> Unless you also expected that the big Config.in file be split too?
>
> I would argue against, because its fate is already a bit special: it is
> not sourced from package/Config.in but from linux/Config.in. and I would
> prefer we avoid a bit more complexity...

Yeah, I was wondering about that myself, actually. I do agree that
several directories containing only one file each don't seem all that
desirable. I'll see if I can get everything to work with just a single
sub-directory.

Regards,
-Markus
Markus Mayer July 13, 2017, 8:16 p.m. UTC | #13 
On 9 July 2017 at 20:49, Markus Mayer <mmayer@broadcom.com> wrote:
> On 9 July 2017 at 05:41, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>> Arnout, All,
>>
>> On 2017-07-09 13:10 +0200, Arnout Vandecappelle spake thusly:
>>> On 09-07-17 10:13, Yann E. MORIN wrote:
>>> > Markus, All,
>>> >
>>> > On 2017-07-05 10:19 -0700, Markus Mayer spake thusly:
>>> > [--SNIP--]
>>> >> That said, and while I am a complete buildroot newbie, I do like the
>>> >> suggestion of creating sub-directories for the tools and including the
>>> >> makefiles from linux-tools.mk rather than relying on alphabetical
>>> >> sorting.
>>> >
>>> > To be sure we're on the same line: one single sub-directory with all tools
>>> > in there, not one sub-dir per tool.
>>>
>>>  Really? One sub-dir per tool would be more in-line with how we handle normal
>>> packages.
>>
>> Well, I would find it a bit over-blown to have many sub-dirs with a
>> single file (the .mk) in each.
>>
>> Unless you also expected that the big Config.in file be split too?
>>
>> I would argue against, because its fate is already a bit special: it is
>> not sourced from package/Config.in but from linux/Config.in. and I would
>> prefer we avoid a bit more complexity...
>
> Yeah, I was wondering about that myself, actually. I do agree that
> several directories containing only one file each don't seem all that
> desirable. I'll see if I can get everything to work with just a single
> sub-directory.

I think I may need some help. I started implementing the suggested
solution, but I have run into some unexpected problems.

Specifically, if I move the linux-tool-*.mk files into their own
sub-directory, all linux-tools related targets disappear. It won't
build any linux-tools during a regular run of 'make' and it will
actually say "No rule..." when explicitly asked to build linux-tools:

$ make linux-tools
umask 0022 && make -C /home/mmayer/Development/buildroot
O=/home/mmayer/Development/output/arm64/. linux-tools
make[1]: *** No rule to make target 'linux-tools'.  Stop.
Makefile:16: recipe for target '_all' failed
make: *** [_all] Error 2

Here's what I have done:

(mmayer@lbrmn-mmayer) ~/Development/buildroot$ l package/linux-tools/
total 84
drwxrwxr-x    3 mmayer mmayer  4096 Jul 13 11:38 .
drwxrwxr-x 1843 mmayer mmayer 65536 Jul 10 12:28 ..
-rw-rw-r--    1 mmayer mmayer  2577 Jul 10 11:02 Config.in
-rw-rw-r--    1 mmayer mmayer  2049 Jul 13 11:20 linux-tools.mk
drwxrwxr-x    2 mmayer mmayer  4096 Jul 13 11:20 tools

(mmayer@lbrmn-mmayer) ~/Development/buildroot$ l package/linux-tools/tools/
total 28
drwxrwxr-x 2 mmayer mmayer 4096 Jul 13 11:20 .
drwxrwxr-x 3 mmayer mmayer 4096 Jul 13 11:38 ..
-rw-rw-r-- 1 mmayer mmayer 1061 Jul 10 13:04 linux-tool-cpupower.mk
-rw-rw-r-- 1 mmayer mmayer  753 Jul 10 15:27 linux-tool-gpio.mk
-rw-rw-r-- 1 mmayer mmayer  730 Jul 10 13:04 linux-tool-iio.mk
-rw-rw-r-- 1 mmayer mmayer 4015 Jul 10 13:04 linux-tool-perf.mk
-rw-rw-r-- 1 mmayer mmayer 1469 Jul 10 13:04 linux-tool-selftests.mk

linux-tools.mk gains this new line at the very beginning:

include $(sort $(wildcard package/linux-tools/tools/*.mk))

This results in the aforementioned non-build of linux-tools. Though
when I look at all the variables, they are set properly. LINUX_TOOLS,
LINUX_TOOLS_DEPENDENCIES, etc. all have the proper values. Everything
*looks* like it should work, but somehow the linux-tools targets do
not exist, so it doesn't actually work.

I tried something else for test-purposes. If I don't put the
linux-tool-*.mk into their separate sub-directory and, instead, simply
change their extension (so they don't match the *.mk pattern in the
main Makefile), it all starts to work perfectly.

$ l package/linux-tools/
total 104
drwxrwxr-x    3 mmayer mmayer  4096 Jul 13 11:46 .
drwxrwxr-x 1843 mmayer mmayer 65536 Jul 10 12:28 ..
-rw-rw-r--    1 mmayer mmayer  2577 Jul 10 11:02 Config.in
-rw-rw-r--    1 mmayer mmayer  1061 Jul 10 13:04 linux-tool-cpupower.xmk
-rw-rw-r--    1 mmayer mmayer   753 Jul 10 15:27 linux-tool-gpio.xmk
-rw-rw-r--    1 mmayer mmayer   730 Jul 10 13:04 linux-tool-iio.xmk
-rw-rw-r--    1 mmayer mmayer  4015 Jul 10 13:04 linux-tool-perf.xmk
-rw-rw-r--    1 mmayer mmayer  1469 Jul 10 13:04 linux-tool-selftests.xmk
-rw-rw-r--    1 mmayer mmayer  2043 Jul 13 11:46 linux-tools.mk

$ grep ^include package/linux-tools/linux-tools.mk
include $(sort $(wildcard package/linux-tools/*.xmk))

So, now, everything is at the package/linux-tools level again. The sub
makefiles simply have the extension .xmk. This also ensures they don't
get picked up by the main Makefile, but by linux-tools.mk. This setup
works flawlessly.

As far as I can tell, it should make no difference if the
sub-makefiles are being included from package/linux-tools or
package/linux-tools/tools. All the make variables are being set to the
same values in both cases. Somehow, however, it doesn't generate the
proper target list in the sub-directory case, but does the right thing
if it's all in the same directory.

Does anybody have any pointers as to why this might be and what to do about it?

Thanks,
-Markus
Yann E. MORIN July 13, 2017, 9:45 p.m. UTC | #14 
Markus, All,

On 2017-07-13 13:16 -0700, Markus Mayer spake thusly:
[--SNIP--]
> I think I may need some help. I started implementing the suggested
> solution, but I have run into some unexpected problems.
> 
> Specifically, if I move the linux-tool-*.mk files into their own
> sub-directory, all linux-tools related targets disappear. It won't
> build any linux-tools during a regular run of 'make' and it will
> actually say "No rule..." when explicitly asked to build linux-tools:
> 
> $ make linux-tools
> umask 0022 && make -C /home/mmayer/Development/buildroot
> O=/home/mmayer/Development/output/arm64/. linux-tools
> make[1]: *** No rule to make target 'linux-tools'.  Stop.
> Makefile:16: recipe for target '_all' failed
> make: *** [_all] Error 2
> 
> Here's what I have done:
> 
> (mmayer@lbrmn-mmayer) ~/Development/buildroot$ l package/linux-tools/
> total 84
> drwxrwxr-x    3 mmayer mmayer  4096 Jul 13 11:38 .
> drwxrwxr-x 1843 mmayer mmayer 65536 Jul 10 12:28 ..
> -rw-rw-r--    1 mmayer mmayer  2577 Jul 10 11:02 Config.in
> -rw-rw-r--    1 mmayer mmayer  2049 Jul 13 11:20 linux-tools.mk
> drwxrwxr-x    2 mmayer mmayer  4096 Jul 13 11:20 tools

OK, I will attempt a wild gues here... Rename the 'tools' directory to
'linux-tools' and try again.

[--SNIP--]
> Does anybody have any pointers as to why this might be and what to do about it?

Yes, this is becasue the last line in package/linux-tools/linux-tools.mk
is calling to the generic-package infrastructure.

In turn, that line will at some place evaluate the $(pkgname) macro.
pkgname is defined as thus:

    pkgname = $(lastword $(subst /, ,$(pkgdir)))

And in turn, pkgdir is defined as:

    pkgdir = $(dir $(lastword $(MAKEFILE_LIST)))

So, what this does is get the filename of the last included makefile,
keep the directopry part of it, and then extract the last component of
that, to get the package name.

By moving the tools to a sub-directory named 'tools' make that the last
makefile parsed is in that sub-directory by the time pkgname is called,
and it then gets 'tools' instead of 'linux-tools'.

We have a similar situation in linux/linux.mk, see line 425 where we
have an explanation about this:

  423 # Include all our extensions.
  424 #
  425 # Note: our package infrastructure uses the full-path of the last-scanned
  426 # Makefile to determine what package we're currently defining, using the
  427 # last directory component in the path. As such, including other Makefile,
  428 # like below, before we call one of the *-package macro is usally not
  429 # working.
  430 # However, since the files we include here are in the same directory as
  431 # the current Makefile, we are OK. But this is a hard requirement: files
  432 # included here *must* be in the same directory!
  433 include $(sort $(wildcard linux/linux-ext-*.mk))

So I would argue against the sub-directory, but in favour of renaming
the files.

Except that is not necessary, is it? IIRC, you confirmed that this was
in fact not a problem, right?

So, rather than try to fix a problem that does not exist, using fragile
heuristics, I would recommend we keep the status-quo.

Regards,
Yann E. MORIN.
Markus Mayer July 13, 2017, 11:59 p.m. UTC | #15 
On 13 July 2017 at 14:45, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Markus, All,
>
> On 2017-07-13 13:16 -0700, Markus Mayer spake thusly:
> [--SNIP--]
>> I think I may need some help. I started implementing the suggested
>> solution, but I have run into some unexpected problems.
>>
>> Specifically, if I move the linux-tool-*.mk files into their own
>> sub-directory, all linux-tools related targets disappear. It won't
>> build any linux-tools during a regular run of 'make' and it will
>> actually say "No rule..." when explicitly asked to build linux-tools:
>>
>> $ make linux-tools
>> umask 0022 && make -C /home/mmayer/Development/buildroot
>> O=/home/mmayer/Development/output/arm64/. linux-tools
>> make[1]: *** No rule to make target 'linux-tools'.  Stop.
>> Makefile:16: recipe for target '_all' failed
>> make: *** [_all] Error 2
>>
>> Here's what I have done:
>>
>> (mmayer@lbrmn-mmayer) ~/Development/buildroot$ l package/linux-tools/
>> total 84
>> drwxrwxr-x    3 mmayer mmayer  4096 Jul 13 11:38 .
>> drwxrwxr-x 1843 mmayer mmayer 65536 Jul 10 12:28 ..
>> -rw-rw-r--    1 mmayer mmayer  2577 Jul 10 11:02 Config.in
>> -rw-rw-r--    1 mmayer mmayer  2049 Jul 13 11:20 linux-tools.mk
>> drwxrwxr-x    2 mmayer mmayer  4096 Jul 13 11:20 tools
>
> OK, I will attempt a wild gues here... Rename the 'tools' directory to
> 'linux-tools' and try again.

Your guess was spot-on. That was exactly it.

[...]

> We have a similar situation in linux/linux.mk, see line 425 where we
> have an explanation about this:
>
>   423 # Include all our extensions.
>   424 #
>   425 # Note: our package infrastructure uses the full-path of the last-scanned
>   426 # Makefile to determine what package we're currently defining, using the
>   427 # last directory component in the path. As such, including other Makefile,
>   428 # like below, before we call one of the *-package macro is usally not
>   429 # working.
>   430 # However, since the files we include here are in the same directory as
>   431 # the current Makefile, we are OK. But this is a hard requirement: files
>   432 # included here *must* be in the same directory!
>   433 include $(sort $(wildcard linux/linux-ext-*.mk))
>
> So I would argue against the sub-directory, but in favour of renaming
> the files.
>
> Except that is not necessary, is it? IIRC, you confirmed that this was
> in fact not a problem, right?

Yes, it is not a problem for linux-tool-tmon.mk. I originally thought it was.

> So, rather than try to fix a problem that does not exist, using fragile
> heuristics, I would recommend we keep the status-quo.

It was my understanding that the alphabetical sorting of linux-tools
has been considered to be not really all that desirable and that a
"once and for all" solution that does away with this requirement would
be welcomed. That's why I looked into these changes.

I'll leave the decision to the maintainers as to whether this should
be changed (either using a sub-directory or an file extension other
than .mk) or if it is preferable to keep the alphabetical sorting. My
personal take is that relying on alphabetical sorting is more fragile
than the alternate solution, but it is up to you guys.

Thanks,
-Markus
Yann E. MORIN July 16, 2017, 3:08 p.m. UTC | #16 
Peter, Thomas, Arnout, All,

Your position on Markus' (non-)issue?

Do we keep the code as-is, or do we "fix" it so that it is not
ambiguous?

I've checked, and make uses strcmp(3) internally, and strcmp() does not
use locales at all; it just compares chars.

Regards,
Yann E. MORIN.

On 2017-07-13 16:59 -0700, Markus Mayer spake thusly:
> On 13 July 2017 at 14:45, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > Markus, All,
> >
> > On 2017-07-13 13:16 -0700, Markus Mayer spake thusly:
> > [--SNIP--]
> >> I think I may need some help. I started implementing the suggested
> >> solution, but I have run into some unexpected problems.
> >>
> >> Specifically, if I move the linux-tool-*.mk files into their own
> >> sub-directory, all linux-tools related targets disappear. It won't
> >> build any linux-tools during a regular run of 'make' and it will
> >> actually say "No rule..." when explicitly asked to build linux-tools:
> >>
> >> $ make linux-tools
> >> umask 0022 && make -C /home/mmayer/Development/buildroot
> >> O=/home/mmayer/Development/output/arm64/. linux-tools
> >> make[1]: *** No rule to make target 'linux-tools'.  Stop.
> >> Makefile:16: recipe for target '_all' failed
> >> make: *** [_all] Error 2
> >>
> >> Here's what I have done:
> >>
> >> (mmayer@lbrmn-mmayer) ~/Development/buildroot$ l package/linux-tools/
> >> total 84
> >> drwxrwxr-x    3 mmayer mmayer  4096 Jul 13 11:38 .
> >> drwxrwxr-x 1843 mmayer mmayer 65536 Jul 10 12:28 ..
> >> -rw-rw-r--    1 mmayer mmayer  2577 Jul 10 11:02 Config.in
> >> -rw-rw-r--    1 mmayer mmayer  2049 Jul 13 11:20 linux-tools.mk
> >> drwxrwxr-x    2 mmayer mmayer  4096 Jul 13 11:20 tools
> >
> > OK, I will attempt a wild gues here... Rename the 'tools' directory to
> > 'linux-tools' and try again.
> 
> Your guess was spot-on. That was exactly it.
> 
> [...]
> 
> > We have a similar situation in linux/linux.mk, see line 425 where we
> > have an explanation about this:
> >
> >   423 # Include all our extensions.
> >   424 #
> >   425 # Note: our package infrastructure uses the full-path of the last-scanned
> >   426 # Makefile to determine what package we're currently defining, using the
> >   427 # last directory component in the path. As such, including other Makefile,
> >   428 # like below, before we call one of the *-package macro is usally not
> >   429 # working.
> >   430 # However, since the files we include here are in the same directory as
> >   431 # the current Makefile, we are OK. But this is a hard requirement: files
> >   432 # included here *must* be in the same directory!
> >   433 include $(sort $(wildcard linux/linux-ext-*.mk))
> >
> > So I would argue against the sub-directory, but in favour of renaming
> > the files.
> >
> > Except that is not necessary, is it? IIRC, you confirmed that this was
> > in fact not a problem, right?
> 
> Yes, it is not a problem for linux-tool-tmon.mk. I originally thought it was.
> 
> > So, rather than try to fix a problem that does not exist, using fragile
> > heuristics, I would recommend we keep the status-quo.
> 
> It was my understanding that the alphabetical sorting of linux-tools
> has been considered to be not really all that desirable and that a
> "once and for all" solution that does away with this requirement would
> be welcomed. That's why I looked into these changes.
> 
> I'll leave the decision to the maintainers as to whether this should
> be changed (either using a sub-directory or an file extension other
> than .mk) or if it is preferable to keep the alphabetical sorting. My
> personal take is that relying on alphabetical sorting is more fragile
> than the alternate solution, but it is up to you guys.
> 
> Thanks,
> -Markus
Thomas Petazzoni July 17, 2017, 7:37 a.m. UTC | #17 
Hello,

On Sun, 16 Jul 2017 17:08:59 +0200, Yann E. MORIN wrote:

> Your position on Markus' (non-)issue?
> 
> Do we keep the code as-is, or do we "fix" it so that it is not
> ambiguous?
> 
> I've checked, and make uses strcmp(3) internally, and strcmp() does not
> use locales at all; it just compares chars.

I don't have a very strong opinion on this. To me, it would be fine to
have linux-tools.mk and then the per-tool files called
linux-tool-<foo>.mk.in or something like that.

Best regards,

Thomas
Yann E. MORIN July 17, 2017, 3:36 p.m. UTC | #18 
Thomas, Markus, All,

On 2017-07-17 09:37 +0200, Thomas Petazzoni spake thusly:
> On Sun, 16 Jul 2017 17:08:59 +0200, Yann E. MORIN wrote:
> 
> > Your position on Markus' (non-)issue?
> > 
> > Do we keep the code as-is, or do we "fix" it so that it is not
> > ambiguous?
> > 
> > I've checked, and make uses strcmp(3) internally, and strcmp() does not
> > use locales at all; it just compares chars.
> 
> I don't have a very strong opinion on this. To me, it would be fine to
> have linux-tools.mk and then the per-tool files called
> linux-tool-<foo>.mk.in or something like that.

Markus, would you want to rework your patch accordingly? Thanks! :-)

Regards,
Yann E. MORIN.
Markus Mayer July 17, 2017, 5:32 p.m. UTC | #19 
On 17 July 2017 at 08:36, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Thomas, Markus, All,
>
> On 2017-07-17 09:37 +0200, Thomas Petazzoni spake thusly:
>> On Sun, 16 Jul 2017 17:08:59 +0200, Yann E. MORIN wrote:
>>
>> > Your position on Markus' (non-)issue?
>> >
>> > Do we keep the code as-is, or do we "fix" it so that it is not
>> > ambiguous?
>> >
>> > I've checked, and make uses strcmp(3) internally, and strcmp() does not
>> > use locales at all; it just compares chars.
>>
>> I don't have a very strong opinion on this. To me, it would be fine to
>> have linux-tools.mk and then the per-tool files called
>> linux-tool-<foo>.mk.in or something like that.
>
> Markus, would you want to rework your patch accordingly? Thanks! :-)

Sure. Will do.

-Markus
diff mbox

Patch

diff --git a/Makefile b/Makefile
index b37171fcf..b5596444d 100644
--- a/Makefile
+++ b/Makefile
@@ -495,7 +495,11 @@  ifneq ($(PACKAGE_OVERRIDE_FILE),)
 -include $(PACKAGE_OVERRIDE_FILE)
 endif
 
-include $(sort $(wildcard package/*/*.mk))
+# We need to insure that linux-tools.mk is included after all linux-tool-*.mk
+# makefiles, and we can't rely on alphabetical sort order for this.
+LINUX_TOOLS_MK = package/linux-tools/linux-tools.mk
+include $(sort $(filter-out $(LINUX_TOOLS_MK),$(wildcard package/*/*.mk)))
+include $(LINUX_TOOLS_MK)
 
 include boot/common.mk
 include linux/linux.mk
diff --git a/package/linux-tools/linux-tools.mk b/package/linux-tools/linux-tools.mk
index 7fa8d194c..3d7a0bd92 100644
--- a/package/linux-tools/linux-tools.mk
+++ b/package/linux-tools/linux-tools.mk
@@ -14,11 +14,9 @@ 
 # to guarantee that each tool has a chance to register itself before we build
 # the list of build and install hooks, below.
 #
-# This is currently guaranteed by the naming of each file:
-# - they get included by the top-level Makefile, with $(sort $(wildcard ...))
-# - make's $(sort) function will aways sort in the C locale
-# - the files names correctly sort out in the C locale so that each tool's
-#   .mk file is included before this one.
+# This is ensured by the main buildroot makefile, which explicitly includes
+# linux-tools.mk after all linux-tool*.mk makefiles. Look for LINUX_TOOLS_MK
+# in the top-level makefile to see where this is happening.
 
 # We only need the kernel to be extracted, not actually built
 LINUX_TOOLS_PATCH_DEPENDENCIES = linux