[Buildroot] [PATCH 4/5] x264: new package

[David du Colombier]

Thanks for the review, Thomas.

> One question: we received only PATCH 4/5 and PATCH 5/5 from your
> series. Is this normal? Maybe it's just a small mistake on your side
> when generating the patches.

Yes, there are only two patches.

>> diff --git a/package/Config.in b/package/Config.in
>> index 2ad72bc..33616e1 100644
>> --- a/package/Config.in
>> +++ b/package/Config.in
>> @@ -33,6 +33,7 @@ menu "Audio and video applications"
>>       source "package/vlc/Config.in"
>>       source "package/vorbis-tools/Config.in"
>>       source "package/wavpack/Config.in"
>> +     source "package/x264/Config.in"
>
> I'd x264 is mostly a library, so I'd prefer to see it in the libraries.

Done.

>> diff --git a/package/x264/x264.mk b/package/x264/x264.mk
>> new file mode 100644
>> index 0000000..dff2313
>> --- /dev/null
>> +++ b/package/x264/x264.mk
>> @@ -0,0 +1,42 @@
>> +###############################################################
>> +#
>> +# x264
>> +#
>> +###############################################################
>> +
>> +X264_VERSION = 20140930-2245-stable
>> +X264_SOURCE = x264-snapshot-$(X264_VERSION).tar.bz2
>> +X264_SITE= ftp://ftp.videolan.org/pub/videolan/x264/snapshots
>> +X264_LICENSE = GPL
>
> The license is actually GPLv2+.

Done.

>> +X264_LICENSE_FILES = COPYING
>> +X264_INSTALL_STAGING = YES
>> +X264_INSTALL_TARGET = YES
>
> Line not needed.

Done.

>> +X264_DEPENDENCIES = host-pkgconf
>> +
>
> Might be worth adding a comment right before X264_CONFIGURE_CMDS to
> indicate that the configure script is not generated by autoconf.

Done.

>> +define X264_CONFIGURE_CMDS
>> +     (cd $(@D);./configure \
>> +             --prefix=/usr \
>> +             --host="$(GNU_TARGET_NAME)" \
>> +             --cross-prefix="$(TARGET_CROSS)" \
>> +             --enable-static \
>> +             --enable-strip \
>> +             --enable-pic \
>> +             --enable-shared \
>
> --enable-shared will not work for static only builds. Maybe you should
> try something such as:
>
> X264_CONF_OPTS = --enable-static
>
> ifeq ($(BR2_PREFER_STATIC_LIB),)
> X264_CONF_OPTS = --enable-pic --enable-shared
> endif
>
> and then use $(X264_CONF_OPTS) when calling the configure script.

Done. I've let --enable-static as is to always build the static library.

>> +             --disable-ffms \
>> +             --disable-cli \
>> +     )
>> +endef
>> +
>> +define X264_BUILD_CMDS
>> +     $(MAKE) CC="$(TARGET_CC)" -C $(@D)
>
> Could you try:
>
>         $(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D)
>
>  ?

I've chosen to put TARGET_CONFIGURE_OPTS before ./configure instead,
so it will not override the variables defined in config.mak.

> CC="$(TARGET_CC)" as well as many other useful variables are passed in
> $(TARGET_CONFIGURE_OPTS). It's also a good habit to pass
> $(TARGET_MAKE_ENV) in the environment.
>
>> +endef
>> +
>> +define X264_INSTALL_STAGING_CMDS
>> +     $(MAKE) DESTDIR="$(STAGING_DIR)" -C $(@D) install
>
> Also pass $(TARGET_MAKE_ENV).

Done.

>> +endef
>> +
>> +define X264_INSTALL_TARGET_CMDS
>> +     $(MAKE) DESTDIR="$(TARGET_DIR)" -C $(@D) install
>
> Ditto.

Done.

>> +endef
>> +
>> +$(eval $(generic-package))
>
> Could you respin after fixing those minor issues?

I'll send the updated patch.

-- 
David du Colombier