diff mbox

[6/6] Makefile: do not add the toolchain target to the TARGETS variable

Message ID CAHkwnC8+MsDLUd5XMU1GDmfNBCVU0p9JTaUmpWiYReTrSsAnKQ@mail.gmail.com
State Superseded
Headers show

Commit Message

Fabio Porcedda March 19, 2014, 11:10 a.m. UTC
On Mon, Mar 17, 2014 at 8:42 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 03/11/14 13:17, Fabio Porcedda wrote:
>> Do not add the toolchain target to the TARGETS variable because now the
>> toolchain target is a dependency added automatically so it is not
>> necessary to add it anymore.
>
>  I disagree with this one. When you run make, you expect buildroot to
> build the toolchain even if no target package is selected. One use case I
> can think of: when preparing an internal toolchain for use as an external
> toolchain in later builds.

Take in account that the "toolchain" target is always built because is
a dependency of the "busybox" package that is always build and cannot
be disabled and
the "toolchain-buildroot" and "toolchain-external" are always enabled
in the Config.in.

I was thinking that the "toolchain" target is a package that uses tha
package infrastructure, what about using the Config.in:



I think this is more coherent to other always enabled packages like
busybox and toolchain-*.

Best regards

Comments

Arnout Vandecappelle March 19, 2014, 11:46 a.m. UTC | #1
On 19/03/14 12:10, Fabio Porcedda wrote:
> On Mon, Mar 17, 2014 at 8:42 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>> On 03/11/14 13:17, Fabio Porcedda wrote:
>>> Do not add the toolchain target to the TARGETS variable because now the
>>> toolchain target is a dependency added automatically so it is not
>>> necessary to add it anymore.
>>
>>  I disagree with this one. When you run make, you expect buildroot to
>> build the toolchain even if no target package is selected. One use case I
>> can think of: when preparing an internal toolchain for use as an external
>> toolchain in later builds.
> 
> Take in account that the "toolchain" target is always built because is
> a dependency of the "busybox" package that is always build and cannot
> be disabled and
> the "toolchain-buildroot" and "toolchain-external" are always enabled
> in the Config.in.

 busybox can be disabled (if you set the init system to "none").

 toolchain, toolchain-buildroot and toolchain-external are not added to
TARGETS because BR2_PACKAGE_TOOLCHAIN... is not defined.

> 
> I was thinking that the "toolchain" target is a package that uses tha
> package infrastructure, what about using the Config.in:
> 
> diff --git a/toolchain/Config.in b/toolchain/Config.in
> index 44f3ac6..94a0c7d 100644
> --- a/toolchain/Config.in
> +++ b/toolchain/Config.in
> @@ -1,5 +1,9 @@
>  menu "Toolchain"
> 
> +config BR2_TOOLCHAIN
> +       bool
> +       default y

 That would indeed be an option (it would have to be
BR2_PACKAGE_TOOLCHAIN, but that's a minor detail). I'm not sure, however,
if this is better than TARGETS := toolchain in the Makefile.


 Actually, I am in favour of adding blind options for everything that
uses the package infrastructure. That will allow us to do things like
checking select/DEPENDENCIES consistency, and generally makes things more
consistent.


 Regards,
 Arnout

> +
>  # Should be selected for glibc or eglibc
>  config BR2_TOOLCHAIN_USES_GLIBC
>         bool
> diff --git a/Makefile b/Makefile
> index 384693a..3df9144 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -299,7 +299,7 @@ GNU_HOST_NAME:=$(shell support/gnuconfig/config.guess)
>  #
>  ################################################################################
> 
> -TARGETS:= toolchain
> +TARGETS :=
> 
>  # silent mode requested?
>  QUIET:=$(if $(findstring s,$(MAKEFLAGS)),-q)
> 
> 
> I think this is more coherent to other always enabled packages like
> busybox and toolchain-*.
> 
> Best regards
>
Yann E. MORIN March 19, 2014, 6:35 p.m. UTC | #2
Fabio, Arnout, All,

On 2014-03-19 12:46 +0100, Arnout Vandecappelle spake thusly:
> On 19/03/14 12:10, Fabio Porcedda wrote:
[--SNIP--]
> > Take in account that the "toolchain" target is always built because is
> > a dependency of the "busybox" package that is always build and cannot
> > be disabled and
> > the "toolchain-buildroot" and "toolchain-external" are always enabled
> > in the Config.in.
> 
>  busybox can be disabled (if you set the init system to "none").

Indeed, and I very often uses that when I want to just test a single
package I'm adding.

Regards,
Yann E. MORIN.
Fabio Porcedda March 20, 2014, 9:38 a.m. UTC | #3
On Wed, Mar 19, 2014 at 12:46 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 19/03/14 12:10, Fabio Porcedda wrote:
>> On Mon, Mar 17, 2014 at 8:42 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>> On 03/11/14 13:17, Fabio Porcedda wrote:
>>>> Do not add the toolchain target to the TARGETS variable because now the
>>>> toolchain target is a dependency added automatically so it is not
>>>> necessary to add it anymore.
>>>
>>>  I disagree with this one. When you run make, you expect buildroot to
>>> build the toolchain even if no target package is selected. One use case I
>>> can think of: when preparing an internal toolchain for use as an external
>>> toolchain in later builds.
>>
>> Take in account that the "toolchain" target is always built because is
>> a dependency of the "busybox" package that is always build and cannot
>> be disabled and
>> the "toolchain-buildroot" and "toolchain-external" are always enabled
>> in the Config.in.
>
>  busybox can be disabled (if you set the init system to "none").

I didn't know that, great!

>  toolchain, toolchain-buildroot and toolchain-external are not added to
> TARGETS because BR2_PACKAGE_TOOLCHAIN... is not defined.

They are added to targets, just use this command to check it:

$ make defconfig && make show-targets

toolchain-buildroot toolchain busybox target-generic-securetty
target-generic-hostname target-generic-issue target-root-passwd
target-generic-getty-busybox target-generic-do-remount-rw rootfs-tar

>>
>> I was thinking that the "toolchain" target is a package that uses tha
>> package infrastructure, what about using the Config.in:
>>
>> diff --git a/toolchain/Config.in b/toolchain/Config.in
>> index 44f3ac6..94a0c7d 100644
>> --- a/toolchain/Config.in
>> +++ b/toolchain/Config.in
>> @@ -1,5 +1,9 @@
>>  menu "Toolchain"
>>
>> +config BR2_TOOLCHAIN
>> +       bool
>> +       default y
>
>  That would indeed be an option (it would have to be
> BR2_PACKAGE_TOOLCHAIN, but that's a minor detail). I'm not sure, however,
> if this is better than TARGETS := toolchain in the Makefile.

I've not used the PACKAGE prefix because it's not used at all for the
toolchain config options, like the  BR2_TOOLCHAIN_BUILDROOT option.

>
>  Actually, I am in favour of adding blind options for everything that
> uses the package infrastructure. That will allow us to do things like
> checking select/DEPENDENCIES consistency, and generally makes things more
> consistent.

In next revision of the patch i will send the proposed patch.

Thanks and regards
Fabio Porcedda March 20, 2014, 10:01 a.m. UTC | #4
On Wed, Mar 19, 2014 at 7:35 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Fabio, Arnout, All,
>
> On 2014-03-19 12:46 +0100, Arnout Vandecappelle spake thusly:
>> On 19/03/14 12:10, Fabio Porcedda wrote:
> [--SNIP--]
>> > Take in account that the "toolchain" target is always built because is
>> > a dependency of the "busybox" package that is always build and cannot
>> > be disabled and
>> > the "toolchain-buildroot" and "toolchain-external" are always enabled
>> > in the Config.in.
>>
>>  busybox can be disabled (if you set the init system to "none").
>
> Indeed, and I very often uses that when I want to just test a single
> package I'm adding.

Good idea, next time i will test a package i will do that.

Thanks
Arnout Vandecappelle March 20, 2014, 10:13 a.m. UTC | #5
On 20/03/14 10:38, Fabio Porcedda wrote:
> On Wed, Mar 19, 2014 at 12:46 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
[snip]
>>  toolchain, toolchain-buildroot and toolchain-external are not added to
>> TARGETS because BR2_PACKAGE_TOOLCHAIN... is not defined.
> 
> They are added to targets, just use this command to check it:
> 
> $ make defconfig && make show-targets
> 
> toolchain-buildroot toolchain busybox target-generic-securetty
> target-generic-hostname target-generic-issue target-root-passwd
> target-generic-getty-busybox target-generic-do-remount-rw rootfs-tar
> 
>>>
>>> I was thinking that the "toolchain" target is a package that uses tha
>>> package infrastructure, what about using the Config.in:
>>>
>>> diff --git a/toolchain/Config.in b/toolchain/Config.in
>>> index 44f3ac6..94a0c7d 100644
>>> --- a/toolchain/Config.in
>>> +++ b/toolchain/Config.in
>>> @@ -1,5 +1,9 @@
>>>  menu "Toolchain"
>>>
>>> +config BR2_TOOLCHAIN
>>> +       bool
>>> +       default y
>>
>>  That would indeed be an option (it would have to be
>> BR2_PACKAGE_TOOLCHAIN, but that's a minor detail). I'm not sure, however,
>> if this is better than TARGETS := toolchain in the Makefile.
> 
> I've not used the PACKAGE prefix because it's not used at all for the
> toolchain config options, like the  BR2_TOOLCHAIN_BUILDROOT option.

 Yeah, I forgot about this condition in pkg-generic.mk:

else ifneq ($(filter toolchain/%,$(pkgdir)),)
$(2)_KCONFIG_VAR = BR2_$(2)

> 
>>
>>  Actually, I am in favour of adding blind options for everything that
>> uses the package infrastructure. That will allow us to do things like
>> checking select/DEPENDENCIES consistency, and generally makes things more
>> consistent.
> 
> In next revision of the patch i will send the proposed patch.

 Can you mention in the commit log this second advantage?

 But first, Peter, (and other buildroot maintainers as well), do you
agree that it would be a good idea to have blind Kconfig symbols for
everything that uses the package infrastructure?

 Regards,
 Arnout
diff mbox

Patch

diff --git a/toolchain/Config.in b/toolchain/Config.in
index 44f3ac6..94a0c7d 100644
--- a/toolchain/Config.in
+++ b/toolchain/Config.in
@@ -1,5 +1,9 @@ 
 menu "Toolchain"

+config BR2_TOOLCHAIN
+       bool
+       default y
+
 # Should be selected for glibc or eglibc
 config BR2_TOOLCHAIN_USES_GLIBC
        bool
diff --git a/Makefile b/Makefile
index 384693a..3df9144 100644
--- a/Makefile
+++ b/Makefile
@@ -299,7 +299,7 @@  GNU_HOST_NAME:=$(shell support/gnuconfig/config.guess)
 #
 ################################################################################

-TARGETS:= toolchain
+TARGETS :=

 # silent mode requested?
 QUIET:=$(if $(findstring s,$(MAKEFLAGS)),-q)