| Message ID | 1412154841-708-1-git-send-email-0intro@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear David du Colombier, On Wed, 1 Oct 2014 11:14:00 +0200, David du Colombier wrote: > This package is based on an earlier package > proposed by Ayaka in December 2013. Thanks for reviving this patch, definitely appreciated! 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. Some comments below. > 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. > 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+. > +X264_LICENSE_FILES = COPYING > +X264_INSTALL_STAGING = YES > +X264_INSTALL_TARGET = YES Line not needed. > +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. > +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. If you want to try a purely static configuration, you can try with the basic configuration at http://autobuild.buildroot.org/toolchains/configs/bfin-uclinux.config. > + --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) ? 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). > +endef > + > +define X264_INSTALL_TARGET_CMDS > + $(MAKE) DESTDIR="$(TARGET_DIR)" -C $(@D) install Ditto. > +endef > + > +$(eval $(generic-package)) Could you respin after fixing those minor issues? Thanks! Thomas
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.
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" source "package/xbmc/Config.in" source "package/yavta/Config.in" endmenu diff --git a/package/x264/Config.in b/package/x264/Config.in new file mode 100644 index 0000000..6ad2495 --- /dev/null +++ b/package/x264/Config.in @@ -0,0 +1,9 @@ +config BR2_PACKAGE_X264 + bool "x264" + help + x264 is a free software library and application for + encoding video streams into the H.264/MPEG-4 AVC + compression format, and is released under the terms + of the GNU GPL. + + https://www.videolan.org/developers/x264.html 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 +X264_LICENSE_FILES = COPYING +X264_INSTALL_STAGING = YES +X264_INSTALL_TARGET = YES +X264_DEPENDENCIES = host-pkgconf + +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 \ + --disable-ffms \ + --disable-cli \ + ) +endef + +define X264_BUILD_CMDS + $(MAKE) CC="$(TARGET_CC)" -C $(@D) +endef + +define X264_INSTALL_STAGING_CMDS + $(MAKE) DESTDIR="$(STAGING_DIR)" -C $(@D) install +endef + +define X264_INSTALL_TARGET_CMDS + $(MAKE) DESTDIR="$(TARGET_DIR)" -C $(@D) install +endef + +$(eval $(generic-package))
This package is based on an earlier package proposed by Ayaka in December 2013. Signed-off-by: David du Colombier <0intro@gmail.com> --- package/Config.in | 1 + package/x264/Config.in | 9 +++++++++ package/x264/x264.mk | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+) create mode 100644 package/x264/Config.in create mode 100644 package/x264/x264.mk