Created attachment 219892 [details] Patch for musicpd Replace PORTVERSION with DISTVERSION Use USES= localbase:ldflags instead of setting C/CPP/LDFLAGS manually Replace unmaintained (upstream) libsidplay2 (SIDPLAY2) library with libsidplayfp Make OSS support optional Remove ID3TAG from default option as FFmpeg supports ID3 tags Set LAME as default option as it gets pulled in by FFmpeg as of r555166 References: https://www.freebsd.org/doc/en/books/porters-handbook/book.html#idp46911864 Compile tested on FreeBSD 13.0-CURRENT #0 r367711 (amd64) Compile and run-time tested on FreeBSD 13.0-CURRENT #3 0381b3d8be3-c272567(master)-dirty (arm64) Poudriere testport OK 12.2-RELEASE (amd64)
Unless there's a very strong argument which I'm unaware of I think it's reasonable to drop vorbis support as default option and later in FFmpeg since MP3 is much more versatile.
Why would we make OSS support optional? This is FreeBSD, we have it in the base system, it does not draw additional dependencies? Also I don't understand the comment on vorbis. Are you saying we should disable it vorbis support in mpd since we have it in ffmpeg by default?
(In reply to Thomas Zander from comment #2) I don't see any reason why we shouldn't, we don't hardcode base OpenSSL for every port just because it's supported and so on. What I suggest is that we should evaluate if Vorbis support needs be default as it's more or less superseded by Opus (enabled by default in FFmpeg), LAME in terms of overall support (also enabled by default in FFmpeg) and futher down the line in FFmpeg itself. In general I think we should try to pull in as few dependencies as possible without (if possible) impacting overall user experience. That being said, Vorbis isn't that much of a deal in terms of size and it's pretty quick to compile. If there are any strong opinions about keeping it that's fine with me.
(In reply to daniel.engberg.lists from comment #3) I agree with your points on LAME and OPUS. Now that we have a lame binary package, it should be included by default. I'd argue it makes sense for LAME, OPUS and VORBIS to be enabled by default. Since FFMPEG is also enabled by default, OPUS and VORBIS would not cause additional packages to be installed. I prefer to keep the OSS part as it is, though. I don't see what we gain of introducing this OPTION, but it would increase the number of OPTIONS and maybe more build failures when folks are customising their builds. I'll update said parts of the port. We can update the dependency on sidplay as soon as bug 251305 is resolved.
Sounds good to me, thanks :-) One more day until maintainer timeout regarding PR 251305
A commit references this bug: Author: riggs Date: Sun Dec 6 10:39:18 UTC 2020 New revision: 557129 URL: https://svnweb.freebsd.org/changeset/ports/557129 Log: Include LAME and OPUS by default. USES localbase instead of LDFLAGS. PR: 251317 Reported by: daniel.engberg.lists@pyret.net Changes: head/audio/musicpd/Makefile
Hi, CXXFLAGS+= -D_LIBCPP_ENABLE_CXX17_REMOVED_AUTO_PTR is needed for the SIDPLAY plugin to compile Best regards, Daniel
...and according to "Table 5.2. Package Naming Examples" in Porter's handbook we should use DISTVERSION not PORTVERSION. Best regards, Daniel
(In reply to daniel.engberg.lists from comment #8) Yes, we can change during one of the follow-up commits.
(In reply to daniel.engberg.lists from comment #7) Is this after the change to sidplayfp? Or is this a FreeBSD-13 issue? Currently, the sidplay plugin seems to build fine for me.
I'm pretty sure it's a clang (11) issue, the error occurs in musicpd's code. I can have a look on my buildbox but it'll be until tomorrow since it wants to recompile LLVM which takes multiple hours (4+).
FAILED: src/decoder/plugins/libdecoder_plugins.a.p/SidplayDecoderPlugin.cxx.o c++ -Isrc/decoder/plugins/libdecoder_plugins.a.p -Isrc/decoder/plugins -I../src/decoder/plugins -Isrc -I../src -I. -I.. -I/usr/local/include -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Wpedantic -std=c++17 -O3 -Wvla -Wdouble-promotion -fvisibility=hidden -ffast-math -ftree-vectorize -fno-threadsafe-statics -fmerge-all-constants -Wmissing-declarations -Wshadow -Wpointer-arith -Wcast-qual -Wwrite-strings -Wsign-compare -Wcomma -Wextra-semi -Wheader-hygiene -Winconsistent-missing-destructor-override -Wunreachable-code-break -Wunused -Wused-but-marked-unused -Wno-non-virtual-dtor -funwind-tables -ffunction-sections -fdata-sections -D_GNU_SOURCE -O2 -march=ivybridge -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing -isystem /usr/local/include -isystem /usr/local/include -fPIC -MD -MQ src/decoder/plugins/libdecoder_plugins.a.p/SidplayDecoderPlugin.cxx.o -MF src/decoder/plugins/libdecoder_plugins.a.p/SidplayDecoderPlugin.cxx.o.d -o src/decoder/plugins/libdecoder_plugins.a.p/SidplayDecoderPlugin.cxx.o -c ../src/decoder/plugins/SidplayDecoderPlugin.cxx In file included from ../src/decoder/plugins/SidplayDecoderPlugin.cxx:46: /usr/local/include/sidplayfp/SidTune.h:48:10: error: no template named 'auto_ptr' in namespace 'std' std::auto_ptr<SidTuneBase> tune; ~~~~~^ 1 error generated. 13-CURRENT, doesn't matter what version of sidplayfp is being used.
fwiw, 251305 can now be committed due to maintainer timeout.
@ Thomas, Can we do the final switch now? CXXFLAGS is also needed to build Best regards, Daniel
(In reply to daniel.engberg.lists from comment #14) Thanks for the reminder. I'll take a look today.
A commit references this bug: Author: riggs Date: Sun Dec 27 15:49:15 UTC 2020 New revision: 559366 URL: https://svnweb.freebsd.org/changeset/ports/559366 Log: Rename OPTION to SIDPLAY, depend on audio/libsidplayfp. PR: 251317 Reported by: daniel.engberg.lists@pyret.net Changes: head/audio/musicpd/Makefile