x264_r2245: fix DEPENDS for x86

Submitted by Bob Foerster on May 4, 2011, 3:56 p.m.

Details

Message ID 1304524618-4864-1-git-send-email-robert@erafx.com
State Accepted
Headers show

Commit Message

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

Patch hide | download patch | download mbox

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}"'
 

Comments

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
>