legal-info: fix detection of "override" packages

PATCH New Luca Ceresoli 3 participants 6 messages Mar 4, 2016 – Mar 6, 2016

Share & tools

Luca Ceresoli

author

2016-03-04 23:39 UTC #0 lore raw

PATCH 1/2 legal-info: fix detection of "override" packages

The ifneq ($$($(2)_SITE_METHOD),override) test will always match because SITE_METHOD is never set to "override". The result is that some packages are downloaded and extracted unnecessarily, although they are (correctly) not saved in output/legal-info/.

Fix by simply testing $(2)_OVERRIDE_SRCDIR instead: when empty, it indicates that the packages is neither a "local" site-method package nor a package that has its source directory overridden.

Signed-off-by Luca Ceresoli <[email protected]>

Reported-by Thomas Petazzoni <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc "Yann E. MORIN" <[email protected]>

---

---
 package/pkg-generic.mk | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

package/pkg-generic.mk +1 -3

index e22babbcd0c9..9ea2a707baa4 100644
@@ -757,13 +757,11 @@ endif
 # non-local, and non-overriden. So only store, in the manifest, the tarball
 # name of those packages.
 ifeq ($$($(2)_REDISTRIBUTE),YES)
-ifneq ($$($(2)_SITE_METHOD),local)
-ifneq ($$($(2)_SITE_METHOD),override)
+ifeq ($$($(2)_OVERRIDE_SRCDIR),)
 # Packages that have a tarball need it downloaded beforehand
 $(1)-legal-info: $(1)-source $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
 endif
 endif
-endif
 
 # If FOO_ACTUAL_SOURCE_TARBALL is explicitly defined, it means FOO_SOURCE is
 # indeed a binary (e.g. external toolchain) and FOO_ACTUAL_SOURCE_TARBALL/_SITE

Signature · 1 line

1.9.1

Luca Ceresoli

author

2016-03-04 23:39 UTC #1 lore raw

PATCH 2/2 legal-info: remove outdated comment

This comment is outdated since the assignment of $(2)_MANIFEST_TARBALL and $(2)_MANIFEST_SITE have been removed, which happened in b9f4727ff4d512868352494f5209fff4da147473.

Signed-off-by Luca Ceresoli <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc "Yann E. MORIN" <[email protected]>

---
 package/pkg-generic.mk | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

package/pkg-generic.mk +1 -2

index 9ea2a707baa4..189ccf8c8415 100644
@@ -754,8 +754,7 @@ $(1)-legal-info: $(1)-patch
 endif
 
 # We only save the sources of packages we want to redistribute, that are
-# non-local, and non-overriden. So only store, in the manifest, the tarball
-# name of those packages.
+# non-local, and non-overriden.
 ifeq ($$($(2)_REDISTRIBUTE),YES)
 ifeq ($$($(2)_OVERRIDE_SRCDIR),)
 # Packages that have a tarball need it downloaded beforehand

Signature · 1 line

1.9.1

Yann E. MORIN

2016-03-06 10:19 UTC #2 lore raw

PATCH 2/2 legal-info: remove outdated comment

Luca, All,

On 2016-03-05 00:39 +0100, Luca Ceresoli spake thusly:

Quoted 16 lines

This comment is outdated since the assignment of $(2)_MANIFEST_TARBALL and $(2)_MANIFEST_SITE have been removed, which happened in b9f4727ff4d512868352494f5209fff4da147473.
Signed-off-by Luca Ceresoli <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc "Yann E. MORIN" <[email protected]>
---
 package/pkg-generic.mk | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
package/pkg-generic.mk +0 -0
index 9ea2a707baa4..189ccf8c8415 100644

package/pkg-generic.mk $(1)-legal-info: $(1)-patch · line 757

Quoted 6 lines

package/pkg-generic.mk +1 -2

@@ -754,8 +754,7 @@ $(1)-legal-info: $(1)-patch
 endif
 
 # We only save the sources of packages we want to redistribute, that are
-# non-local, and non-overriden. So only store, in the manifest, the tarball
-# name of those packages.
+# non-local, and non-overriden.

I wondered why you kept 'local' here, but then I remembered that we handle 'local' packages as if they were overriden. I think that's inherently bad that we do that, but this is not the purpose of your patch.

Still, I would prefer something like:

... that are non-overriden (local or true override).

otherwise:

Reviewed-by "Yann E. MORIN" <[email protected]>

BTW, you're not making my life easy: I'll have to rebase my own legal series over that, and I expect quite a bunch of conflicts! ;-)

Regards,
Yann E. MORIN.

Quoted 6 lines

package/pkg-generic.mk +0 -0

 ifeq ($$($(2)_REDISTRIBUTE),YES)
 ifeq ($$($(2)_OVERRIDE_SRCDIR),)
 # Packages that have a tarball need it downloaded beforehand

Signature · 1 line

1.9.1

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Luca Ceresoli

author

2016-03-06 22:52 UTC #3 lore raw

PATCH 2/2 legal-info: remove outdated comment

Dear Yann,

Quoted 33 lines · Yann E. MORIN

Luca, All,
On 2016-03-05 00:39 +0100, Luca Ceresoli spake thusly:
Quoted 16 lines
This comment is outdated since the assignment of $(2)_MANIFEST_TARBALL and $(2)_MANIFEST_SITE have been removed, which happened in b9f4727ff4d512868352494f5209fff4da147473.
Signed-off-by Luca Ceresoli <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc "Yann E. MORIN" <[email protected]>
---
 package/pkg-generic.mk | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
package/pkg-generic.mk +0 -0
index 9ea2a707baa4..189ccf8c8415 100644
package/pkg-generic.mk $(1)-legal-info: $(1)-patch · line 757
Quoted 6 lines
package/pkg-generic.mk +1 -2
@@ -754,8 +754,7 @@ $(1)-legal-info: $(1)-patch
 endif
 
 # We only save the sources of packages we want to redistribute, that are
-# non-local, and non-overriden. So only store, in the manifest, the tarball
-# name of those packages.
+# non-local, and non-overriden.
I wondered why you kept 'local' here, but then I remembered that we handle 'local' packages as if they were overriden. I think that's inherently bad that we do that, but this is not the purpose of your patch.
Still, I would prefer something like:

Quoted 8 lines

... that are non-overriden (local or true override).
otherwise:
Reviewed-by "Yann E. MORIN" <[email protected]>
BTW, you're not making my life easy: I'll have to rebase my own legal series over that, and I expect quite a bunch of conflicts! ;-)

Although I'm trying hard to make your life more difficult, I'm afraid I'm not succeding! :-)

Seriously speaking, your legal series in its v4 cleanly rebases on top of the present patch.

Signature · 1 line

Luca

Yann E. MORIN

2016-03-06 10:13 UTC #4 lore raw

PATCH 1/2 legal-info: fix detection of "override" packages

In reply to Luca Ceresoli

Luca, All,

On 2016-03-05 00:39 +0100, Luca Ceresoli spake thusly:

Quoted 5 lines

The ifneq ($$($(2)_SITE_METHOD),override) test will always match because SITE_METHOD is never set to "override". The result is that some packages are downloaded and extracted unnecessarily, although they are (correctly) not saved in output/legal-info/.

Quoted 8 lines

Fix by simply testing $(2)_OVERRIDE_SRCDIR instead: when empty, it indicates that the packages is neither a "local" site-method package nor a package that has its source directory overridden.
Signed-off-by Luca Ceresoli <[email protected]>
Reported-by Thomas Petazzoni <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc "Yann E. MORIN" <[email protected]>

Acked-by "Yann E. MORIN" <[email protected]>

Regards,
Yann E. MORIN.

Quoted 30 lines

---

---
 package/pkg-generic.mk | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

package/pkg-generic.mk +1 -3

index e22babbcd0c9..9ea2a707baa4 100644
@@ -757,13 +757,11 @@ endif
 # non-local, and non-overriden. So only store, in the manifest, the tarball
 # name of those packages.
 ifeq ($$($(2)_REDISTRIBUTE),YES)
-ifneq ($$($(2)_SITE_METHOD),local)
-ifneq ($$($(2)_SITE_METHOD),override)
+ifeq ($$($(2)_OVERRIDE_SRCDIR),)
 # Packages that have a tarball need it downloaded beforehand
 $(1)-legal-info: $(1)-source $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
 endif
 endif
-endif
 
 # If FOO_ACTUAL_SOURCE_TARBALL is explicitly defined, it means FOO_SOURCE is
 # indeed a binary (e.g. external toolchain) and FOO_ACTUAL_SOURCE_TARBALL/_SITE

Signature · 1 line

1.9.1

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Thomas Petazzoni

Applied

2016-03-06 13:34 UTC #5 lore raw

PATCH 1/2 legal-info: fix detection of "override" packages

In reply to Luca Ceresoli

Dear Luca Ceresoli,

Quoted 9 lines · Luca Ceresoli

The ifneq ($$($(2)_SITE_METHOD),override) test will always match because SITE_METHOD is never set to "override". The result is that some packages are downloaded and extracted unnecessarily, although they are (correctly) not saved in output/legal-info/.
Fix by simply testing $(2)_OVERRIDE_SRCDIR instead: when empty, it indicates that the packages is neither a "local" site-method package nor a package that has its source directory overridden.

Quoted 6 lines

Signed-off-by Luca Ceresoli <[email protected]>
Reported-by Thomas Petazzoni <[email protected]>
Cc Thomas Petazzoni <[email protected]>
Cc "Yann E. MORIN" <[email protected]>
---

Applied to master, thanks.

Thomas

Signature · 3 lines

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