Skip to contentSkip to conversation

pkg-generic: Fix host _DL_VERSION corner case

PATCH New Clayton Shotwell 3 participants 7 messages Feb 24, 2015 – Jul 11, 2015

lore
Share & tools
lore

Clayton Shotwell

author
#0 lore raw

PATCH 1/1 pkg-generic: Fix host _DL_VERSION corner case

In the case when a package has a host version, the package is dependent on the host version, and the version contains a '/', the host version does not evaluate properly. The host version will contain a '_' instead of a '/', resulting in a failed download. To solve this corner case, add a check to see if the _DL_VERSION of the package has been defined before defining the host _DL_VERSION. If the package _DL_VERSION has not been defined yet, then the version string has not been formatted yet and is still good to use.

This error occured on a package in a BR2_EXTERNAL that uses a git repo for its remote storage with '/' in the tag names. I do not believe this affects any packages in the Buildroot mainline but it could in the future.

Signed-off-by Clayton Shotwell <[email protected]>
---
 package/pkg-generic.mk | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
package/pkg-generic.mk +5 -1
index 1b09955..fcef461 100644
@@ -323,7 +323,11 @@ $(2)_RAWNAME			=  $$(patsubst host-%,%,$(1))
 # version control system branch or tag, for example remotes/origin/1_10_stable.
 ifndef $(2)_VERSION
  ifdef $(3)_VERSION
-  $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
+  ifdef $(3)_DL_VERSION
+   $(2)_DL_VERSION := $$(strip $$($(3)_DL_VERSION))
+  else
+   $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
+  endif
   $(2)_VERSION := $$(subst /,_,$$(strip $$($(3)_VERSION)))
  else
   $(2)_VERSION = undefined
Signature · 1 line
1.9.1
#1 lore raw

PATCH 1/1 pkg-generic: Fix host _DL_VERSION corner case

Dear Clayton Shotwell,

Quoted 9 lines · Clayton Shotwell

In the case when a package has a host version, the package is dependent on the host version, and the version contains a '/', the host version does not evaluate properly. The host version will contain a '_' instead of a '/', resulting in a failed download. To solve this corner case, add a check to see if the _DL_VERSION of the package has been defined before defining the host _DL_VERSION. If the package _DL_VERSION has not been defined yet, then the version string has not been formatted yet and is still good to use.

Quoted 6 lines

This error occured on a package in a BR2_EXTERNAL that uses a git repo for its remote storage with '/' in the tag names. I do not believe this affects any packages in the Buildroot mainline but it could in the future.

Signed-off-by Clayton Shotwell <[email protected]>

So, I had a look at the issue, and I can confirm it. I did the following stupid change:

-LIBGLIB2_VERSION = $(LIBGLIB2_VERSION_MAJOR).0 +LIBGLIB2_VERSION = $(LIBGLIB2_VERSION_MAJOR).0/bar

(which obviously doesn't work, but we don't care)

And then dumped the interesting variables:

$ make printvars 2> /dev/null | grep LIBGLIB2 | grep VERSION
HOST_LIBGLIB2_DL_VERSION=2.42.0_bar (2.42.0_bar)
HOST_LIBGLIB2_VERSION=2.42.0_bar (2.42.0_bar)
LIBGLIB2_DL_VERSION=2.42.0/bar (2.42.0/bar)
LIBGLIB2_VERSION=2.42.0_bar (2.42.0_bar)

So indeed, HOST_LIBGLIB2_DL_VERSION is wrong, it should be '2.42.0/bar'.

Quoted 5 lines
package/pkg-generic.mk +0 -0
index 1b09955..fcef461 100644

package/pkg-generic.mk $(2)_RAWNAME = $$(patsubst host-%,%,$(1)) · line 330

Quoted 9 lines
package/pkg-generic.mk +5 -1
@@ -323,7 +323,11 @@ $(2)_RAWNAME			=  $$(patsubst host-%,%,$(1))
 # version control system branch or tag, for example remotes/origin/1_10_stable.
 ifndef $(2)_VERSION
  ifdef $(3)_VERSION
-  $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
+  ifdef $(3)_DL_VERSION
+   $(2)_DL_VERSION := $$(strip $$($(3)_DL_VERSION))
+  else
+   $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
+  endif

However, I haven't yet made up my mind on whether this proposed solution is the best one, or if we have a chance of doing something clearer/nicer for this _VERSION vs. _DL_VERSION thing.

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 pkg-generic: Fix host _DL_VERSION corner case

Thomas,

Quoted 7 lines · Thomas Petazzoni
$ make printvars 2> /dev/null | grep LIBGLIB2 | grep VERSION
HOST_LIBGLIB2_DL_VERSION=2.42.0_bar (2.42.0_bar)
HOST_LIBGLIB2_VERSION=2.42.0_bar (2.42.0_bar)
LIBGLIB2_DL_VERSION=2.42.0/bar (2.42.0/bar)
LIBGLIB2_VERSION=2.42.0_bar (2.42.0_bar)

So indeed, HOST_LIBGLIB2_DL_VERSION is wrong, it should be '2.42.0/bar'.

That is exactly what I was running into.

Quoted 11 lines
Quoted 11 lines
package/pkg-generic.mk +1 -1
index 1b09955..fcef461 100644
@@ -323,7 +323,11 @@ $(2)_RAWNAME                     =  $$(patsubst host-%,%,$(1))
 # version control system branch or tag, for example remotes/origin/1_10_stable.
 ifndef $(2)_VERSION
  ifdef $(3)_VERSION
-  $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
+  ifdef $(3)_DL_VERSION
Quoted 8 lines

package/pkg-generic.mk $(2)_RAWNAME = $$(patsubst host-%,%,$(1)) · line 330

Quoted 4 lines
package/pkg-generic.mk +4 -0
+   $(2)_DL_VERSION := $$(strip $$($(3)_DL_VERSION))
+  else
+   $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
+  endif

However, I haven't yet made up my mind on whether this proposed solution is the best one, or if we have a chance of doing something clearer/nicer for this _VERSION vs. _DL_VERSION thing.

This was the simplest solution I could come up with without reworking a lot of the pkg-download logic. I figured that if the _DL_VERSION of non-host version was already set then it should be used, otherwise the _VERSION string should be modified.

Thanks,
Clayton

Clayton Shotwell
Senior Software Engineer, Rockwell Collins

#3 lore raw

PATCH 1/1 pkg-generic: Fix host _DL_VERSION corner case

Dear Clayton Shotwell,

Quoted 19 lines · Clayton Shotwell
Quoted 10 lines
Quoted 10 lines
package/pkg-generic.mk +1 -1
index 1b09955..fcef461 100644
@@ -323,7 +323,11 @@ $(2)_RAWNAME                     =  $$(patsubst host-%,%,$(1))
 # version control system branch or tag, for example remotes/origin/1_10_stable.
 ifndef $(2)_VERSION
  ifdef $(3)_VERSION
-  $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
+  ifdef $(3)_DL_VERSION
Quoted 8 lines

package/pkg-generic.mk $(2)_RAWNAME = $$(patsubst host-%,%,$(1)) · line 330

Quoted 4 lines
package/pkg-generic.mk +4 -0
+   $(2)_DL_VERSION := $$(strip $$($(3)_DL_VERSION))
+  else
+   $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
+  endif

However, I haven't yet made up my mind on whether this proposed solution is the best one, or if we have a chance of doing something clearer/nicer for this _VERSION vs. _DL_VERSION thing.

Quoted 4 lines

This was the simplest solution I could come up with without reworking a lot of the pkg-download logic. I figured that if the _DL_VERSION of non-host version was already set then it should be used, otherwise the _VERSION string should be modified.

Note that I was not saying that your proposal is not good. It's just that it was already almost midnight my time, and it was too late to really think about the right solution :-)

Thanks,

Thomas

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

PATCH 1/1 pkg-generic: Fix host _DL_VERSION corner case

In the case when a package has a host version, the package is dependent on the host version, and the version contains a '/', the host version does not evaluate properly. The host version will contain a '_' instead of a '/', resulting in a failed download. To solve this corner case, add a check to see if the _DL_VERSION of the package has been defined before defining the host _DL_VERSION. If the package _DL_VERSION has not been defined yet, then the version string has not been formatted yet and is still good to use.

[Arnout: further simplify things by lifting the override over _VERSION out of the condition - it is always the same.]

This error occured on a package in a BR2_EXTERNAL that uses a git repo for its remote storage with '/' in the tag names. I do not believe this affects any packages in the Buildroot mainline but it could in the future.

[Arnout: rebase on master, fix existing whitespace error in the else branch.]

Signed-off-by Clayton Shotwell <[email protected]>
Signed-off-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>
---
 package/pkg-generic.mk | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
package/pkg-generic.mk +6 -6
index 9fe01b8..5f630e4 100644
@@ -298,17 +298,17 @@ $(2)_RAWNAME			=  $$(patsubst host-%,%,$(1))
 # Similar for spaces and colons (:) that may appear in date-based revisions for
 # CVS.
 ifndef $(2)_VERSION
- ifdef $(3)_VERSION
-  $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
-  $(2)_VERSION := $$(call sanitize,$$($(3)_VERSION))
+ ifdef $(3)_DL_VERSION
+  $(2)_DL_VERSION := $$($(3)_DL_VERSION)
+ else ifdef $(3)_VERSION
+  $(2)_DL_VERSION := $$($(3)_VERSION)
  else
-  $(2)_VERSION = undefined
   $(2)_DL_VERSION = undefined
  endif
 else
-  $(2)_DL_VERSION := $$(strip $$($(2)_VERSION))
-  $(2)_VERSION := $$(call sanitize,$$($(2)_VERSION))
+ $(2)_DL_VERSION := $$(strip $$($(2)_VERSION))
 endif
+$(2)_VERSION := $$(call sanitize,$$($(2)_DL_VERSION))
 
 $(2)_BASE_NAME	=  $(1)-$$($(2)_VERSION)
 $(2)_DL_DIR	=  $$(DL_DIR)/$$($(2)_BASE_NAME)
Signature · 1 line
2.1.4
#5 lore raw

PATCH v3 1/1 pkg-generic: Fix host _DL_VERSION corner case

From: Clayton Shotwell <[email protected]>

In the case when a package has a host version, the package is dependent on the host version, and the version contains a '/', the host version does not evaluate properly. The host version will contain a '_' instead of a '/', resulting in a failed download. To solve this corner case, add a check to see if the _DL_VERSION of the package has been defined before defining the host _DL_VERSION. If the package _DL_VERSION has not been defined yet, then the version string has not been formatted yet and is still good to use.

[Arnout: further simplify things by lifting the override over _VERSION out of the condition - it is always the same.]

This error occured on a package in a BR2_EXTERNAL that uses a git repo for its remote storage with '/' in the tag names. I do not believe this affects any packages in the Buildroot mainline but it could in the future.

[Arnout: rebase on master, fix existing whitespace error in the else branch.]

Signed-off-by Clayton Shotwell <[email protected]>
Signed-off-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

---
v3: Re-instate Clayton as author - sorry about that
v2: rebase, fix whitespace error, lift _VERSION assignment out of condition

---
 package/pkg-generic.mk | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
package/pkg-generic.mk +6 -6
index 9fe01b8..5f630e4 100644
@@ -298,17 +298,17 @@ $(2)_RAWNAME			=  $$(patsubst host-%,%,$(1))
 # Similar for spaces and colons (:) that may appear in date-based revisions for
 # CVS.
 ifndef $(2)_VERSION
- ifdef $(3)_VERSION
-  $(2)_DL_VERSION := $$(strip $$($(3)_VERSION))
-  $(2)_VERSION := $$(call sanitize,$$($(3)_VERSION))
+ ifdef $(3)_DL_VERSION
+  $(2)_DL_VERSION := $$($(3)_DL_VERSION)
+ else ifdef $(3)_VERSION
+  $(2)_DL_VERSION := $$($(3)_VERSION)
  else
-  $(2)_VERSION = undefined
   $(2)_DL_VERSION = undefined
  endif
 else
-  $(2)_DL_VERSION := $$(strip $$($(2)_VERSION))
-  $(2)_VERSION := $$(call sanitize,$$($(2)_VERSION))
+ $(2)_DL_VERSION := $$(strip $$($(2)_VERSION))
 endif
+$(2)_VERSION := $$(call sanitize,$$($(2)_DL_VERSION))
 
 $(2)_BASE_NAME	=  $(1)-$$($(2)_VERSION)
 $(2)_DL_DIR	=  $$(DL_DIR)/$$($(2)_BASE_NAME)
Signature · 1 line
2.1.4
Applied
#6 lore raw

PATCH v3 1/1 pkg-generic: Fix host _DL_VERSION corner case

Dear Arnout Vandecappelle (Essensium/Mind),

Quoted 22 lines · Arnout Vandecappelle (Essensium/Mind)

From: Clayton Shotwell <[email protected]>

In the case when a package has a host version, the package is dependent on the host version, and the version contains a '/', the host version does not evaluate properly. The host version will contain a '_' instead of a '/', resulting in a failed download. To solve this corner case, add a check to see if the _DL_VERSION of the package has been defined before defining the host _DL_VERSION. If the package _DL_VERSION has not been defined yet, then the version string has not been formatted yet and is still good to use.

[Arnout: further simplify things by lifting the override over _VERSION out of the condition - it is always the same.]

This error occured on a package in a BR2_EXTERNAL that uses a git repo for its remote storage with '/' in the tag names. I do not believe this affects any packages in the Buildroot mainline but it could in the future.

[Arnout: rebase on master, fix existing whitespace error in the else branch.]

Quoted 8 lines
Signed-off-by Clayton Shotwell <[email protected]>
Signed-off-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

---
v3: Re-instate Clayton as author - sorry about that
v2: rebase, fix whitespace error, lift _VERSION assignment out of condition

---
 package/pkg-generic.mk | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Applied, thanks.

Thomas

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