Skip to contentSkip to conversation

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

PATCH New Thomas De Schampheleire 3 participants 17 messages May 6, 2014 – May 8, 2014

lore
Share & tools
lore

Thomas De Schampheleire

author
#0 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

This patch causes the log messages to change from:

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external undefined Downloading
toolchain-external undefined Extracting

...

to

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external virtual Downloading
toolchain-external virtual Extracting

...

and similar for 'toolchain' and 'toolchain-buildroot', simply because it looks nicer.
At the same time, the directory names also become toolchain-virtual, toolchain-buildroot-virtual, toolchain-external-virtual instead of the corresponding 'undefined' variants.

Signed-off-by Thomas De Schampheleire <[email protected]>
---
 toolchain/toolchain-buildroot/toolchain-buildroot.mk |  1 +
 toolchain/toolchain-external/toolchain-external.mk   |  2 ++
 toolchain/toolchain/toolchain.mk                     |  1 +
 3 files changed, 4 insertions(+), 0 deletions(-)
toolchain/toolchain-buildroot/toolchain-buildroot.mk +1 -0
@@ -5,6 +5,7 @@
 ################################################################################
 
 TOOLCHAIN_BUILDROOT_SOURCE =
+TOOLCHAIN_BUILDROOT_VERSION = virtual
 
 BR_LIBC = $(call qstrip,$(BR2_TOOLCHAIN_BUILDROOT_LIBC))
toolchain/toolchain-external/toolchain-external.mk +2 -0
@@ -383,6 +383,8 @@ TOOLCHAIN_EXTERNAL_SITE =
 TOOLCHAIN_EXTERNAL_SOURCE =
 endif
 
+TOOLCHAIN_EXTERNAL_VERSION = virtual
+
 TOOLCHAIN_EXTERNAL_ADD_TOOLCHAIN_DEPENDENCY = NO
 
 TOOLCHAIN_EXTERNAL_INSTALL_STAGING = YES
toolchain/toolchain/toolchain.mk +1 -0
@@ -5,6 +5,7 @@
 ################################################################################
 
 TOOLCHAIN_SOURCE =
+TOOLCHAIN_VERSION = virtual
 
 ifeq ($(BR2_TOOLCHAIN_BUILDROOT),y)
 TOOLCHAIN_DEPENDENCIES += toolchain-buildroot
#1 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Dear Thomas De Schampheleire,

Quoted 12 lines · Thomas De Schampheleire

This patch causes the log messages to change from:

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external undefined Downloading
toolchain-external undefined Extracting

...

to

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external virtual Downloading
toolchain-external virtual Extracting

...

Quoted 7 lines

and similar for 'toolchain' and 'toolchain-buildroot', simply because it looks nicer.
At the same time, the directory names also become toolchain-virtual, toolchain-buildroot-virtual, toolchain-external-virtual instead of the corresponding 'undefined' variants.

Signed-off-by Thomas De Schampheleire <[email protected]>

Shouldn't we instead migrate these to use the virtual package infrastructure, and make this infrastructure define this magic 'virtual' version?

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#2 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Quoted 21 lines · Thomas Petazzoni

Dear Thomas De Schampheleire,

Quoted 18 lines · Thomas De Schampheleire

This patch causes the log messages to change from:

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external undefined Downloading
toolchain-external undefined Extracting

...

to

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external virtual Downloading
toolchain-external virtual Extracting

...

and similar for 'toolchain' and 'toolchain-buildroot', simply because it looks nicer.
At the same time, the directory names also become toolchain-virtual, toolchain-buildroot-virtual, toolchain-external-virtual instead of the corresponding 'undefined' variants.

Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas De Schampheleire <[email protected]>

Shouldn't we instead migrate these to use the virtual package infrastructure, and make this infrastructure define this magic 'virtual' version?

For toolchain and toolchain-buildroot that would be possible, but for toolchain-external not, right?

Thanks,
Thomas

#3 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Dear Thomas De Schampheleire,

Quoted 6 lines · Thomas De Schampheleire
Quoted 3 lines

Shouldn't we instead migrate these to use the virtual package infrastructure, and make this infrastructure define this magic 'virtual' version?

For toolchain and toolchain-buildroot that would be possible, but for toolchain-external not, right?

Right. But toolchain-external is not a virtual package, so setting its version to 'virtual' would anyway be strange, no?

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#4 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Quoted 8 lines · Thomas Petazzoni

Dear Thomas De Schampheleire,

Quoted 4 lines · Thomas De Schampheleire
Quoted 3 lines

Shouldn't we instead migrate these to use the virtual package infrastructure, and make this infrastructure define this magic 'virtual' version?

Quoted 5 lines
Quoted 2 lines

For toolchain and toolchain-buildroot that would be possible, but for toolchain-external not, right?

Right. But toolchain-external is not a virtual package, so setting its version to 'virtual' would anyway be strange, no?

Not more strange than setting the version to 'undefined' :-D

Any other string than 'undefined' is fine for me too.

Btw, for makedevs and mkpasswd, the version is also 'undefined' as the source is included in buildroot.
I was first going to change their version to 'buildroot', but it makes the messages a bit strange:

Quoted 1 line
Quoted 1 line
Quoted 1 line

host-makedevs buildroot Configuring...

etc.

Other ideas?
Thomas

#5 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Quoted 16 lines · Thomas De Schampheleire
Quoted 8 lines · Thomas Petazzoni

Dear Thomas De Schampheleire,

Quoted 4 lines · Thomas De Schampheleire
Quoted 3 lines

Shouldn't we instead migrate these to use the virtual package infrastructure, and make this infrastructure define this magic 'virtual' version?

Quoted 5 lines
Quoted 2 lines

For toolchain and toolchain-buildroot that would be possible, but for toolchain-external not, right?

Right. But toolchain-external is not a virtual package, so setting its version to 'virtual' would anyway be strange, no?

Quoted 10 lines

Not more strange than setting the version to 'undefined' :-D

Any other string than 'undefined' is fine for me too.

Btw, for makedevs and mkpasswd, the version is also 'undefined' as the source is included in buildroot.
I was first going to change their version to 'buildroot', but it makes the messages a bit strange:

Quoted 1 line
Quoted 1 line
Quoted 1 line

host-makedevs buildroot Configuring...

MAKEDEVS_VERSION = buildroot-$(BR2_VERSION)

?

Regards,
Arnout

Signature · 6 lines
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
author
#6 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

In reply to Thomas Petazzoni

Thomas Petazzoni <[email protected]> schreef:

Quoted 21 lines

Dear Thomas De Schampheleire,

Quoted 18 lines · Thomas De Schampheleire

This patch causes the log messages to change from:

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external undefined Downloading
toolchain-external undefined Extracting

...

to

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external virtual Downloading
toolchain-external virtual Extracting

...

and similar for 'toolchain' and 'toolchain-buildroot', simply because it looks nicer.
At the same time, the directory names also become toolchain-virtual, toolchain-buildroot-virtual, toolchain-external-virtual instead of the corresponding 'undefined' variants.

Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas De Schampheleire <[email protected]>

Shouldn't we instead migrate these to use the virtual package infrastructure, and make this infrastructure define this magic 'virtual' version?

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

Yann, Thomas: any ideas?

Thanks,
Thomas

author
#7 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Hi,

Quoted 22 lines · Thomas De Schampheleire

Thomas Petazzoni <[email protected]> schreef:

Quoted 21 lines

Dear Thomas De Schampheleire,

Quoted 18 lines · Thomas De Schampheleire

This patch causes the log messages to change from:

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external undefined Downloading
toolchain-external undefined Extracting

...

to

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external virtual Downloading
toolchain-external virtual Extracting

...

and similar for 'toolchain' and 'toolchain-buildroot', simply because it looks nicer.
At the same time, the directory names also become toolchain-virtual, toolchain-buildroot-virtual, toolchain-external-virtual instead of the corresponding 'undefined' variants.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas De Schampheleire <[email protected]>

Shouldn't we instead migrate these to use the virtual package infrastructure, and make this infrastructure define this magic 'virtual' version?

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

Quick update:

- Yann mentioned on IRC a comment he wrote in a mail when the virtual string was introduced. I think the below is the comment Yann refers to:

http://lists.busybox.net/pipermail/buildroot/2014-April/093670.html
" - the version string is strange. This does not work:
$(2)_VERSION = virtual
so I had to come up with the solution in patch 20.
"

that solution in patch 20 is:

+# Fake a version string, so it looks nicer in the build log +$(3)_VERSION = virtual
+HOST_$(3)_VERSION = virtual
+

The code I was using originally is:

toolchain/toolchain/toolchain.mk +5 -3
@@ -4,8 +4,6 @@
 #
 ################################################################################
 
-TOOLCHAIN_SOURCE =
-
 ifeq ($(BR2_TOOLCHAIN_BUILDROOT),y)
 TOOLCHAIN_DEPENDENCIES += toolchain-buildroot
 else ifeq ($(BR2_TOOLCHAIN_EXTERNAL),y)

@@ -14,6 +12,10 @@ endif
 
 TOOLCHAIN_ADD_TOOLCHAIN_DEPENDENCY = NO
 
-$(eval $(generic-package))
+$(eval $(virtual-package))
 
 toolchain: $(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake
+
+foo:
+       @echo TOOLCHAIN_VERSION = $(TOOLCHAIN_VERSION)
+       @echo HOST_TOOLCHAIN_VERSION = $(HOST_TOOLCHAIN_VERSION)

In this case of using virtual-package, the output of 'make foo' is:
TOOLCHAIN_VERSION =
HOST_TOOLCHAIN_VERSION = virtual

And if I change the code to use host-virtual-package iso virtual-package, the output is (surprisingly):
TOOLCHAIN_VERSION = virtual HOST_TOOLCHAIN_VERSION =

So there is definitely something odd going on here.

If I print the parameters to the infra: (for virtual and host-virtual respectively):
1=toolchain 2=TOOLCHAIN 3=TOOLCHAIN 4=target 1=host-toolchain 2=HOST_TOOLCHAIN 3=TOOLCHAIN 4=host

so the code
$(3)_VERSION = virtual
HOST_$(3)_VERSION = virtual

is supposed to work fine in either case, as would the original $(2)_VERSION
by the way.

Looking at printvars doesn't tell much more:
virtual-package:
HOST_TOOLCHAIN_VERSION=virtual (virtual) TOOLCHAIN_BASE_NAME=toolchain- (toolchain-$(TOOLCHAIN_VERSION)) TOOLCHAIN_VERSION= ()

host-virtual-package:
HOST_TOOLCHAIN_BASE_NAME=host-toolchain-
(host-toolchain-$(HOST_TOOLCHAIN_VERSION))
HOST_TOOLCHAIN_VERSION= ()
TOOLCHAIN_VERSION=virtual (virtual)

I'm a little puzzled at the moment on how to proceed...

Best regards,
Thomas

author
#8 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Thomas De Schampheleire <[email protected]> schreef:

Quoted 115 lines

Hi,

Quoted 22 lines · Thomas De Schampheleire

Thomas Petazzoni <[email protected]> schreef:

Quoted 21 lines

Dear Thomas De Schampheleire,

Quoted 18 lines · Thomas De Schampheleire

This patch causes the log messages to change from:

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external undefined Downloading
toolchain-external undefined Extracting

...

to

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

toolchain-external virtual Downloading
toolchain-external virtual Extracting

...

and similar for 'toolchain' and 'toolchain-buildroot', simply because it looks nicer.
At the same time, the directory names also become toolchain-virtual, toolchain-buildroot-virtual, toolchain-external-virtual instead of the corresponding 'undefined' variants.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas De Schampheleire <[email protected]>

Shouldn't we instead migrate these to use the virtual package infrastructure, and make this infrastructure define this magic 'virtual' version?

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

Quick update:

- Yann mentioned on IRC a comment he wrote in a mail when the virtual string was introduced. I think the below is the comment Yann refers to:

http://lists.busybox.net/pipermail/buildroot/2014-April/093670.html
" - the version string is strange. This does not work:

       $(2)_VERSION = virtual

so I had to come up with the solution in patch 20.
"

that solution in patch 20 is:

(quoted diff) +4 -0
+# Fake a version string, so it looks nicer in the build log
+$(3)_VERSION = virtual
+HOST_$(3)_VERSION = virtual
+

The code I was using originally is:

toolchain/toolchain/toolchain.mk +0 -2
@@ -4,8 +4,6 @@
 #
 ################################################################################
 
-TOOLCHAIN_SOURCE =
-
 ifeq ($(BR2_TOOLCHAIN_BUILDROOT),y)

TOOLCHAIN_DEPENDENCIES += toolchain-buildroot
else ifeq ($(BR2_TOOLCHAIN_EXTERNAL),y)

toolchain/toolchain/toolchain.mk +0 -0
@@ -14,6 +12,10 @@ endif

TOOLCHAIN_ADD_TOOLCHAIN_DEPENDENCY = NO

-$(eval $(generic-package))
+$(eval $(virtual-package))

toolchain: $(HOST_DIR)/usr/share/buildroot/toolchainfile.cmake

toolchain/toolchain/toolchain.mk +4 -0
+
+foo:
+       @echo TOOLCHAIN_VERSION = $(TOOLCHAIN_VERSION)
+       @echo HOST_TOOLCHAIN_VERSION = $(HOST_TOOLCHAIN_VERSION)

In this case of using virtual-package, the output of 'make foo' is:
TOOLCHAIN_VERSION =
HOST_TOOLCHAIN_VERSION = virtual

And if I change the code to use host-virtual-package iso virtual-package, the output is (surprisingly):
TOOLCHAIN_VERSION = virtual HOST_TOOLCHAIN_VERSION =

So there is definitely something odd going on here.

If I print the parameters to the infra: (for virtual and host-virtual respectively):
1=toolchain 2=TOOLCHAIN 3=TOOLCHAIN 4=target 1=host-toolchain 2=HOST_TOOLCHAIN 3=TOOLCHAIN 4=host

so the code
$(3)_VERSION = virtual
HOST_$(3)_VERSION = virtual

is supposed to work fine in either case, as would the original $(2)_VERSION
by the way.

Looking at printvars doesn't tell much more:
virtual-package:
HOST_TOOLCHAIN_VERSION=virtual (virtual) TOOLCHAIN_BASE_NAME=toolchain- (toolchain-$(TOOLCHAIN_VERSION)) TOOLCHAIN_VERSION= ()

Quoted 8 lines

host-virtual-package:
HOST_TOOLCHAIN_BASE_NAME=host-toolchain-
(host-toolchain-$(HOST_TOOLCHAIN_VERSION))
HOST_TOOLCHAIN_VERSION= ()
TOOLCHAIN_VERSION=virtual (virtual)

I'm a little puzzled at the moment on how to proceed...

My current assumption is that there is some interference between the virtual and generic infra wrt the version variables...

author
#9 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Hi,

On Wed, May 7, 2014 at 9:37 PM, Thomas De Schampheleire <[email protected]> wrote:
[..]

Quoted 1 line
Quoted 1 line
Quoted 1 line

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.

package/mytest/mytest.mk new file +10 -0
new file mode 100644
@@ -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)
package/pkg-generic.mk +6 -0
@@ -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?

Thanks,
Thomas

#10 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Quoted 69 lines · Thomas De Schampheleire

Hi,

On Wed, May 7, 2014 at 9:37 PM, Thomas De Schampheleire <[email protected]> wrote:
[..]

Quoted 1 line
Quoted 1 line
Quoted 1 line

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.

package/mytest/mytest.mk new file +10 -0
new file mode 100644
@@ -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)
package/pkg-generic.mk +6 -0
@@ -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.

Quoted 2 lines

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

Signature · 6 lines
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
author
#11 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Hi Arnout,

Quoted 109 lines · Arnout Vandecappelle
Quoted 69 lines · Thomas De Schampheleire

Hi,

On Wed, May 7, 2014 at 9:37 PM, Thomas De Schampheleire <[email protected]> wrote:
[..]

Quoted 1 line
Quoted 1 line
Quoted 1 line

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.

package/mytest/mytest.mk new file +10 -0
new file mode 100644
@@ -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)
package/pkg-generic.mk +6 -0
@@ -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.

Quoted 2 lines

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

Quoted 4 lines

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

#12 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Quoted 3 lines · Thomas De Schampheleire

Hi Arnout,

On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <[email protected]> wrote:

[snip]

Quoted 8 lines
Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 6 lines

then, right?
I haven't tested whether this works though...

Best regards,
Thomas

Signature · 6 lines
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
author
#13 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Quoted 27 lines · Arnout Vandecappelle
Quoted 3 lines · Thomas De Schampheleire

Hi Arnout,

On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <[email protected]> wrote:

[snip]

Quoted 8 lines
Quoted 4 lines

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))

Quoted 9 lines

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

Quoted 2 lines

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

#14 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Quoted 34 lines · Thomas De Schampheleire
Quoted 33 lines · Arnout Vandecappelle
Quoted 3 lines · Thomas De Schampheleire

Hi Arnout,

On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <[email protected]> wrote:

[snip]

Quoted 8 lines
Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 6 lines
Quoted 3 lines

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

Quoted 6 lines

I will do a test with your suggestion, thanks!

Best regards,
Thomas

Signature · 6 lines
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
author
#15 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Quoted 35 lines · Arnout Vandecappelle
Quoted 34 lines · Thomas De Schampheleire
Quoted 33 lines · Arnout Vandecappelle
Quoted 3 lines · Thomas De Schampheleire

Hi Arnout,

On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <[email protected]> wrote:

[snip]

Quoted 8 lines
Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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

#16 lore raw

PATCH 1/1 toolchain package: set version as 'virtual' instead of 'undefined'

Quoted 40 lines · Thomas De Schampheleire
Quoted 39 lines · Arnout Vandecappelle
Quoted 38 lines · Thomas De Schampheleire
Quoted 36 lines · Arnout Vandecappelle
Quoted 3 lines · Thomas De Schampheleire

Hi Arnout,

On Thu, May 8, 2014 at 10:31 AM, Arnout Vandecappelle <[email protected]> wrote:

[snip]

Quoted 8 lines
Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

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.

Quoted 7 lines

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

Signature · 6 lines
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