Created attachment 159969 [details] v1 Preparing to modernize mplayer2 port. - Fix OPTIONS - Use PLIST_FILES instead pkg-plist - Clean-up Makefile - Regenerate all patches.
Created attachment 159970 [details] v2 % portlint -ac WARN: Makefile: [180]: use of != in assignments is almost never a good thing to do. Try to avoid using them. See http://lists.freebsd.org/pipermail/freebsd-ports/2008-July/049777.html for some helpful hints on what to do instead. WARN: Makefile: possible use of absolute pathname "/dev/cd0". WARN: Makefile: no port directory ${CODEC_PORT} found, even though it is listed in RUN_DEPENDS. WARN: Consider to set DEVELOPER=yes in /etc/make.conf 0 fatal errors and 4 warnings found.
mplayer2 port inherited a lot of cruft from mplayer port. Try using |svn blame| on the parent. For example, CODEC_* was removed in ports r393396 but != usage predated bug 79823 thus the whole WIN32 conditional could be optimized into OPTIONS_DEFINE_i386+= WIN32 # Depend on RESTRICTED package when built manually .if !defined(PACKAGE_BUILDING) OPTIONS_DEFAULT_i386+= WIN32 .endif WIN32_DESC= Win32 codec pack support WIN32_RUN_DEPENDS= win32-codecs>0:${PORTSDIR}/multimedia/win32-codecs WIN32_CONFIGURE_ON= --codecsdir=${LOCALBASE}/lib/win32 WIN32_CONFIGURE_OFF= --disable-win32dll --disable-qtx X11 option can be optimized via ports r394573.
Created attachment 159998 [details] v3 # portlint -ac WARN: Makefile: possible use of absolute pathname "/dev/somedevice\". WARN: Consider to set DEVELOPER=yes in /etc/make.conf 0 fatal errors and 2 warnings found.
Almost all configuration options can be optimized.
Comment on attachment 159998 [details] v3 >-WITH_CDROM_DEVICE?= /dev/cd0 >+.if defined(WITH_CDROM_DEVICE) >+DEFAULT_CDROM_DEVICE=${WITH_CDROM_DEVICE} >+.else >+DEFAULT_CDROM_DEVICE=/dev/acd0 >+.endif /dev/acd0 is provided by atapicd(4) and was phased out by ATA_CAM transition. "device atapicd" was removed from GENERIC since 9.0-RELEASE while 8.x branch is officially EOL since 2015-08-01. And some lines still reference WITH_CDROM_DEVICE which now can be empty. > -.if ${PORT_OPTIONS:MLIBCACA} > +.if ${PORT_OPTIONS:MCACA} > LIB_DEPENDS+= libcaca.so:${PORTSDIR}/graphics/libcaca > .else > CONFIGURE_ARGS+= --disable-caca PORTREVISION bump is required for users who enabled CACA option but didn't notice lack of its effect. >++++ configure >+@@ -1234,7 +1234,7 @@ if test "$(basename $_cc)" = "icc" || te >+ esac >+ echores "$cc_version" >+ else >+- for _cc in "$_cc" gcc cc ; do >++ for _cc in "$_cc" cc cc ; do Before regenarating patches make sure there's no noise from changes caused by REINPLACE_CMD. >+PLIST_FILES= bin/mplayer \ >+ etc/mplayer/codecs.conf.sample \ >+ etc/mplayer/input.conf.sample \ >+ etc/mplayer/mplayer.conf.sample \ >+ man/man1/mplayer.1.gz >+ [...] >--- /usr/ports/multimedia/mplayer2.old/pkg-plist 2015-02-01 21:24:27.000000000 +0100 >+++ /usr/ports/multimedia/mplayer2/pkg-plist 1970-01-01 01:00:00.000000000 +0100 >@@ -1,5 +0,0 @@ >-bin/mplayer >-etc/mplayer/codecs.conf.sample >-etc/mplayer/input.conf.sample >-etc/mplayer/mplayer.conf.sample >-man/man1/mplayer.1.gz On one hand it introduces more churn for |svn blame|, on the other one less file to parse during build. Also, multimedia/mplayer still uses pkg-plist.
Comment 2 only gave you an idea for a complex option. Converting the rest and getting rid of .include line should be trivial -> your job, not reviewer's.
(In reply to Jan Beich from comment #6) Sure, it's my job :)
Created attachment 160285 [details] v4 There is a lot of OPTIONS broken :(
Comment on attachment 160285 [details] v4 > @@ -25,7 +25,6 @@ > CONFIGURE_ARGS= --cc=${CC} \ > --extra-libs='-lavresample ${PORTAUDIOLIB}' \ > --mandir=${PREFIX}/man \ > - --enable-libavresample \ What's the rationale? > +OPTIONS_DEFINE= ... V4L ... If it was previously commented out and didn't work then don't advertise it. Option helpers can be left uncommented as they wouldn't be used. > -.if ${PORT_OPTIONS:MIPV6} > -CATEGORIES+= ipv6 > -.else > -CONFIGURE_ARGS+= --disable-inet6 > -.endif [...] > +IPV6_CONFIGURE_ON= --enable-inet6 > +IPV6_CONFIGURE_OFF= --disable-inet6 CATEGORIES is lost? > +ASS_CONFIGURE_OFF= --disable-libass --disable-enca Make it ASS_IMPLIES=ENCA instead. > +CACA_CONFIGURE_ON= --enable-caca > +CACA_CONFIGURE_OFF= --disable-caca Better replace such instances with the likes of CACA_CONFIGURE_ENABLE= caca > +THEORA_EXTRA_LIBS= -ltheoradec > +V4L_EXTRA_LIBS= -lv4l2 [...] > +.if defined(THEORA_EXTRA_LIBS) > +CONFIGURE_ARGS+= --extra-libs="${THEORA_EXTRA_LIBS}" > +.endif > +.if defined(V4L_EXTRA_LIBS) > +CONFIGURE_ARGS+= --extra-libs="${V4L_EXTRA_LIBS}" > +.endif This isn't going to work. Each instance of --extra-libs overrides the previous one. Try using standard variable. CONFIGURE_ARGS+= --extra-libs="${LIBS}" [...] THEORA_LIBS= -ltheoradec V4L_LIBS= -lv4l2 > -.if ${PORT_OPTIONS:MX11} > -USE_XORG= x11 xv xxf86vm > -.if ${PORT_OPTIONS:MOPENGL} > -USE_XORG+= glproto > -.if ${PORT_OPTIONS:MXINERAMA} > -USE_XORG+= xinerama xineramaproto > +OPENGL_USE= GL=gl XORG=glproto > +XINERAMA_USE= XORG=xinerama,xineramaproto Lossy conversion. Where are _IMPLIES= X11 ? > .include <bsd.port.options.mk> This line should disappear. > > .if ${ARCH} == "sparc64" > BROKEN= Does not compile on sparc64 > .endif Convert into BROKEN_sparc64. > +--- configure.orig 2013-07-09 16:33:11 UTC > ++++ configure [...] > +- for _cc in "$_cc" gcc cc ; do > ++ for _cc in "$_cc" cc cc ; do Same mistake as v3. You're converting post-patch into files/patch-configure.
Created attachment 160300 [details] v5
Created attachment 160301 [details] v5
Build MPlayer2 fails with GIF option active because giflib version is unsupported.
Comment on attachment 160301 [details] v5 > + --disable-gif \ Have you checked if maybe mplayer contains the fix? $ svn log -v svn://svn.mplayerhq.hu/mplayer/trunk/libvo/vo_gif89a.c ------------------------------------------------------------------------ r37293 | al | 2014-10-05 06:39:08 +0400 (Sun, 05 Oct 2014) | 3 lines Changed paths: M /trunk/libmpdemux/demux_gif.c M /trunk/libvo/vo_gif89a.c Fix build for GIFLIB versions 5.1 and higher ------------------------------------------------------------------------ r36367 | al | 2013-07-28 01:16:06 +0400 (Sun, 28 Jul 2013) | 24 lines Changed paths: M /trunk/configure M /trunk/libmpdemux/demux_gif.c M /trunk/libvo/vo_gif89a.c Support newer GIFLIB versions Work with GIFLIB version >= 4.2 Several functions have been renamed or changed in signature. GIFLIB is used by vo gif89a and demuxer gif. Note about GIFLIB Version 4.2: It does not work with vanilla GIFLIB 4.2 but it works with versions that have re-added quantize support like e.g. the package from arch linux. Note about GIFLIB Version 5: The newly added GCB functions use size_t but the necessary standard headers are not included in gif_lib.h . To workaround this: * configure: use statement_check_broken to include stdlib.h * vo gif89: include gif_lib.h after stdlib.h * demuxer gif: no workaround needed, gif_lib.h is already included after stdlib.h > -RTC_DESC= Enable kernel realtime clock timing > -PULSE_DESC= Enable PulseAudio support > -THEORA_DESC= Enable ogg theora video support > ASS_DESC= Enable ASS/SSA subtitle rendering > -WIN32_DESC= Enable win32 codec set on the IA32 arch > -REALPLAYER_DESC=Enable realplayer plugin > -LIRC_DESC= Enable lirc support > +DVDREAD_DESC= DVD Playback support > +DVDNAV_DESC= DVD menu navigation > +ENCA_DESC= Enable encoding detection support > +LIBAV_DESC= Enable libav support > LIBCDIO_DESC= Enable libcdio support > +LIRC_DESC= Enable lirc support > +PULSE_DESC= Enable PulseAudio support > +REALPLAYER_DESC=Enable realplayer plugin > +RTC_DESC= Enable kernel realtime clock timing > +THEORA_DESC= Enable ogg theora video support Maybe standardize on option names and their descriptions using Mk/bsd.options.desc.mk. > +ASS_IMPLIES= ENCA mplayer2 builds fine with ASS=on ENCA=off. I've misunderstood what --disable-ass --disable-enca was supposed to accomplish. The line can go away. > +DV_CONFIGURE_ON= --enable-libdv > +DV_CONFIGURE_OFF= --disable-libdv > +IPV6_CONFIGURE_ON= --enable-inet6 > +IPV6_CONFIGURE_OFF= --disable-inet6 > +JACK_CONFIGURE_ON= --enable-jack > +JACK_CONFIGURE_OFF= --disable-jack [and many more] Do you have a reason to prefer _CONFIGURE_(ON|OFF) over _CONFIGURE_ENABLE? > +LIBAV_LIB_DEPENDS= libavresample.so:${PORTSDIR}/multimedia/libav > +LIBAV_CONFIGURE_ENABLE= libavresample > +LIBAV_LIBS= -lavresample Does this actually work? multimedia/libav alters its libraries' names to avoid conflict with multimedia/ffmpeg. pkg-config won't find them without extra hackery. $ pkg which -o /usr/local/lib/libavresample.so /usr/local/lib/libavresample.so was installed by package multimedia/ffmpeg
Jan, As always thanks for reviewing my mess :)
Let's also ask Thomas in assisting with review here. To an extent some of the cruft is still present in mplayer port.
I'm reworking the patch. Although, I noticed that build fails using ASS_CONFIGURE_ENABLE cc -MD -MP -Wall -Wno-switch-enum -Wno-logical-op-parentheses -Wpointer-arith -Wundef -Wno-pointer-sign -Wmissing-prototypes -Werror=implicit-function-declaration -D_ISOC99_SOURCE -D_BSD_SOURCE -O2 -march=core2 -pipe -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FILE_OFFSET_BITS=64 -I. -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -c -o sub/ass_mp.o sub/ass_mp.c In file included from sub/ass_mp.c:35: sub/ass_mp.h:53:16: error: redefinition of 'ass_image' typedef struct ass_image { ^ /usr/local/include/ass/ass.h:37:16: note: previous definition is here typedef struct ass_image { ^ In file included from sub/ass_mp.c:35: sub/ass_mp.h:60:3: error: typedef redefinition with different types ('struct ASS_Image' vs 'struct ass_image') } ASS_Image; ^ /usr/local/include/ass/ass.h:54:3: note: previous definition is here } ASS_Image; ^ but it passes the above error if I don't set ASS_CONFIGURE_ENABLE or ASS_CONFIGURE_ON target. ASS_LIB_DEPENDS= libass.so:${PORTSDIR}/multimedia/libass ASS_CONFIGURE_OFF= --disable-libass
Created attachment 160366 [details] v6
Please don't take offense here, but is it really worthwhile to keep mplayer2 around? Upstream seems abandoned, while mplayer and mpv are still actively developed.
(In reply to Thomas Zander from comment #18) Agreed. If nobody says otherwise, I will prepare mplayer2 to be removed.
Created attachment 160375 [details] v4l fix
Comment on attachment 160366 [details] v6 > + --disable-radio-v4l2 \ > + --disable-radio-bsdbt848 \ > + --disable-tv \ > + --disable-tv-v4l2 \ > + --disable-tv-bsdbt848 \ > + --disable-v4l2 \ These are trivial to unbreak, see attachment 160375 [details]. And radio is disabled by default, anyway. > @@ -270,17 +206,17 @@ pre-everything:: > > post-patch: > .if ${OSVERSION} >= 900010 This conditional is always true since 8.x is EOL since 1 August. Remove? > - @${REINPLACE_CMD} -e \ > + ${REINPLACE_CMD} -e \ > - @${REINPLACE_CMD} \ > + ${REINPLACE_CMD} \ [and others, since v4] Both variants are fine. Only INSTALL_* commands or those in *-install targets are required (by portlint) to be echoed. Actually, all examples in Porter's Handbook for REINPLACE_CMD have @ prepended. Unless you feel strongly the change adds unnecessary noise for |svn blame|. > +A52_LIBS= -la52 > +ASS_LIBS= -lass > +CACA_LIBS= -lcaca > +ENCA_LIBS= -lenca > +JACK_LIBS= -ljack [and many more] Looking at configure it has the following pattern which consistently fails to add -lfoo in --enable-foo case. I think, _foo=auto options are better left with only _CONFIGURE_OFF like before. Knowing which -lfoo to provide is an implementation detail and shouldn't be present in the port's Makefile. echocheck "FOO support" if test "$_foo" = auto ; then _foo=no if pkg_config_add foo ; then _foo=yes fi fi if test "$_foo" = yes; then def_foo="#define CONFIG_FOO 1" else def_foo="#undef CONFIG_FOO" fi echores "$_foo" mplayer's configure is no better and one such mistake slipped in the port. # multimedia/mplayer/Makefile.options .if ${PORT_OPTIONS:MRTMP} LIB_DEPENDS+= librtmp.so:${PORTSDIR}/multimedia/librtmp CONFIGURE_ARGS+= --enable-librtmp EXTRA_LIBS+= -lrtmp .else CONFIGURE_ARGS+= --disable-librtmp .endif should be .if ${PORT_OPTIONS:MRTMP} LIB_DEPENDS+= librtmp.so:${PORTSDIR}/multimedia/librtmp .else CONFIGURE_ARGS+= --disable-librtmp .endif or (via option helpers) RTMP_LIB_DEPENDS= librtmp.so:${PORTSDIR}/multimedia/librtmp RTMP_CONFIGURE_OFF= --disable-librtmp In mplayer2 example it'be reverting JACK_LIB_DEPENDS= libjack.so:${PORTSDIR}/audio/jack JACK_CONFIGURE_ENABLE= jack JACK_LIBS= -ljack back to .if ${PORT_OPTIONS:MJACK} LIB_DEPENDS+= libjack.so:${PORTSDIR}/audio/jack .else CONFIGURE_ARGS+= --disable-jack .endif or (via option helpers) JACK_LIB_DEPENDS= libjack.so:${PORTSDIR}/audio/jack JACK_CONFIGURE_OFF= --disable-jack > -LIBAV_LIB_DEPENDS= libavresample.so:${PORTSDIR}/multimedia/libav > +LIBAV_LIB_DEPENDS= libavresample.so:${PORTSDIR}/multimedia/ffmpeg > LIBAV_CONFIGURE_ENABLE= libavresample > LIBAV_LIBS= -lavresample ffmpeg is integral requirement like in multimedia/mpv. Just copy uppermost LIB_DEPENDS from there and drop LIBAV option.
Thanks Jan :) but the main question here is keep or remove mplayer2 port.
Created attachment 160377 [details] v7
Comment on attachment 160377 [details] v7 As ffmpeg CLI isn't actually used but its libraries are BUILD_DEPENDS= ffmpeg:${PORTSDIR}/multimedia/ffmpeg RUN_DEPENDS= ffmpeg:${PORTSDIR}/multimedia/ffmpeg can be converted to LIB_DEPENDS= libavcodec.so:${PORTSDIR}/multimedia/ffmpeg > +OPTIONS_DEFAULT= ASS ENCA LIBAV X11 ENCA and LIBAV are orphaned. LIBAV_* were removed in v7 while ASS_IMPLIES=ENCA in v6. > post-patch: > -.if ${OSVERSION} >= 900010 > - @${REINPLACE_CMD} -e \ > - '/CFLAGS.*-D_LARGEFILE64_SOURCE/ s/-D_LARGEFILE64_SOURCE/-D_FILE_OFFSET_BITS=64/' \ > - ${CONFIGURE_WRKSRC}/${CONFIGURE_SCRIPT} > -.endif riggs@ added the substitution in ports r253004, so he should probably review its removal. I've only suggested to remove .if guard. > @@ -261,6 +174,13 @@ DEFAULT_KERN_HZ=${WITH_KERN_HZ} > DEFAULT_KERN_HZ=1024 > .endif > +.include <bsd.port.options.mk> [...] > +BROKEN_sparc64= Does not compile on sparc64 [...] > +CONFIGURE_ARGS+= --extra-libs="${LIBS}" Why not concatenate value with the top CONFIGURE_ARGS definition? LIBS won't be evaluated before do-configure calls configure script after all options are already processed. > + > +# Depend on RESTRICTED package when built manually > +.if !defined(PACKAGE_BUILDING) > +OPTIONS_DEFAULT_i386+= WIN32 > +.endif > + Try to avoid putting anything below .include lines unless they don't work otherwise. For example, all those .if defined() expect variables defined in either environment, make.conf or command line, thus way before the port's Makefile. One exception is underdocumented Makefile.local. (In reply to Carlos J Puga Medina from comment #22) > but the main question here is keep or remove mplayer2 port. A dead project can stay in ports as long as there's a maintainer for it. Unmaintained code in libmpdemux/ and stream/ may cause a security concern but most parsing/decoding is done by ffmpeg libraries. That said, I agree about deprecation/removal as a former user of mplayer2. mpv seems like the continuation of the fast evolution of mplayer code started by Uoti. For a cooling-off period mark with DEPRECATED, otherwise use MOVED file to switch users from mplayer2 to either mpv or mplayer. DEPRECATED and EXPIRATION_DATE are invisible for package users and maybe unnoticed until periodic reminders are restored as the last one was from 2015-05-07. https://www.freebsd.org/doc/en/books/porters-handbook/dads-deprecated.html https://www.freebsd.org/doc/en/books/porters-handbook/moved-and-updating-files.html#moved-and-updating-moved https://lists.freebsd.org/pipermail/freebsd-ports/2015-May/099082.html
Created attachment 160401 [details] v8
> + > +# Depend on RESTRICTED package when built manually > +.if !defined(PACKAGE_BUILDING) > +OPTIONS_DEFAULT_i386+= WIN32 > +.endif > + > Try to avoid putting anything below .include lines unless they don't work > otherwise. For example, all those .if defined() expect variables defined > in either environment, make.conf or command line, thus way before the > port's Makefile. One exception is underdocumented Makefile.local. Regarding this comment, it's necessary to leave it as is. Otherwise, there it problems.
GIF fix is incomplete. |make configure| shows Checking for GIF support ... no (In reply to Carlos J Puga Medina from comment #26) > Otherwise, there it problems. Such as? OPTIONS_* cannot be altered after bsd.port.options.mk. $ make -V PORT_OPTIONS ARCH=i386 WIN32 ASS IPV6 VDPAU X11 $ make -V CONFIGURE_ARGS ARCH=i386 | sed 'y/ /\n/' | fgrep win32 --disable-win32dll $ make -V CONFIGURE_ARGS ARCH=i386 WITH=WIN32 | sed 'y/ /\n/' | fgrep win32 --codecsdir=/usr/local/lib/win32
A commit references this bug: Author: jbeich Date: Thu Aug 27 16:58:55 UTC 2015 New revision: 395434 URL: https://svnweb.freebsd.org/changeset/ports/395434 Log: multimedia/mplayer2: slightly improve options - Make CACA=on actually work - Convert PULSE and LIBCDIO to standard spelling - Drop option descriptions where standardized PR: 202404 Submitted by: Carlos J Puga Medina <cpm@fbsd.es> (maintainer) MFH: 2015Q3 Changes: head/multimedia/mplayer2/Makefile
A commit references this bug: Author: jbeich Date: Thu Aug 27 16:59:13 UTC 2015 New revision: 395435 URL: https://svnweb.freebsd.org/changeset/ports/395435 Log: multimedia/mplayer2: minor cleanup via portlint - Unsilence INSTALL_* lines - Regenerate patches with makepatch - Remove extra newline PR: 202404 Submitted by: Carlos J Puga Medina <cpm@fbsd.es> (maintainer) Changes: head/multimedia/mplayer2/Makefile head/multimedia/mplayer2/files/patch-configure head/multimedia/mplayer2/files/patch-libao2-ao_oss.c head/multimedia/mplayer2/files/patch-libmpcodecs-vd_theora.c head/multimedia/mplayer2/files/patch-libmpdemux-demux_gif.c head/multimedia/mplayer2/files/patch-libmpdemux-demux_ogg.c head/multimedia/mplayer2/files/patch-libmpdemux-demuxer.h head/multimedia/mplayer2/files/patch-stream-tvi_bsdbt848.c head/multimedia/mplayer2/files/patch-stream-tvi_v4l2.c head/multimedia/mplayer2/files/patch-sub-subassconvert.c head/multimedia/mplayer2/files/patch-sub-subreader.c
A commit references this bug: Author: jbeich Date: Thu Aug 27 16:59:26 UTC 2015 New revision: 395436 URL: https://svnweb.freebsd.org/changeset/ports/395436 Log: multimedia/mplayer2: respect LIBS While here clean up unnecessary -lfoo due to --enable-foo not populating --extra-libs. PR: 202404 Submitted by: Carlos J Puga Medina <cpm@fbsd.es> (maintainer) Changes: head/multimedia/mplayer2/Makefile
A commit references this bug: Author: jbeich Date: Thu Aug 27 16:59:41 UTC 2015 New revision: 395437 URL: https://svnweb.freebsd.org/changeset/ports/395437 Log: multimedia/mplayer2: unbreak/restore V4L option PR: 202404 Submitted by: Carlos J Puga Medina <cpm@fbsd.es> (maintainer) Changes: head/multimedia/mplayer2/Makefile head/multimedia/mplayer2/files/patch-configure head/multimedia/mplayer2/files/patch-libvo_vo__v4l2.c head/multimedia/mplayer2/files/patch-stream_stream__pvr.c head/multimedia/mplayer2/files/patch-stream_stream__radio.c
I've landed easy changes. Please, rebase. Option helpers need more work i.e., not complete until bsd.port.options.mk line is gone. For one, I'd kill the following block without mercy: #On i386, gcc runs out of general purpose registers when #trying to compile a debug version with the default flags. .if ${PORT_OPTIONS:MDEBUG} .if ${ARCH} == "i386" DEBUG_FLAGS= -g -O -fomit-frame-pointer .endif .else .if defined(PACKAGE_BUILDING) CONFIGURE_ARGS+= --enable-runtime-cpudetection CFLAGS+= -O2 -fomit-frame-pointer .else CONFIGURE_ENV+= CPPFLAGS= CFLAGS= LDFLAGS= .endif .endif o Compiler-specific hacks should be properly enlosed. gcc42 is not used by default since FreeBSD 10. Convert to COMPILER_VERSION if the statement in the comment is still correct. o -O2 is in CFLAGS by default o -fomit-frame-pointer is harmful both for profiling and debugging o --enable-runtime-cpudetection should be converted to RTCPU, see multimedia/mplayer o Erasing user-provided *FLAGS is a foot-shooting prevention. We have packages for those who cannot properly sanitize their environment. See Porter's Handbook chapter on respecting CFLAGS.
OK, I need to sanitize this before remove .include <bsd.port.options.mk> line. .if ${PORT_OPTIONS:MRTC} @${REINPLACE_CMD} -e \ 's|irqp = 1024|irqp = ${DEFAULT_KERN_HZ}|' \ ${WRKSRC}/mplayer.c .endif Any thoughts?
Created attachment 160430 [details] v9
GIF option enabled should work now.
A commit references this bug: Author: jbeich Date: Fri Aug 28 10:32:06 UTC 2015 New revision: 395460 URL: https://svnweb.freebsd.org/changeset/ports/395460 Log: multimedia/mplayer2: make GIF=on actually work GIF option was both auto-disabled during configure and broken when forced to be enabled. So, bump PORTREVISION to restore GIF support for users with GIF=on. PR: 202404 Submitted by: Carlos J Puga Medina <cpm@fbsd.es> (maintainer) MFH: 2015Q3 Changes: head/multimedia/mplayer2/Makefile head/multimedia/mplayer2/files/patch-libmpdemux-demux_gif.c head/multimedia/mplayer2/files/patch-libvo_vo_gif89a.c
Comment on attachment 160430 [details] v9 Let me try to enumerate what haven't landed yet. I wonder if a single blob commit is OK. - Expose A52, ENCA, DVDREAD, DVDNAV, RTCPU as options - Hide WIN32 option on non-i386 architectures - Drop no longer supported OSVERSION range - Drop DEBUG and PACKAGE_BUILDING hacks - Convert to option helpers - Sort options > + --disable-liba52 \ I thought you've converted it into A52 option since v6. > -ENCA_LIB_DEPENDS= libenca.so:${PORTSDIR}/converters/enca > -ENCA_CONFIGURE_OFF= --disable-enca Why remove since v8? mplayer2 directly links against libenca with ASS=on due to libass package having ENCA=on by default. Hidden dependencies make build unpredictable and may cause issues e.g., - libenca ABI changed -> bump PORTREVISION of consumers - libass is rebuilt with ENCA=off then libenca can be pruned by |pkg autoremove| Maybe also make ENCA=on by default to keep the feature intact for users of freebsd.org packages. >+OPTIONS_DEFINE= A52 ASS CACA DEBUG DV DVDREAD DVDNAV GIF IPV6 JACK LADSPA \ >+ LIBBLURAY LIBCDIO LIRC MAD OPENGL PORTAUDIO PULSEAUDIO \ >+ REALPLAYER RTC RTCPU SDL SMB SPEEX THEORA V4L VDPAU X11 XINERAMA The option is guarded in configure, so you need to provide it only for specific architectures. And it may need to be enabled by default to keep the feature for packages users intact. if test "$_runtime_cpudetection" = yes && ! x86 && ! ppc; then die "Runtime CPU detection only works for x86, x86-64 and PPC!" fi See below WIN32 example but ignore !PACKAGE_BUILDING guard. > .if !defined(PACKAGE_BUILDING) >+OPTIONS_DEFINE_i386+= WIN32 >+OPTIONS_DEFAULT_i386+= WIN32 > .endif > -WIN32_DESC= Win32 codec pack support > +WIN32_DESC= Enable win32 codec Why? "Enable" is a noise word and can be removed from local descriptions for other options as well (as part of options cleanup). "win32" is not a codec but a "set" or "pack" of them. Original description used "codec set" but "codec pack" is more popular term on Windows. WIN32_DESC= Enable win32 codec set on the IA32 arch > +LIBCDIO_DESC= Enable libcdio support [...] > +LIBCDIO_LIB_DEPENDS= libcdio_paranoia.so:${PORTSDIR}/sysutils/libcdio-paranoia > +LIBCDIO_CONFIGURE_OFF= --disable-libcdio I've renamed LIBCDIO to CDIO in ports r395434. This was supposed to be part of your v6 as CDIO_DESC is already defined in Mk/bsd.options.desc.mk. Please, rebase carefully. >-.if defined(WITH_KERN_HZ) >-DEFAULT_KERN_HZ=${WITH_KERN_HZ} >-.else >-DEFAULT_KERN_HZ=1024 >-.endif WITH_KERN_HZ still referenced later in Makefile and would break build if the assignment not restored or the reference not removed. And per comment 24 the conditional will work just fine without bsd.port.options.mk line. mplayer.c:4082:34: error: expected expression unsigned long irqp = ; /* 512 seemed OK. 128 is jerky. */ ^ Note, if you're removing a feature provide rationale to be used in commit message. It doesn't need to be more than one sentence long but should explain "why" and not "what" was done. (In reply to Carlos J Puga Medina from comment #33) > OK, I need to sanitize this before remove .include <bsd.port.options.mk> line. > > .if ${PORT_OPTIONS:MRTC} Use target option helpers. They're documented in /usr/ports/CHANGES from 20150701 or https://www.freebsd.org/doc/en/books/porters-handbook/makefile-options.html#options-targets-on
Created attachment 160445 [details] v10 # portlint -ac WARN: /usr/ports/multimedia/mplayer2/pkg-plist: There are only 5 items in the plist. Consider using PLIST_FILES instead of pkg-plist when installing less than 6 items. WARN: Consider to set DEVELOPER=yes in /etc/make.conf 0 fatal errors and 2 warnings found.
Jan, Thanks for your kindly support.
A commit references this bug: Author: jbeich Date: Sat Aug 29 21:47:27 UTC 2015 New revision: 395569 URL: https://svnweb.freebsd.org/changeset/ports/395569 Log: multimedia/mplayer2: modernize options - Expose A52, ENCA, DVDREAD, DVDNAV, RTCPU as options - Hide WIN32 option for non-i386 architectures - Drop no longer supported OSVERSION range - Drop DEBUG and PACKAGE_BUILDING hacks - Convert to option helpers - Sort options PR: 202404 Submitted by: Carlos J Puga Medina <cpm@fbsd.es> (maintainer) Changes: head/multimedia/mplayer2/Makefile
Thanks for patience. Committed. I've fixed the rest myself. > -.if !defined(PACKAGE_BUILDING) > OPTIONS_DEFINE_i386+= WIN32 > OPTIONS_DEFAULT_i386+= WIN32 > -.endif win32-codecs is a RESTRICTED package thus will make any port that depends on it broken due to unsatisfied dependency. The check originates from bug 115170 for mplayer. _OFF option helper won't trigger if the option isn't defined via _DEFINE*, _SINGLE, etc. That's why I've moved OPTIONS_DEFINE_i386 outside of PACKAGE_BUILDING in comment 2. Thus package cluster makes sure to provide --disable-win32dll when building for i386, the only architecture where configure tries to auto-enable it. > OPTIONS_DEFAULT=... RTCPU ... Did you check on either arm*, aarch64, mips*, ia64, sparc64 ? --enable-runtime-cpudetection is only supported on x86 and ppc which translates to i386, amd64, powerpc, powerpc64 on FreeBSD. On other architectures configure calls die() i.e., exits with error message quoted in comment 37. > -V4L_LIBS= -lv4l2 > +V4L_LIBS= -lv4l1 -lv4l2 libv4l2 usage is inflicted by files/patch-stream-tvi_v4l2.c. Upstream only uses <linux/videodev2.h> which is part of V4L2 API. Nothing in mplayer2 uses <linux/videodev.h> aka V4L1 API not to mention <libv4l1.h>. Explain if you think otherwise. > -.if ${PORT_OPTIONS:MREALPLAYER} > -RUN_DEPENDS+= realplay:${PORTSDIR}/multimedia/linux-realplayer > -BUILD_DEPENDS+= realplay:${PORTSDIR}/multimedia/linux-realplayer > -.else > -CONFIGURE_ARGS+= --disable-real > -.endif > +REALPLAYER_BUILD_DEPENDS=realplay:${PORTSDIR}/multimedia/linux-realplayer > +REALPLAYER_CONFIGURE_OFF=--disable-real RUN_DEPENDS are lost in conversion. > WITH_CDROM_DEVICE?= /dev/cd0 This is still referenced. > -.if defined(WITH_KERN_HZ) > -DEFAULT_KERN_HZ=${WITH_KERN_HZ} > -.else > -DEFAULT_KERN_HZ=1024 > -.endif Did you ignore the build error I reported in comment 37 ? > - @${REINPLACE_CMD} -e \ > - 's|irqp = 1024|irqp = ${DEFAULT_KERN_HZ}|' \ > - ${WRKSRC}/mplayer.c > + @${REINPLACE_CMD} -e \ > + 's|irqp = 1024|irqp = ${DEFAULT_KERN_HZ}|' \ > + ${WRKSRC}/mplayer.c Pointless reindenting causes noise for |svn blame|.
Created attachment 160503 [details] pre-commit cleanup Here're changes since v10 that landed for reference.
All is clear now. I learned a lot while you've supervised my patches. Thanks again, Jan!
A commit references this bug: Author: jbeich Date: Sat Aug 29 21:58:28 UTC 2015 New revision: 395575 URL: https://svnweb.freebsd.org/changeset/ports/395575 Log: MFH: r395434 multimedia/mplayer2: slightly improve options - Make CACA=on actually work - Convert PULSE and LIBCDIO to standard spelling - Drop option descriptions where standardized PR: 202404 Submitted by: Carlos J Puga Medina <cpm@fbsd.es> (maintainer) Approved by: ports-secteam (delphij) Changes: _U branches/2015Q3/ branches/2015Q3/multimedia/mplayer2/Makefile
A commit references this bug: Author: jbeich Date: Sat Aug 29 22:07:26 UTC 2015 New revision: 395576 URL: https://svnweb.freebsd.org/changeset/ports/395576 Log: MFH: r395460 multimedia/mplayer2: make GIF=on actually work GIF option was both auto-disabled during configure and broken when forced to be enabled. So, bump PORTREVISION to restore GIF support for users with GIF=on. PR: 202404 Submitted by: Carlos J Puga Medina <cpm@fbsd.es> (maintainer) Approved by: ports-secteam (delphij) Changes: _U branches/2015Q3/ branches/2015Q3/multimedia/mplayer2/Makefile branches/2015Q3/multimedia/mplayer2/files/patch-libmpdemux-demux_gif.c branches/2015Q3/multimedia/mplayer2/files/patch-libvo_vo_gif89a.c