toolchain package: set version as 'virtual' instead of 'undefined'

Hi,

On Wed, May 7, 2014 at 9:37 PM, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
[..]

>>> I tried using virtual-package in toolchain/toolchain/toolchain.mk, but TOOLCHAIN_VERSION becomes empty, while HOST_TOOLCHAIN_VERSION is virtual.

[..]

Some more feedback: I stepped away from toolchain and added a simple
test package 'mytest' as below.
I added a debug target 'bar' and some variable assignment in pkg-generic.mk.




In pkg-generic.mk, the _VERSION variables are defined, and there is a
subst() statement to replace / with _.
My conclusion is that the line with the subst statement does not work
when _VERSION is set like in virtual-package.

The output of 'make bar' is:
MYTEST_VERSION =
HOST_MYTEST_VERSION = virtual
MYTEST_X =
MYTEST_Y = virtual
HOST_MYTEST_X =
HOST_MYTEST_Y =

X is using the standard subst line as pkg-generic is using, while Y
adds an extra dollar sign.
When the version is set directly, as in MYTEST_VERSION = bla, then X
seems to be correct. This is the way buildroot works today. However,
when the version is set indirectly from an inner-xxx-package, like
$(3)_VERSION, then X no longer works but Y does. However, adding an
extra dollar by default doesn't work for the normal packages.

So how to proceed? Any experts in the inner-xxx-package and make internals?
Arnout, ThomasP, Yann?

Thanks,
Thomas

On 08/05/14 09:04, Thomas De Schampheleire wrote:
> Hi,
> 
> On Wed, May 7, 2014 at 9:37 PM, Thomas De Schampheleire
> <patrickdepinguin@gmail.com> wrote:
> [..]
> 
>>>> I tried using virtual-package in toolchain/toolchain/toolchain.mk, but TOOLCHAIN_VERSION becomes empty, while HOST_TOOLCHAIN_VERSION is virtual.
> 
> [..]
> 
> Some more feedback: I stepped away from toolchain and added a simple
> test package 'mytest' as below.
> I added a debug target 'bar' and some variable assignment in pkg-generic.mk.
> 
> 
> diff --git a/package/mytest/mytest.mk b/package/mytest/mytest.mk
> new file mode 100644
> --- /dev/null
> +++ b/package/mytest/mytest.mk
> @@ -0,0 +1,10 @@
> +
> +$(eval $(virtual-package))
> +
> +bar:
> +       @echo MYTEST_VERSION = $(MYTEST_VERSION)
> +       @echo HOST_MYTEST_VERSION = $(HOST_MYTEST_VERSION)
> +       @echo MYTEST_X = $(MYTEST_X)
> +       @echo MYTEST_Y = $(MYTEST_Y)
> +       @echo HOST_MYTEST_X = $(HOST_MYTEST_X)
> +       @echo HOST_MYTEST_Y = $(HOST_MYTEST_Y)
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -262,6 +262,12 @@ define inner-generic-package
>  # sanitize the package version that is used in paths, directory and file names.
>  # Forward slashes may appear in the package's version when pointing to a
>  # version control system branch or tag, for example remotes/origin/1_10_stable.
> +
> +ifdef $(2)_VERSION
> +  $(2)_X := $(subst /,_,$($(2)_VERSION))
> +  $(2)_Y := $(subst /,_,$$($(2)_VERSION))
> +endif
> +
>  ifndef $(2)_VERSION
>   ifdef $(3)_VERSION
>    $(2)_DL_VERSION = $($(3)_VERSION)
> 
> 
> In pkg-generic.mk, the _VERSION variables are defined, and there is a
> subst() statement to replace / with _.
> My conclusion is that the line with the subst statement does not work
> when _VERSION is set like in virtual-package.
> 
> The output of 'make bar' is:
> MYTEST_VERSION =
> HOST_MYTEST_VERSION = virtual
> MYTEST_X =
> MYTEST_Y = virtual
> HOST_MYTEST_X =
> HOST_MYTEST_Y =
> 
> X is using the standard subst line as pkg-generic is using, while Y
> adds an extra dollar sign.
> When the version is set directly, as in MYTEST_VERSION = bla, then X
> seems to be correct. This is the way buildroot works today. However,
> when the version is set indirectly from an inner-xxx-package, like
> $(3)_VERSION, then X no longer works but Y does. However, adding an
> extra dollar by default doesn't work for the normal packages.
> 
> So how to proceed? Any experts in the inner-xxx-package and make internals?
> Arnout, ThomasP, Yann?

 Let me see if I can analyse it.

$(eval $(virtual-package))

recursively expands to:

$(eval
...
MYTEST_VERSION = virtual
HOST_MYTEST_VERSION = virtual
...

# expansion of inner-generic-package
# ** At this point MYTEST_VERSION is not set yet! **
ifdef MYTEST_VERSION
  MYTEST_X :=
  MYTEST_Y := $$(MYTEST_VERSION)
endif

ifndef MYTEST_VERSION
 ifdef MYTEST_VERSION
  MYTEST_DL_VERSION =
  MYTEST_VERSION =
 else
  MYTEST_VERSION = undefined
  MYTEST_DL_VERSION = undefined
 endif
else
  MYTEST_DL_VERSION =
  MYTEST_VERSION =
endif
...
) #eval



 I repeat my earlier statement: within a function that is supposed to be
eval'ed, *everything* should be $$'ed except the function arguments and except
some very specific cases.


 Regards,
 Arnout

Hi Arnout,

On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 08/05/14 09:04, Thomas De Schampheleire wrote:
>> Hi,
>>
>> On Wed, May 7, 2014 at 9:37 PM, Thomas De Schampheleire
>> <patrickdepinguin@gmail.com> wrote:
>> [..]
>>
>>>>> I tried using virtual-package in toolchain/toolchain/toolchain.mk, but TOOLCHAIN_VERSION becomes empty, while HOST_TOOLCHAIN_VERSION is virtual.
>>
>> [..]
>>
>> Some more feedback: I stepped away from toolchain and added a simple
>> test package 'mytest' as below.
>> I added a debug target 'bar' and some variable assignment in pkg-generic.mk.
>>
>>
>> diff --git a/package/mytest/mytest.mk b/package/mytest/mytest.mk
>> new file mode 100644
>> --- /dev/null
>> +++ b/package/mytest/mytest.mk
>> @@ -0,0 +1,10 @@
>> +
>> +$(eval $(virtual-package))
>> +
>> +bar:
>> +       @echo MYTEST_VERSION = $(MYTEST_VERSION)
>> +       @echo HOST_MYTEST_VERSION = $(HOST_MYTEST_VERSION)
>> +       @echo MYTEST_X = $(MYTEST_X)
>> +       @echo MYTEST_Y = $(MYTEST_Y)
>> +       @echo HOST_MYTEST_X = $(HOST_MYTEST_X)
>> +       @echo HOST_MYTEST_Y = $(HOST_MYTEST_Y)
>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>> --- a/package/pkg-generic.mk
>> +++ b/package/pkg-generic.mk
>> @@ -262,6 +262,12 @@ define inner-generic-package
>>  # sanitize the package version that is used in paths, directory and file names.
>>  # Forward slashes may appear in the package's version when pointing to a
>>  # version control system branch or tag, for example remotes/origin/1_10_stable.
>> +
>> +ifdef $(2)_VERSION
>> +  $(2)_X := $(subst /,_,$($(2)_VERSION))
>> +  $(2)_Y := $(subst /,_,$$($(2)_VERSION))
>> +endif
>> +
>>  ifndef $(2)_VERSION
>>   ifdef $(3)_VERSION
>>    $(2)_DL_VERSION = $($(3)_VERSION)
>>
>>
>> In pkg-generic.mk, the _VERSION variables are defined, and there is a
>> subst() statement to replace / with _.
>> My conclusion is that the line with the subst statement does not work
>> when _VERSION is set like in virtual-package.
>>
>> The output of 'make bar' is:
>> MYTEST_VERSION =
>> HOST_MYTEST_VERSION = virtual
>> MYTEST_X =
>> MYTEST_Y = virtual
>> HOST_MYTEST_X =
>> HOST_MYTEST_Y =
>>
>> X is using the standard subst line as pkg-generic is using, while Y
>> adds an extra dollar sign.
>> When the version is set directly, as in MYTEST_VERSION = bla, then X
>> seems to be correct. This is the way buildroot works today. However,
>> when the version is set indirectly from an inner-xxx-package, like
>> $(3)_VERSION, then X no longer works but Y does. However, adding an
>> extra dollar by default doesn't work for the normal packages.
>>
>> So how to proceed? Any experts in the inner-xxx-package and make internals?
>> Arnout, ThomasP, Yann?
>
>  Let me see if I can analyse it.
>
> $(eval $(virtual-package))
>
> recursively expands to:
>
> $(eval
> ...
> MYTEST_VERSION = virtual
> HOST_MYTEST_VERSION = virtual
> ...
>
> # expansion of inner-generic-package
> # ** At this point MYTEST_VERSION is not set yet! **
> ifdef MYTEST_VERSION
>   MYTEST_X :=
>   MYTEST_Y := $$(MYTEST_VERSION)
> endif
>
> ifndef MYTEST_VERSION
>  ifdef MYTEST_VERSION
>   MYTEST_DL_VERSION =
>   MYTEST_VERSION =
>  else
>   MYTEST_VERSION = undefined
>   MYTEST_DL_VERSION = undefined
>  endif
> else
>   MYTEST_DL_VERSION =
>   MYTEST_VERSION =
> endif
> ...
> ) #eval
>
>
>
>  I repeat my earlier statement: within a function that is supposed to be
> eval'ed, *everything* should be $$'ed except the function arguments and except
> some very specific cases.
>

But in the case of host variables being set based on target variables
if there is no explicit host variable, as for the _VERSION, there
would be a recursive assignment as $(2) == $(3).
So statement   $(2)_VERSION = $$(subst /,_,$($(3)_VERSION))
should become   $(2)_VERSION := $$(subst /,_,$($(3)_VERSION))
then, right?
I haven't tested whether this works though...

Best regards,
Thomas

On 08/05/14 11:28, Thomas De Schampheleire wrote:
> Hi Arnout,
> 
> On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <arnout@mind.be> wrote:

[snip]
>>  I repeat my earlier statement: within a function that is supposed to be
>> eval'ed, *everything* should be $$'ed except the function arguments and except
>> some very specific cases.
>>
> 
> But in the case of host variables being set based on target variables
> if there is no explicit host variable, as for the _VERSION, there
> would be a recursive assignment as $(2) == $(3).

True; if you set
$(2)_VERSION = $$(subst /,_,$$($(3)_VERSION))

that will expand to
PKG_VERSION = $$(subst /,_,$$(PKG_VERSION))

in the eval, which is indeed recursive.

But that can be solved with a simple:

$(2)_VERSION := $$(subst /,_,$$($(3)_VERSION))

Fortunately, make is smart enough to break the recursion even if PKG_VERSION had
been assigned with = before.

> So statement   $(2)_VERSION = $$(subst /,_,$($(3)_VERSION))
> should become   $(2)_VERSION := $$(subst /,_,$($(3)_VERSION))

 No, that doesn't make a difference, because the $(MYPKG_VERSION) is still
expanded too early.


 Regards,
 Arnout

> then, right?
> I haven't tested whether this works though...
> 
> Best regards,
> Thomas
>

On Thu, May 8, 2014 at 11:52 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 08/05/14 11:28, Thomas De Schampheleire wrote:
>> Hi Arnout,
>>
>> On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>
> [snip]
>>>  I repeat my earlier statement: within a function that is supposed to be
>>> eval'ed, *everything* should be $$'ed except the function arguments and except
>>> some very specific cases.
>>>
>>
>> But in the case of host variables being set based on target variables
>> if there is no explicit host variable, as for the _VERSION, there
>> would be a recursive assignment as $(2) == $(3).
>
> True; if you set
> $(2)_VERSION = $$(subst /,_,$$($(3)_VERSION))
>
> that will expand to
> PKG_VERSION = $$(subst /,_,$$(PKG_VERSION))
>
> in the eval, which is indeed recursive.
>
> But that can be solved with a simple:
>
> $(2)_VERSION := $$(subst /,_,$$($(3)_VERSION))
>
> Fortunately, make is smart enough to break the recursion even if PKG_VERSION had
> been assigned with = before.
>
>> So statement   $(2)_VERSION = $$(subst /,_,$($(3)_VERSION))
>> should become   $(2)_VERSION := $$(subst /,_,$($(3)_VERSION))
>
>  No, that doesn't make a difference, because the $(MYPKG_VERSION) is still
> expanded too early.
>

I'm pretty sure I said this before, but all these $$$$'s are making me dizzy...
:-D

I will do a test with your suggestion, thanks!

Best regards,
Thomas

On 08/05/14 13:37, Thomas De Schampheleire wrote:
> On Thu, May 8, 2014 at 11:52 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>> On 08/05/14 11:28, Thomas De Schampheleire wrote:
>>> Hi Arnout,
>>>
>>> On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>
>> [snip]
>>>>  I repeat my earlier statement: within a function that is supposed to be
>>>> eval'ed, *everything* should be $$'ed except the function arguments and except
>>>> some very specific cases.
>>>>
>>>
>>> But in the case of host variables being set based on target variables
>>> if there is no explicit host variable, as for the _VERSION, there
>>> would be a recursive assignment as $(2) == $(3).
>>
>> True; if you set
>> $(2)_VERSION = $$(subst /,_,$$($(3)_VERSION))
>>
>> that will expand to
>> PKG_VERSION = $$(subst /,_,$$(PKG_VERSION))
>>
>> in the eval, which is indeed recursive.
>>
>> But that can be solved with a simple:
>>
>> $(2)_VERSION := $$(subst /,_,$$($(3)_VERSION))
>>
>> Fortunately, make is smart enough to break the recursion even if PKG_VERSION had
>> been assigned with = before.
>>
>>> So statement   $(2)_VERSION = $$(subst /,_,$($(3)_VERSION))
>>> should become   $(2)_VERSION := $$(subst /,_,$($(3)_VERSION))
>>
>>  No, that doesn't make a difference, because the $(MYPKG_VERSION) is still
>> expanded too early.
>>
> 
> I'm pretty sure I said this before, but all these $$$$'s are making me dizzy...
> :-D

 There shouldn't be a need for 4 $$$$'s. The ones in pkg-autotools could be avoided.

 Regards,
 Arnout

> 
> I will do a test with your suggestion, thanks!
> 
> Best regards,
> Thomas
>

On Thu, May 8, 2014 at 3:00 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 08/05/14 13:37, Thomas De Schampheleire wrote:
>> On Thu, May 8, 2014 at 11:52 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>> On 08/05/14 11:28, Thomas De Schampheleire wrote:
>>>> Hi Arnout,
>>>>
>>>> On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>>
>>> [snip]
>>>>>  I repeat my earlier statement: within a function that is supposed to be
>>>>> eval'ed, *everything* should be $$'ed except the function arguments and except
>>>>> some very specific cases.
>>>>>
>>>>
>>>> But in the case of host variables being set based on target variables
>>>> if there is no explicit host variable, as for the _VERSION, there
>>>> would be a recursive assignment as $(2) == $(3).
>>>
>>> True; if you set
>>> $(2)_VERSION = $$(subst /,_,$$($(3)_VERSION))
>>>
>>> that will expand to
>>> PKG_VERSION = $$(subst /,_,$$(PKG_VERSION))
>>>
>>> in the eval, which is indeed recursive.
>>>
>>> But that can be solved with a simple:
>>>
>>> $(2)_VERSION := $$(subst /,_,$$($(3)_VERSION))
>>>
>>> Fortunately, make is smart enough to break the recursion even if PKG_VERSION had
>>> been assigned with = before.
>>>
>>>> So statement   $(2)_VERSION = $$(subst /,_,$($(3)_VERSION))
>>>> should become   $(2)_VERSION := $$(subst /,_,$($(3)_VERSION))
>>>
>>>  No, that doesn't make a difference, because the $(MYPKG_VERSION) is still
>>> expanded too early.
>>>
>>
>> I'm pretty sure I said this before, but all these $$$$'s are making me dizzy...
>> :-D
>
>  There shouldn't be a need for 4 $$$$'s. The ones in pkg-autotools could be avoided.
>

Quick update: I implemented the suggested approach and it seems to
work. I need to do some further testing though.

One question: for some variables, two dollar signs aren't really
needed, for example HOST_DIR, TARGET_DIR, DISABLE_NLS, QUIET, etc. as
the value of these variables are fixed and independent of the actual
package.
The usage of single vs. double dollar signs is inconsistent for such variables.
Should we use single ones (as doubles aren't necessary) or should we
use double dollars to avoid confusion?

Thanks,
Thomas

On 08/05/14 22:03, Thomas De Schampheleire wrote:
> On Thu, May 8, 2014 at 3:00 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>> On 08/05/14 13:37, Thomas De Schampheleire wrote:
>>> On Thu, May 8, 2014 at 11:52 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>>> On 08/05/14 11:28, Thomas De Schampheleire wrote:
>>>>> Hi Arnout,
>>>>>
>>>>> On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>>>>
>>>> [snip]
>>>>>>  I repeat my earlier statement: within a function that is supposed to be
>>>>>> eval'ed, *everything* should be $$'ed except the function arguments and except
>>>>>> some very specific cases.
>>>>>>
>>>>>
>>>>> But in the case of host variables being set based on target variables
>>>>> if there is no explicit host variable, as for the _VERSION, there
>>>>> would be a recursive assignment as $(2) == $(3).
>>>>
>>>> True; if you set
>>>> $(2)_VERSION = $$(subst /,_,$$($(3)_VERSION))
>>>>
>>>> that will expand to
>>>> PKG_VERSION = $$(subst /,_,$$(PKG_VERSION))
>>>>
>>>> in the eval, which is indeed recursive.
>>>>
>>>> But that can be solved with a simple:
>>>>
>>>> $(2)_VERSION := $$(subst /,_,$$($(3)_VERSION))
>>>>
>>>> Fortunately, make is smart enough to break the recursion even if PKG_VERSION had
>>>> been assigned with = before.
>>>>
>>>>> So statement   $(2)_VERSION = $$(subst /,_,$($(3)_VERSION))
>>>>> should become   $(2)_VERSION := $$(subst /,_,$($(3)_VERSION))
>>>>
>>>>  No, that doesn't make a difference, because the $(MYPKG_VERSION) is still
>>>> expanded too early.
>>>>
>>>
>>> I'm pretty sure I said this before, but all these $$$$'s are making me dizzy...
>>> :-D
>>
>>  There shouldn't be a need for 4 $$$$'s. The ones in pkg-autotools could be avoided.
>>
> 
> Quick update: I implemented the suggested approach and it seems to
> work. I need to do some further testing though.

 While you're at it, can you change the inner-virtual-package macro back to
$(2)_VERSION = virtual
(and remove the HOST_$(3)_VERSION)? The reason that it was needed originally is
exactly the missing $$ that you're fixing now.


> One question: for some variables, two dollar signs aren't really
> needed, for example HOST_DIR, TARGET_DIR, DISABLE_NLS, QUIET, etc. as
> the value of these variables are fixed and independent of the actual
> package.
> The usage of single vs. double dollar signs is inconsistent for such variables.
> Should we use single ones (as doubles aren't necessary) or should we
> use double dollars to avoid confusion?

 We should *always* use $$. I think I've said this before :-)

 The reasoning is: the number of situations where $$ is wrong is very, very
limited. By putting $$ by default, we avoid that someone _thinks_ that it's not
necessary, and years later it turns out that it was necessary. If you need an
example of such a situation: _VERSION :-)

 I've taken a look at the single-$ occurences in inner-generic-package, and
there are just a few that I think should not be changed:

- The $(pkgdir) calls that are used to calculate KCONFIG_VAR _may_ need single-$
(though I really think they don't).

- The arguments of generic-package itself _can_ be double-$$'ed, but it makes
parsing the makefile 4 times slower.


 Regards,
 Arnout