[Buildroot] a philosophical question about Config.in and "comment" directives

[Buildroot] a philosophical question about Config.in and "comment" directives

[Robert P. J. Day]

  mostly a pedantic question about Config.in file aesthetics, but
consider this one, ripped straight from the code base:

  config BR2_PACKAGE_A10DISP
        bool "a10disp"
        depends on BR2_arm
        depends on BR2_LINUX_KERNEL
        help
          Program to change the display mode of Allwinner ARM SOCs running
          the linux-sunxi kernel (and not the mainline kernel.)

          http://github.com/hglm/a10disp

  comment "a10disp needs a Linux kernel to be built"
        depends on BR2_arm
        depends on !BR2_LINUX_KERNEL

  yes, i can see what the above does ... first, it depends on arm to
even be processed. furthermore, it depends on the selection of a
linux kernel, otherwise the comment kicks in to warn the user that
that option requires a kernel. so far, fine. but here's the rub.

  as i see it, there are two levels of "dependency" for this option.
first, there's the dependency on arch being arm, and you can see that
the dependency line

   depends on BR2_arm

appears in *both* the config and comment directives. so without that,
you see *nothing* about that selection -- not the choice, nor the
comment warning you about it.

  OTOH, there is the dependency on a kernel, for which this file
*will* generate a comment stating that, if you want this selection,
you'll need a kernel. in *that* case, the user is being politely
notified that, while this selection is not currently available,
they're "close", and they can see it if only they select a kernel.

  from my perspective, those are two different levels of dependency.
the dependency on arch being arm should be treated as wrapping the
*entire* config file. i mean, you'd never generate a comment stating,
"this option requires you to have selected arch = arm" -- that would
be just silly. but telling someone they need a kernel to configure
this option *does* make sense.

  to distinguish between these two "levels" of dependency, i think it
would be far clearer to rewrite a file like that as:

  if BR2_arm

  config BR2_PACKAGE_A10DISP
        bool "a10disp"
        depends on BR2_LINUX_KERNEL
        help
          Program to change the display mode of Allwinner ARM SOCs running
          the linux-sunxi kernel (and not the mainline kernel.)

          http://github.com/hglm/a10disp

  comment "a10disp needs a Linux kernel to be built"
        depends on !BR2_LINUX_KERNEL

  endif

that layout makes it far clearer that the entire option depends on
arm or you see *nothing* and, further, internally, the dependencies
in the comment list *only* those dependencies that the user will be
warned that they need if they want this selection.

  i just think having the dependency line

  depends on BR2_arm

in both the config and comment directives is unnecessary duplication,
and that that kind of dependency should be moved up to encompass the
entire Config.in file, however that's best done.

  thoughts?

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

[Buildroot] a philosophical question about Config.in and "comment" directives

[Yann E. MORIN]

Rovert, All,

On 2015-04-17 08:58 -0400, Robert P. J. Day spake thusly:
[--SNIP--]
>   to distinguish between these two "levels" of dependency, i think it
> would be far clearer to rewrite a file like that as:
> 
>   if BR2_arm
> 
>   config BR2_PACKAGE_A10DISP
>         bool "a10disp"
>         depends on BR2_LINUX_KERNEL
>         help
>           Program to change the display mode of Allwinner ARM SOCs running
>           the linux-sunxi kernel (and not the mainline kernel.)
> 
>           http://github.com/hglm/a10disp
> 
>   comment "a10disp needs a Linux kernel to be built"
>         depends on !BR2_LINUX_KERNEL
> 
>   endif
> 
> that layout makes it far clearer that the entire option depends on
> arm or you see *nothing* and, further, internally, the dependencies
> in the comment list *only* those dependencies that the user will be
> warned that they need if they want this selection.
> 
>   i just think having the dependency line
> 
>   depends on BR2_arm
> 
> in both the config and comment directives is unnecessary duplication,
> and that that kind of dependency should be moved up to encompass the
> entire Config.in file, however that's best done.
> 
>   thoughts?

Well, you are right that "it would make moere sense" from a theoretical
point of view, and that there is no functional difference. BTW, there
are other  such architectural options, like MMU, that we handle the same
way as well.

Note however, we have more complex packages, like for example WebKit,
for which we move the architectural dependencies into their own option,
like so:

    config BR2_PACKAGE_WEBKIT_ARCH_SUPPORTS
        bool
        # ARM needs BLX, so v5t+
        default y if (BR2_arm || BR2_armeb) && !BR2_ARM_CPU_ARMV4
        default y if BR2_i386 || BR2_mips || BR2_mipsel || \
                BR2_sparc || BR2_x86_64
        depends on BR2_USE_MMU # libgail -> pango -> libglib2

That's because this way, other packages that may want to select WebKit
can select it by just adding a dependency on BR2_PACKAGE_WEBKIR_ARCH_SUPPORT.

And of course, that's the way packages have been written historically.
Changing all the packages is just not feasible, and maintainability and
homogeneity trump eye-candy quite easily. ;-)

So yes, you are right _on principle_. But we're not gonna change that
policy, and we'll continue to require new packages to conform to it.

Just a side note: I personally find it easier to read the way we have it
now: having the "depends on" directly in the package dependency list
looks more obvious to me (but hey! I'm kind of a weirdo! ;-) 

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] a philosophical question about Config.in and "comment" directives

[Arnout Vandecappelle]

On 17/04/15 17:00, Yann E. MORIN wrote:
> Rovert, All,
> 
> On 2015-04-17 08:58 -0400, Robert P. J. Day spake thusly:
> [--SNIP--]
>>   to distinguish between these two "levels" of dependency, i think it
>> would be far clearer to rewrite a file like that as:
>>
>>   if BR2_arm
>>
>>   config BR2_PACKAGE_A10DISP
>>         bool "a10disp"
>>         depends on BR2_LINUX_KERNEL
>>         help
>>           Program to change the display mode of Allwinner ARM SOCs running
>>           the linux-sunxi kernel (and not the mainline kernel.)
>>
>>           http://github.com/hglm/a10disp
>>
>>   comment "a10disp needs a Linux kernel to be built"
>>         depends on !BR2_LINUX_KERNEL
>>
>>   endif
>>
>> that layout makes it far clearer that the entire option depends on
>> arm or you see *nothing* and, further, internally, the dependencies
>> in the comment list *only* those dependencies that the user will be
>> warned that they need if they want this selection.

 I completely agree.

>>
>>   i just think having the dependency line
>>
>>   depends on BR2_arm
>>
>> in both the config and comment directives is unnecessary duplication,
>> and that that kind of dependency should be moved up to encompass the
>> entire Config.in file, however that's best done.
>>
>>   thoughts?
> 
> Well, you are right that "it would make moere sense" from a theoretical
> point of view, and that there is no functional difference. BTW, there
> are other  such architectural options, like MMU, that we handle the same
> way as well.
> 
> Note however, we have more complex packages, like for example WebKit,
> for which we move the architectural dependencies into their own option,
> like so:
> 
>     config BR2_PACKAGE_WEBKIT_ARCH_SUPPORTS
>         bool
>         # ARM needs BLX, so v5t+
>         default y if (BR2_arm || BR2_armeb) && !BR2_ARM_CPU_ARMV4
>         default y if BR2_i386 || BR2_mips || BR2_mipsel || \
>                 BR2_sparc || BR2_x86_64
>         depends on BR2_USE_MMU # libgail -> pango -> libglib2
> 
> That's because this way, other packages that may want to select WebKit
> can select it by just adding a dependency on BR2_PACKAGE_WEBKIR_ARCH_SUPPORT.

 But even so, it would make more sense IMHO to write:

config BR2_PACKAGE_WEBKIT_ARCH_SUPPORTS
	...

if BR2_PACKAGE_WEBKIT_ARCH_SUPPORTS

comment "webkit needs foo bar baz"
	depends on !(BR2_FOO && BR2_BAR && BR2_BAZ)

config BR2_PACKAGE_WEBKIT
	depends on BR2_FOO
	...

...

endif


 Note that I also think it makes more sense to have the comment at the top then
at the bottom.


> 
> And of course, that's the way packages have been written historically.
> Changing all the packages is just not feasible, and maintainability and
> homogeneity trump eye-candy quite easily. ;-)

 But we don't have that much homogeneity at the moment. It is true that we
currently almost always have it as depends on, but the order and how it's || and
&&-ed varies.


> So yes, you are right _on principle_. But we're not gonna change that
> policy, and we'll continue to require new packages to conform to it.

 We _could_ change the policy and just require new packages to conform to the
new policy. We do that regularly (cfr. patch naming, BR2_ prefix, ...).

 That said, I don't think the current situation is bad enough to warrant such a
change.


> Just a side note: I personally find it easier to read the way we have it
> now: having the "depends on" directly in the package dependency list
> looks more obvious to me (but hey! I'm kind of a weirdo! ;-) 

 Well, then either your first statement that it makes more sense was not true,
or else you don't make sense :-P


 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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] a philosophical question about Config.in and "comment" directives

[Robert P. J. Day]

On Fri, 17 Apr 2015, Arnout Vandecappelle wrote:

> On 17/04/15 17:00, Yann E. MORIN wrote:
> > Rovert, All,
> >
> > On 2015-04-17 08:58 -0400, Robert P. J. Day spake thusly:
> > [--SNIP--]
> >>   to distinguish between these two "levels" of dependency, i think it
> >> would be far clearer to rewrite a file like that as:
> >>
> >>   if BR2_arm
> >>
> >>   config BR2_PACKAGE_A10DISP
> >>         bool "a10disp"
> >>         depends on BR2_LINUX_KERNEL
> >>         help
> >>           Program to change the display mode of Allwinner ARM SOCs running
> >>           the linux-sunxi kernel (and not the mainline kernel.)
> >>
> >>           http://github.com/hglm/a10disp
> >>
> >>   comment "a10disp needs a Linux kernel to be built"
> >>         depends on !BR2_LINUX_KERNEL
> >>
> >>   endif
> >>
> >> that layout makes it far clearer that the entire option depends on
> >> arm or you see *nothing* and, further, internally, the dependencies
> >> in the comment list *only* those dependencies that the user will be
> >> warned that they need if they want this selection.
>
>  I completely agree.
>
> >>
> >>   i just think having the dependency line
> >>
> >>   depends on BR2_arm
> >>
> >> in both the config and comment directives is unnecessary duplication,
> >> and that that kind of dependency should be moved up to encompass the
> >> entire Config.in file, however that's best done.
> >>
> >>   thoughts?
> >
> > Well, you are right that "it would make moere sense" from a theoretical
> > point of view, and that there is no functional difference.

  i know there's no functional difference, and that's why i'm quite
prepared for people to suggest it's just meaningless code churn.

> > BTW, there are other such architectural options, like MMU, that we
> > handle the same way as well.

  in fact, that was exactly the other example that jumped out at me --
the duplication of the test for BR2_USE_MMU in quite a number of
recipes. that's the test that got me thinking about this.

  anyway, just my $0.02 ...

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

[Buildroot] a philosophical question about Config.in and "comment" directives

[Yann E. MORIN]

Arnout, All,

On 2015-04-17 18:18 +0200, Arnout Vandecappelle spake thusly:
> On 17/04/15 17:00, Yann E. MORIN wrote:
> > Rovert, All,

Damn, my keyboard skills really need to be improved upon... :-/
Sorry, Robert...

> > On 2015-04-17 08:58 -0400, Robert P. J. Day spake thusly:
[--SNIP--]
> > Well, you are right that "it would make moere sense" from a theoretical
> > point of view, and that there is no functional difference. BTW, there
> > are other  such architectural options, like MMU, that we handle the same
> > way as well.

> > Just a side note: I personally find it easier to read the way we have it
> > now: having the "depends on" directly in the package dependency list
> > looks more obvious to me (but hey! I'm kind of a weirdo! ;-) 
> 
>  Well, then either your first statement that it makes more sense was not true,
> or else you don't make sense :-P

Well, I never I did make sense in my head! ;-)

Seriously: enclosing the whole file inside a big architectural
dependency does make sense at a technical level. But I don;t grok it as
easily as it is now.

I.e. a car is far safer than a motorbike from a technical point of view
(it has airbags and safety-belts, can not easily slide on its side with
driver stuck between it and the asphalt, is warm in winter and has A/C
in summer...). But I'll use my motorbike because I prefer it. ;-)

>  Note that I also think it makes more sense to have the comment at the top then
> at the bottom.

So do I. At least, we can agree on that! hehe! ;-)
 
Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] a philosophical question about Config.in and "comment" directives

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >>> that layout makes it far clearer that the entire option depends on
 >>> arm or you see *nothing* and, further, internally, the dependencies
 >>> in the comment list *only* those dependencies that the user will be
 >>> warned that they need if they want this selection.

 >  I completely agree.

Me too.

 >> So yes, you are right _on principle_. But we're not gonna change that
 >> policy, and we'll continue to require new packages to conform to it.

 >  We _could_ change the policy and just require new packages to conform to the
 > new policy. We do that regularly (cfr. patch naming, BR2_ prefix, ...).

 >  That said, I don't think the current situation is bad enough to
 > warrant such a change.

No indeed. I wouldn't mind new patches doing it like this, but I don't
want to se the churn to change our ~1500 existing packages to it.

-- 
Bye, Peter Korsgaard

[Buildroot] a philosophical question about Config.in and "comment" directives

[Robert P. J. Day]

On Mon, 20 Apr 2015, Peter Korsgaard wrote:

> >>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:
>
> Hi,
>
>  >>> that layout makes it far clearer that the entire option depends on
>  >>> arm or you see *nothing* and, further, internally, the dependencies
>  >>> in the comment list *only* those dependencies that the user will be
>  >>> warned that they need if they want this selection.
>
>  >  I completely agree.
>
> Me too.
>
>  >> So yes, you are right _on principle_. But we're not gonna change that
>  >> policy, and we'll continue to require new packages to conform to it.
>
>  >  We _could_ change the policy and just require new packages to conform to the
>  > new policy. We do that regularly (cfr. patch naming, BR2_ prefix, ...).
>
>  >  That said, I don't think the current situation is bad enough to
>  > warrant such a change.
>
> No indeed. I wouldn't mind new patches doing it like this, but I don't
> want to se the churn to change our ~1500 existing packages to it.

  i didn't mean to open such a can of standardization worms ... the
only reason i mentioned this is because the first time i saw that
structure, i was a bit confused as to why the very same dependency was
in both places.

  i also note that this is (i think) addressed in the user manual,
section 17.2.4:

http://nightly.buildroot.org/manual.html#_config_files

and i quote:

"Many packages depend on certain options of the toolchain: the choice
of C library, C++ support, thread support, RPC support, IPv6 support,
wchar support, or dynamic library support. Some packages can only be
built on certain target architectures, or if an MMU is available in
the processor.

"These dependencies have to be expressed with the appropriate depends
on statements in the Config.in file. Additionally, for dependencies on
toolchain options, a comment should be displayed when the option is
not enabled, so that the user knows why the package is not available.
Dependencies on target architecture or MMU support should not be made
visible in a comment: since it is unlikely that the user can freely
choose another target, it makes little sense to show these
dependencies explicitly.

"The comment should only be visible if the config option itself would
be visible when the toolchain option dependencies are met. This means
that all other dependencies of the package (including dependencies on
target architecture and MMU support) have to be repeated on the
comment definition. To keep it clear, the depends on statement for
these non-toolchain option should be kept separate from the depends on
statement for the toolchain options. If there is a dependency on a
config option in that same file (typically the main package) it is
preferable to have a global if ? endif construct rather than repeating
the depends on statement on the comment and other config options."

  so that last para does in fact describe the requirement for
*repeating* the dependency.

  anyway, i'll shut up about this now, and get back to reading.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================