Patchwork x264_r2245: fix DEPENDS for x86

login
register
mail settings
Submitter Bob Foerster
Date May 4, 2011, 3:56 p.m.
Message ID <1304524618-4864-1-git-send-email-robert@erafx.com>
Download mbox | patch
Permalink /patch/3191/
State Accepted
Headers show

Comments

Bob Foerster - May 4, 2011, 3:56 p.m.
The recipe intended to add "yasm-native" to the dependencies on x86,
but it set DEPENDS_x86 = "yasm-native" which obliterated the base
dependencies. On a fresh build, this package was failing since it
was trying to build before the compiler was ready.

Depends is now set with DEPENDS_append_x86, which leaves the base
dependencies intact.

Signed-off-by: Bob Foerster <robert@erafx.com>
---
 recipes/vlc/x264_r2245.bb |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Khem Raj - May 8, 2011, 12:59 a.m.
On (04/05/11 11:56), Bob Foerster wrote:
> The recipe intended to add "yasm-native" to the dependencies on x86,
> but it set DEPENDS_x86 = "yasm-native" which obliterated the base
> dependencies. On a fresh build, this package was failing since it
> was trying to build before the compiler was ready.
> 
> Depends is now set with DEPENDS_append_x86, which leaves the base
> dependencies intact.
> 
> Signed-off-by: Bob Foerster <robert@erafx.com>
> ---
>  recipes/vlc/x264_r2245.bb |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/recipes/vlc/x264_r2245.bb b/recipes/vlc/x264_r2245.bb
> index 0cffea3..8c2b5b4 100644
> --- a/recipes/vlc/x264_r2245.bb
> +++ b/recipes/vlc/x264_r2245.bb
> @@ -23,7 +23,7 @@ X264_ECFLAGS_mipsel = "-fPIC"
>  X264_DISABLE_ASM = "--disable-asm"
>  # use assembler written functions for those archs supporting this
>  X264_DISABLE_ASM_x86 = ""
> -DEPENDS_x86 = "yasm-native"
> +DEPENDS_append_x86 = "yasm-native"
                         ^
you need a space 

Otherwise looks ok
>  
>  EXTRA_OECONF = '--enable-shared ${X264_DISABLE_ASM} --extra-cflags="${X264_ECFLAGS}"'
>  
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> Openembedded-devel mailing list
> Openembedded-devel@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-devel
Paul Menzel - May 8, 2011, 6:56 a.m.
Am Samstag, den 07.05.2011, 17:59 -0700 schrieb Khem Raj:
> On (04/05/11 11:56), Bob Foerster wrote:

You can be more specific in the commit summary.

	do not override `DEPENDS` for x86

> > The recipe intended to add "yasm-native" to the dependencies on x86,

Use present tense. s/intended/intends/

> > but it set DEPENDS_x86 = "yasm-native" which obliterated the base

s/set/sets/

> > dependencies. On a fresh build, this package was failing since it
> > was trying to build before the compiler was ready.
> > 
> > Depends is now set with DEPENDS_append_x86, which leaves the base
> > dependencies intact.
> > 
> > Signed-off-by: Bob Foerster <robert@erafx.com>
> > ---
> >  recipes/vlc/x264_r2245.bb |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/recipes/vlc/x264_r2245.bb b/recipes/vlc/x264_r2245.bb
> > index 0cffea3..8c2b5b4 100644
> > --- a/recipes/vlc/x264_r2245.bb
> > +++ b/recipes/vlc/x264_r2245.bb
> > @@ -23,7 +23,7 @@ X264_ECFLAGS_mipsel = "-fPIC"
> >  X264_DISABLE_ASM = "--disable-asm"
> >  # use assembler written functions for those archs supporting this
> >  X264_DISABLE_ASM_x86 = ""
> > -DEPENDS_x86 = "yasm-native"
> > +DEPENDS_append_x86 = "yasm-native"
>                          ^
> you need a space 

You should also be able to use +=, should not you?

> Otherwise looks ok
> >  
> >  EXTRA_OECONF = '--enable-shared ${X264_DISABLE_ASM} --extra-cflags="${X264_ECFLAGS}"'


Thanks,

Paul
Phil Blundell - May 8, 2011, 9:02 a.m.
On Sun, 2011-05-08 at 08:56 +0200, Paul Menzel wrote:
> Am Samstag, den 07.05.2011, 17:59 -0700 schrieb Khem Raj:
> > On (04/05/11 11:56), Bob Foerster wrote:
> 
> You can be more specific in the commit summary.
> 
> 	do not override `DEPENDS` for x86
> 
> > > The recipe intended to add "yasm-native" to the dependencies on x86,
> 
> Use present tense. s/intended/intends/

If this is talking about the former, erroneous behaviour (which is
corrected by this patch) then the past tense is correct.

> > > -DEPENDS_x86 = "yasm-native"
> > > +DEPENDS_append_x86 = "yasm-native"
> >                          ^
> > you need a space 
> 
> You should also be able to use +=, should not you?

No, += doesn't work with overrides.  "DEPENDS_x86 +=" would do the same
wrong thing as the original "DEPENDS_x86".

p.
Paul Menzel - May 8, 2011, 9:16 a.m.
Am Sonntag, den 08.05.2011, 10:02 +0100 schrieb Phil Blundell:
> On Sun, 2011-05-08 at 08:56 +0200, Paul Menzel wrote:
> > Am Samstag, den 07.05.2011, 17:59 -0700 schrieb Khem Raj:
> > > On (04/05/11 11:56), Bob Foerster wrote:
> > 
> > You can be more specific in the commit summary.
> > 
> > 	do not override `DEPENDS` for x86
> > 
> > > > The recipe intended to add "yasm-native" to the dependencies on x86,
> > 
> > Use present tense. s/intended/intends/
> 
> If this is talking about the former, erroneous behaviour (which is
> corrected by this patch) then the past tense is correct.

I thought in minutes or proceeding always the present tense is used. The
recipe behaves incorrectly until the patch is applied. But it is not
very important.

> > > > -DEPENDS_x86 = "yasm-native"
> > > > +DEPENDS_append_x86 = "yasm-native"
> > >                          ^
> > > you need a space 
> > 
> > You should also be able to use +=, should not you?
> 
> No, += doesn't work with overrides.  "DEPENDS_x86 +=" would do the same
> wrong thing as the original "DEPENDS_x86".

Thank you for the clarification. Is for example `RDEPENDS_${PN}` also an
override? If yes, then looking at

	$ git grep 'DEPENDS_'

returns a lot of incorrect usages.


Thanks,

Paul
Phil Blundell - May 8, 2011, 10:51 a.m.
On Sun, 2011-05-08 at 11:16 +0200, Paul Menzel wrote:
> Thank you for the clarification. Is for example `RDEPENDS_${PN}` also an
> override?

Not in the sense I was meaning here, no.  (It is true that the ${PN} bit
is technically an OVERRIDE, but in most/all cases RDEPENDS_${PN} is
effectively a primary variable, i.e. there is no underlying RDEPENDS
which it overrides.)

The issue with the "DEPENDS_x86" thing is that, as far as the initial
metadata processing is concerned, DEPENDS and DEPENDS_x86 are distinct
variables.  You might hope that "DEPENDS_x86 += ..." would do a
conditional append to DEPENDS, but it doesn't: what happens is that it
creates a new variable "DEPENDS_x86" which then replaces the contents of
DEPENDS during override processing.  So the initial value of ${DEPENDS}
is lost rather than being appended to.

This whole issue is basically the whole reason that the "_append" syntax
hasn't been deprecated, since we don't currently have any other way to
do conditional appending (apart from resorting to python).

>  If yes, then looking at
> 
> 	$ git grep 'DEPENDS_'
> 
> returns a lot of incorrect usages.

From a quick glance at that output nothing jumped out at me as being
incorrect, but there might well be a few wrong ones in there.  This one,
for example:

classes/vala.bbclass:DEPENDS_virtclass-native += "vala-native"

looks a bit dubious to me (for the same reasons as above) but without
checking the recipe context it's impossible to say whether it's actually
wrong or not.

p.
Bob Foerster - May 8, 2011, 10:56 a.m.
On Sat, May 7, 2011 at 8:59 PM, Khem Raj <raj.khem@gmail.com> wrote:

> On (04/05/11 11:56), Bob Foerster wrote:
> > The recipe intended to add "yasm-native" to the dependencies on x86,
> > but it set DEPENDS_x86 = "yasm-native" which obliterated the base
> > dependencies. On a fresh build, this package was failing since it
> > was trying to build before the compiler was ready.
> >
> > Depends is now set with DEPENDS_append_x86, which leaves the base
> > dependencies intact.
> >
> > Signed-off-by: Bob Foerster <robert@erafx.com>
> > ---
> >  recipes/vlc/x264_r2245.bb |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/recipes/vlc/x264_r2245.bb b/recipes/vlc/x264_r2245.bb
> > index 0cffea3..8c2b5b4 100644
> > --- a/recipes/vlc/x264_r2245.bb
> > +++ b/recipes/vlc/x264_r2245.bb
> > @@ -23,7 +23,7 @@ X264_ECFLAGS_mipsel = "-fPIC"
> >  X264_DISABLE_ASM = "--disable-asm"
> >  # use assembler written functions for those archs supporting this
> >  X264_DISABLE_ASM_x86 = ""
> > -DEPENDS_x86 = "yasm-native"
> > +DEPENDS_append_x86 = "yasm-native"
>                          ^
> you need a space
>
>
Oddly, it works without the space. From bitbake -e:
DEPENDS="pkgconfig-native autoconf-native automake-native help2man-native
libtool-native libtool-cross gnu-config-native virtual/i486-oe-linux-gcc
virtual/libc yasm-native"

Nonetheless, I'll add a space and send a v2.



> Otherwise looks ok
> >
> >  EXTRA_OECONF = '--enable-shared ${X264_DISABLE_ASM}
> --extra-cflags="${X264_ECFLAGS}"'
> >
> > --
> > 1.7.1
> >
> >
> > _______________________________________________
> > Openembedded-devel mailing list
> > Openembedded-devel@lists.openembedded.org
> > http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-devel
>
> --
> -Khem
>
> _______________________________________________
> Openembedded-devel mailing list
> Openembedded-devel@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-devel
>

Patch

diff --git a/recipes/vlc/x264_r2245.bb b/recipes/vlc/x264_r2245.bb
index 0cffea3..8c2b5b4 100644
--- a/recipes/vlc/x264_r2245.bb
+++ b/recipes/vlc/x264_r2245.bb
@@ -23,7 +23,7 @@  X264_ECFLAGS_mipsel = "-fPIC"
 X264_DISABLE_ASM = "--disable-asm"
 # use assembler written functions for those archs supporting this
 X264_DISABLE_ASM_x86 = ""
-DEPENDS_x86 = "yasm-native"
+DEPENDS_append_x86 = "yasm-native"
 
 EXTRA_OECONF = '--enable-shared ${X264_DISABLE_ASM} --extra-cflags="${X264_ECFLAGS}"'