diff mbox

[5,of,7,v3] toolchain-external: change version from 'undefined' to 'virtual'

Message ID 640c5d9c5ac14d442207.1402085581@localhost
State Superseded
Headers show

Commit Message

Thomas De Schampheleire June 6, 2014, 8:13 p.m. UTC 
The toolchain-external package displays the version 'undefined' in the build
messages and the directory in output/build, which is not very nice.
This patch sets the version to 'virtual', in analogy to the toolchain and
toolchain-buildroot packages (which use the virtual-package infrastructure).

Although toolchain-external is not strictly a virtual package, since it uses
the generic-package infrastructure, it can be considered as a virtual
package in the sense that it does not have a fixed version or source (they
depend on the selected external toolchain).

Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>
Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

---
v3: no changes
v2: no changes

 toolchain/toolchain-external/toolchain-external.mk |  2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Comments

Thomas Petazzoni June 7, 2014, 8:57 a.m. UTC | #1 
Dear Thomas De Schampheleire,

On Fri, 06 Jun 2014 22:13:01 +0200, Thomas De Schampheleire wrote:
> The toolchain-external package displays the version 'undefined' in the build
> messages and the directory in output/build, which is not very nice.
> This patch sets the version to 'virtual', in analogy to the toolchain and
> toolchain-buildroot packages (which use the virtual-package infrastructure).
> 
> Although toolchain-external is not strictly a virtual package, since it uses
> the generic-package infrastructure, it can be considered as a virtual
> package in the sense that it does not have a fixed version or source (they
> depend on the selected external toolchain).
> 
> Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>
> Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

I'm sorry, but I continue to disagree. toolchain-external is definitely
*not* a virtual package. I've started using the
TOOLCHAIN_EXTERNAL_VERSION field for musl external toolchains, and I
will send a patch that use it for other toolchains as well.

Thomas
Thomas De Schampheleire June 8, 2014, 3:04 p.m. UTC | #2 
Hi Thomas,

On Sat, Jun 7, 2014 at 10:57 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Thomas De Schampheleire,
>
> On Fri, 06 Jun 2014 22:13:01 +0200, Thomas De Schampheleire wrote:
>> The toolchain-external package displays the version 'undefined' in the build
>> messages and the directory in output/build, which is not very nice.
>> This patch sets the version to 'virtual', in analogy to the toolchain and
>> toolchain-buildroot packages (which use the virtual-package infrastructure).
>>
>> Although toolchain-external is not strictly a virtual package, since it uses
>> the generic-package infrastructure, it can be considered as a virtual
>> package in the sense that it does not have a fixed version or source (they
>> depend on the selected external toolchain).
>>
>> Signed-off-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>
>> Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> I'm sorry, but I continue to disagree. toolchain-external is definitely
> *not* a virtual package. I've started using the
> TOOLCHAIN_EXTERNAL_VERSION field for musl external toolchains, and I
> will send a patch that use it for other toolchains as well.

Ok, I understand your objections. Note that my main point is that it
shouldn't be 'undefined', not that it necessarily has to be 'virtual'.

However, in your pending musl patch, you set
TOOLCHAIN_EXTERNAL_VERSION to 1.0.0 (for example), causing the 'build'
directory to be output/build/toolchain-external-1.0.0 and the messages
to be:
toolchain-external 1.0.0 Downloading

The value 1.0.0 doesn't mean a lot here, you have no clue that this is
a musl, glibc, ... toolchain or which is the provider. Considering
only the messages for a moment, it would make more sense that they
would read:

toolchain-external Sourcery ARM 2012.03 Downloading

(for example), i.e. actually specify which is the external toolchain
we're using.

This could be achieved in multiple ways:
- by setting the TOOLCHAIN_EXTERNAL_VERSION field to this value
(imposing the requirement of it not containing spaces as this value is
also used for the directory name).
- adding an extra field in the MESSAGE definition that can be set in
the toolchain-external package, with 'extra' info. The VERSION field
can then still be 1.0.0 or 2012.03, and the extra info would be
'Sourcery ARM' for example.

The second approach handles both my concern of not having 'undefined'
and is in line with your usage of 1.0.0 for VERSION.

What do you think?

Best regards,
Thomas
Thomas Petazzoni June 8, 2014, 4:10 p.m. UTC | #3 
Dear Thomas De Schampheleire,

On Sun, 8 Jun 2014 17:04:17 +0200, Thomas De Schampheleire wrote:

> > I'm sorry, but I continue to disagree. toolchain-external is definitely
> > *not* a virtual package. I've started using the
> > TOOLCHAIN_EXTERNAL_VERSION field for musl external toolchains, and I
> > will send a patch that use it for other toolchains as well.
> 
> Ok, I understand your objections. Note that my main point is that it
> shouldn't be 'undefined', not that it necessarily has to be 'virtual'.
> 
> However, in your pending musl patch, you set
> TOOLCHAIN_EXTERNAL_VERSION to 1.0.0 (for example), causing the 'build'
> directory to be output/build/toolchain-external-1.0.0 and the messages
> to be:
> toolchain-external 1.0.0 Downloading
> 
> The value 1.0.0 doesn't mean a lot here, you have no clue that this is
> a musl, glibc, ... toolchain or which is the provider. Considering
> only the messages for a moment, it would make more sense that they
> would read:
> 
> toolchain-external Sourcery ARM 2012.03 Downloading
> 
> (for example), i.e. actually specify which is the external toolchain
> we're using.
> 
> This could be achieved in multiple ways:
> - by setting the TOOLCHAIN_EXTERNAL_VERSION field to this value
> (imposing the requirement of it not containing spaces as this value is
> also used for the directory name).
> - adding an extra field in the MESSAGE definition that can be set in
> the toolchain-external package, with 'extra' info. The VERSION field
> can then still be 1.0.0 or 2012.03, and the extra info would be
> 'Sourcery ARM' for example.
> 
> The second approach handles both my concern of not having 'undefined'
> and is in line with your usage of 1.0.0 for VERSION.
> 
> What do you think?

While your second solution adds quite a bit of additional logic in
MESSAGE just for the sake of toolchain-external, I don't really have a
better suggestion right now. Or should we turn all these external
toolchains into individual packages, which become providers for what
would really become a virtual toolchain-external package? I'm not sure
of the benefit, though.

Best regards,

Thomas
Thomas De Schampheleire June 8, 2014, 5:23 p.m. UTC | #4 
On Sun, Jun 8, 2014 at 6:10 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Thomas De Schampheleire,
>
> On Sun, 8 Jun 2014 17:04:17 +0200, Thomas De Schampheleire wrote:
>
>> > I'm sorry, but I continue to disagree. toolchain-external is definitely
>> > *not* a virtual package. I've started using the
>> > TOOLCHAIN_EXTERNAL_VERSION field for musl external toolchains, and I
>> > will send a patch that use it for other toolchains as well.
>>
>> Ok, I understand your objections. Note that my main point is that it
>> shouldn't be 'undefined', not that it necessarily has to be 'virtual'.
>>
>> However, in your pending musl patch, you set
>> TOOLCHAIN_EXTERNAL_VERSION to 1.0.0 (for example), causing the 'build'
>> directory to be output/build/toolchain-external-1.0.0 and the messages
>> to be:
>> toolchain-external 1.0.0 Downloading
>>
>> The value 1.0.0 doesn't mean a lot here, you have no clue that this is
>> a musl, glibc, ... toolchain or which is the provider. Considering
>> only the messages for a moment, it would make more sense that they
>> would read:
>>
>> toolchain-external Sourcery ARM 2012.03 Downloading
>>
>> (for example), i.e. actually specify which is the external toolchain
>> we're using.
>>
>> This could be achieved in multiple ways:
>> - by setting the TOOLCHAIN_EXTERNAL_VERSION field to this value
>> (imposing the requirement of it not containing spaces as this value is
>> also used for the directory name).
>> - adding an extra field in the MESSAGE definition that can be set in
>> the toolchain-external package, with 'extra' info. The VERSION field
>> can then still be 1.0.0 or 2012.03, and the extra info would be
>> 'Sourcery ARM' for example.
>>
>> The second approach handles both my concern of not having 'undefined'
>> and is in line with your usage of 1.0.0 for VERSION.
>>
>> What do you think?
>
> While your second solution adds quite a bit of additional logic in
> MESSAGE just for the sake of toolchain-external, I don't really have a
> better suggestion right now. Or should we turn all these external
> toolchains into individual packages, which become providers for what
> would really become a virtual toolchain-external package? I'm not sure
> of the benefit, though.

I also think the second approach of creating individual external
toolchain packages is overkill.

I don't think that the MESSAGE solution adds a lot of complexity: the
current definition is:
MESSAGE     = echo "$(TERM_BOLD)>>> $($(PKG)_NAME) $($(PKG)_VERSION)
$(1)$(TERM_RESET)"
and this would become something like:
MESSAGE     = echo "$(TERM_BOLD)>>> $($(PKG)_NAME)
$($(PKG)_EXTRA_NAME) $($(PKG)_VERSION) $(1)$(TERM_RESET)"

where FOO_EXTRA_NAME is a new variabel (name to be discussed) that can
be set by any package (not only toolchain-external) if appropriate.
This variable is default empty.

This is not too complex, right?

In any case, I suggest we drop this patch from the series for now.

Best regards,
Thomas
Arnout Vandecappelle June 16, 2014, 5:17 a.m. UTC | #5 
On 06/08/14 19:23, Thomas De Schampheleire wrote:
[snip]
> I don't think that the MESSAGE solution adds a lot of complexity: the
> current definition is:
> MESSAGE     = echo "$(TERM_BOLD)>>> $($(PKG)_NAME) $($(PKG)_VERSION)
> $(1)$(TERM_RESET)"
> and this would become something like:
> MESSAGE     = echo "$(TERM_BOLD)>>> $($(PKG)_NAME)
> $($(PKG)_EXTRA_NAME) $($(PKG)_VERSION) $(1)$(TERM_RESET)"

 Small fix: one of the spaces around $(PKG)_EXTRA_NAME should be removed.
Otherwise we have two spaces in the usual case. The extra name should then be
defined with an embedded space:

TOOLCHAIN_EXTERNAL_EXTRA_NAME = Sourcery$(space)


 Regards,
 Arnout

> 
> where FOO_EXTRA_NAME is a new variabel (name to be discussed) that can
> be set by any package (not only toolchain-external) if appropriate.
> This variable is default empty.
> 
> This is not too complex, right?
> 
> In any case, I suggest we drop this patch from the series for now.
> 
> Best regards,
> Thomas
>
Thomas De Schampheleire June 16, 2014, 6:54 a.m. UTC | #6 
Hi Arnout,

On Mon, Jun 16, 2014 at 7:17 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 06/08/14 19:23, Thomas De Schampheleire wrote:
> [snip]
>> I don't think that the MESSAGE solution adds a lot of complexity: the
>> current definition is:
>> MESSAGE     = echo "$(TERM_BOLD)>>> $($(PKG)_NAME) $($(PKG)_VERSION)
>> $(1)$(TERM_RESET)"
>> and this would become something like:
>> MESSAGE     = echo "$(TERM_BOLD)>>> $($(PKG)_NAME)
>> $($(PKG)_EXTRA_NAME) $($(PKG)_VERSION) $(1)$(TERM_RESET)"
>
>  Small fix: one of the spaces around $(PKG)_EXTRA_NAME should be removed.
> Otherwise we have two spaces in the usual case. The extra name should then be
> defined with an embedded space:
>
> TOOLCHAIN_EXTERNAL_EXTRA_NAME = Sourcery$(space)

Yeah, the spacing of the MESSAGE is not very nice currently: for
non-package messages (like 'Finalizing target') there are already two
leading spaces due to PKG_NAME and PKG_VERSION being empty. Maybe we
should do something like:
MESSAGE = $(subst $(space)$(space),$(space),XXX)

or alternatively compose MESSAGE step by step and first checking each
variable for emptyness, something like:
MESSAGE = echo "$(TERM_BOLD)>>>"
ifneq (,$($(PKG)_NAME))
MESSAGE += $(space)$($(PKG)_NAME)
endif
ifneq (,$($(PKG)_VERSION))
MESSAGE += $(space)$($(PKG)_VERSION)
endif

etc.

Best regards,
Thomas
Thomas Petazzoni June 16, 2014, 7:18 a.m. UTC | #7 
Dear Thomas De Schampheleire,

On Sun, 8 Jun 2014 19:23:09 +0200, Thomas De Schampheleire wrote:

> I also think the second approach of creating individual external
> toolchain packages is overkill.

I actually don't know. The current toolchain-external/Config.in and
toolchain-external/toolchain-external.mk are very long, splitting them
wouldn't be that bad. I'd like to think a bit more about this, and see
if a solution with individual packages wouldn't actually be better.

For example, one thing that the external toolchain stuff doesn't handle
today is fetching the source code for the toolchain.

> I don't think that the MESSAGE solution adds a lot of complexity: the
> current definition is:
> MESSAGE     = echo "$(TERM_BOLD)>>> $($(PKG)_NAME) $($(PKG)_VERSION)
> $(1)$(TERM_RESET)"
> and this would become something like:
> MESSAGE     = echo "$(TERM_BOLD)>>> $($(PKG)_NAME)
> $($(PKG)_EXTRA_NAME) $($(PKG)_VERSION) $(1)$(TERM_RESET)"
> 
> where FOO_EXTRA_NAME is a new variabel (name to be discussed) that can
> be set by any package (not only toolchain-external) if appropriate.
> This variable is default empty.
> 
> This is not too complex, right?

It's not really whether it's complex or not: it's a feature that is
added to the core, common infrastructure, to solve the problem of just
one package. If we do this for each and every need of each single
package, the infrastructure is going to become an awful pile of crap,
and that's what I'd like to avoid.

Thomas
diff mbox

Patch

diff -r 58ea2200cf19 -r 640c5d9c5ac1 toolchain/toolchain-external/toolchain-external.mk
--- a/toolchain/toolchain-external/toolchain-external.mk	Tue May 06 09:36:14 2014 +0200
+++ b/toolchain/toolchain-external/toolchain-external.mk	Sun May 11 14:28:02 2014 +0200
@@ -411,6 +411,8 @@ 
 TOOLCHAIN_EXTERNAL_SOURCE =
 endif
 
+TOOLCHAIN_EXTERNAL_VERSION = virtual
+
 TOOLCHAIN_EXTERNAL_ADD_TOOLCHAIN_DEPENDENCY = NO
 
 TOOLCHAIN_EXTERNAL_INSTALL_STAGING = YES