Bug 251317 - audio/musicpd: Adjustments to port
Summary: audio/musicpd: Adjustments to port
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Thomas Zander
URL:
Keywords:
Depends on: 251305
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-22 18:02 UTC by Daniel Engberg
Modified: 2020-12-27 19:13 UTC (History)
1 user (show)

See Also:
riggs: maintainer-feedback+


Attachments
Patch for musicpd (4.81 KB, patch)
2020-11-22 18:02 UTC, Daniel Engberg
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Engberg freebsd_committer 2020-11-22 18:02:00 UTC
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)
Comment 1 Daniel Engberg freebsd_committer 2020-11-22 18:08:51 UTC
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.
Comment 2 Thomas Zander freebsd_committer 2020-11-28 16:56:08 UTC
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?
Comment 3 Daniel Engberg freebsd_committer 2020-11-28 23:03:42 UTC
(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.
Comment 4 Thomas Zander freebsd_committer 2020-12-05 15:44:56 UTC
(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.
Comment 5 Daniel Engberg freebsd_committer 2020-12-05 15:48:39 UTC
Sounds good to me, thanks :-)
One more day until maintainer timeout regarding PR 251305
Comment 6 commit-hook freebsd_committer 2020-12-06 10:39:42 UTC
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
Comment 7 Daniel Engberg freebsd_committer 2020-12-06 12:25:31 UTC
Hi,

CXXFLAGS+=	-D_LIBCPP_ENABLE_CXX17_REMOVED_AUTO_PTR 
is needed for the SIDPLAY plugin to compile

Best regards,
Daniel
Comment 8 Daniel Engberg freebsd_committer 2020-12-06 12:28:28 UTC
...and according to "Table 5.2. Package Naming Examples" in Porter's handbook we should use DISTVERSION not PORTVERSION.

Best regards,
Daniel
Comment 9 Thomas Zander freebsd_committer 2020-12-06 15:41:04 UTC
(In reply to daniel.engberg.lists from comment #8)

Yes, we can change during one of the follow-up commits.
Comment 10 Thomas Zander freebsd_committer 2020-12-06 16:13:34 UTC
(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.
Comment 11 Daniel Engberg freebsd_committer 2020-12-06 17:01:01 UTC
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+).
Comment 12 Daniel Engberg freebsd_committer 2020-12-06 18:20:58 UTC
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.
Comment 13 Daniel Engberg freebsd_committer 2020-12-09 20:24:40 UTC
fwiw, 251305 can now be committed due to maintainer timeout.
Comment 14 Daniel Engberg freebsd_committer 2020-12-26 17:46:02 UTC
@ Thomas,
Can we do the final switch now? CXXFLAGS is also needed to build

Best regards,
Daniel
Comment 15 Thomas Zander freebsd_committer 2020-12-27 11:02:44 UTC
(In reply to daniel.engberg.lists from comment #14)

Thanks for the reminder. I'll take a look today.
Comment 16 commit-hook freebsd_committer 2020-12-27 15:49:55 UTC
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