Bug 214184 - audio/amarok-kde4: fails to build with ffmpeg 3.x
Summary: audio/amarok-kde4: fails to build with ffmpeg 3.x
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: kde
URL:
Keywords: needs-qa, patch
Depends on:
Blocks: 207547
  Show dependency treegraph
 
Reported: 2016-11-04 04:07 UTC by Jan Beich
Modified: 2016-12-09 11:43 UTC (History)
4 users (show)

See Also:
rakuco: maintainer-feedback+


Attachments
fix amarok-kde4 build with ffmpeg3 (6.36 KB, patch)
2016-11-04 16:24 UTC, Matthew Rezny
no flags Details | Diff
fix amarok-kde4 build with ffmpeg3 (7.58 KB, patch)
2016-11-04 17:51 UTC, Matthew Rezny
no flags Details | Diff
fix MTP option in amarok-kde4 (463 bytes, patch)
2016-12-05 21:06 UTC, Matthew Rezny
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Beich freebsd_committer 2016-11-04 04:07:06 UTC
src/musicbrainz/MusicDNSAudioDecoder.cpp:203:51: warning: 'codec' is deprecated [-Wdeprecated-declarations]
    pCodecCtx = pFormatCtx->streams[audioStream]->codec;
                                                  ^
/usr/local/include/libavformat/avformat.h:880:21: note: 'codec' declared here
    AVCodecContext *codec;
                    ^
src/musicbrainz/MusicDNSAudioDecoder.cpp:228:36: error: use of undeclared identifier 'avcodec_alloc_frame'
                    decodedFrame = avcodec_alloc_frame();
                                   ^
src/musicbrainz/MusicDNSAudioDecoder.cpp:236:25: error: use of undeclared identifier 'avcodec_get_frame_defaults'
                        avcodec_get_frame_defaults( decodedFrame );
                        ^
src/musicbrainz/MusicDNSAudioDecoder.cpp:239:30: warning: 'avcodec_decode_audio4' is deprecated [-Wdeprecated-declarations]
                decoderRet = avcodec_decode_audio4( pCodecCtx, decodedFrame, &gotFrame, &avpkt );
                             ^
/usr/local/include/libavcodec/avcodec.h:4704:5: note: 'avcodec_decode_audio4' declared here
int avcodec_decode_audio4(AVCodecContext *avctx, AVFrame *frame,
    ^
src/musicbrainz/MusicDNSAudioDecoder.cpp:262:9: warning: 'av_free_packet' is deprecated [-Wdeprecated-declarations]
        av_free_packet( &packet );
        ^
/usr/local/include/libavcodec/avcodec.h:4414:6: note: 'av_free_packet' declared here
void av_free_packet(AVPacket *pkt);
     ^
src/musicbrainz/MusicDNSAudioDecoder.cpp:268:5: warning: 'av_free_packet' is deprecated [-Wdeprecated-declarations]
    av_free_packet( &avpkt );
    ^
/usr/local/include/libavcodec/avcodec.h:4414:6: note: 'av_free_packet' declared here
void av_free_packet(AVPacket *pkt);
     ^
4 warnings and 2 errors generated.

http://package23.nyi.freebsd.org/data/103i386-default-PR207547/2016-10-04_18h24m30s/logs/errors/amarok-2.8.0_6.log
Comment 1 Tobias C. Berner freebsd_committer 2016-11-04 08:18:24 UTC
Probably: https://git.reviewboard.kde.org/r/126682
Comment 2 Matthew Rezny freebsd_committer 2016-11-04 16:24:57 UTC
Created attachment 176630 [details]
fix amarok-kde4 build with ffmpeg3

The patch from KDE ReviewBoard works as-is, but while verifying that I could not help but notice a few other issues with this port that are also fixed in the attached patch.

* eliminate obnoxious delay-inducing Makefile warnings my migrating USE_MYSQL to USES=mysql and adding LICENSE
* correct capitalization of the WITH_MP3Tunes option so it works as intended
* remove the MTP option and make libmtp mandatory since it is not actually optional
* add a bunch on missing USE_XORG and other similar bits identified by stage-qa
Comment 3 Matthew Rezny freebsd_committer 2016-11-04 17:51:37 UTC
Created attachment 176638 [details]
fix amarok-kde4 build with ffmpeg3

Refresh the patch to fix the plist after removing MTP option.
Comment 4 commit-hook freebsd_committer 2016-12-05 16:27:47 UTC
A commit references this bug:

Author: rakuco
Date: Mon Dec  5 16:27:39 UTC 2016
New revision: 427906
URL: https://svnweb.freebsd.org/changeset/ports/427906

Log:
  Switch to USES=mysql:embedded

  PR:		214184
  Submitted by:	matthew@reztek.cz

Changes:
  head/audio/amarok-kde4/Makefile
Comment 5 commit-hook freebsd_committer 2016-12-05 17:46:52 UTC
A commit references this bug:

Author: rakuco
Date: Mon Dec  5 17:46:22 UTC 2016
New revision: 427909
URL: https://svnweb.freebsd.org/changeset/ports/427909

Log:
  Add several missing dependencies to the port.

  `make stage-qa' was complaining about a lot of missing dependencies:
  - Amarok actually links against MySQL, so we need USES=mysql, not
    USES=mysql:embedded.
  - Add USE_KDE=soprano and USE_GL=gl.
  - Add several missing X11 dependencies that were being pulled indirectly.
  - The IPOD option causes the iPod plugin to link against a few other libraries
    via libgpod-1.0.pc, so add them.
  - The MP3TUNES option needs either libgcrypt or OpenSSL; explicitly disable
    libgcrypt and add USES=ssl as required.
  - Fix a capitalization typo in MP3TUNES_CMAKE_OFF that was preventing the
    MP3Tunes code from being properly disabled in CMake.

  Based on an initial patch sent by Matthey Rezny <matthew@reztek.cz>.

  PR:		214184
  MFH:		2016Q4

Changes:
  head/audio/amarok-kde4/Makefile
Comment 6 commit-hook freebsd_committer 2016-12-05 17:49:57 UTC
A commit references this bug:

Author: rakuco
Date: Mon Dec  5 17:48:58 UTC 2016
New revision: 427911
URL: https://svnweb.freebsd.org/changeset/ports/427911

Log:
  Stop using deprecated FFMPEG calls in the code.

  This allows the port to be built with the upcoming FFMPEG 3.x series while
  still maintaining compatibility with FFMPEG 2.x that is currently in the ports
  tree.

  I don't think PORTREVISION needs to be bumped here, as both the old and new
  calls exist in FFMPEG 2.x, and when FFMPEG 3 lands there will be a PORTREVISION
  bump anyway.

  PR:		214184
  Submitted by:	Matthey Rezny <matthew@reztek.cz>

Changes:
  head/audio/amarok-kde4/files/patch-kde_rb-126682
Comment 7 Raphael Kubo da Costa freebsd_committer 2016-12-05 17:52:11 UTC
ReviewBoard patch committed, thanks.

As you can see, I've split the changes into separate commits so I can backport some of them to the quarterly branch.

Compared to your proposed patch, I've omitted the LICENSE part, as Amarok is actually licensed under the GPLv2+ and LGPLv2.1+ at least. USES=execinfo was also not used anywhere in the code, and the MTP option seems to be working just fine (I've built the port with it enabled and disabled).
Comment 8 Matthew Rezny freebsd_committer 2016-12-05 21:06:33 UTC
Created attachment 177698 [details]
fix MTP option in amarok-kde4

I had thought MTP was not optional because there is no WITH_MTP switch in the CMakeLists.txt as there is for the other options. tcberner suggested a way to correct the option by disabling the search for the optional package. I have tested this change on a live system with libmtp present to confirm it now does not find and enable MTP when the option is off.
Comment 9 Raphael Kubo da Costa freebsd_committer 2016-12-06 08:44:18 UTC
(In reply to matthew from comment #8)
> I had thought MTP was not optional because there is no WITH_MTP switch in
> the CMakeLists.txt as there is for the other options.

There is, it's just added indirectly. Amarok's top-level CMakeLists.txt has this:

    macro_optional_find_package(Mtp)

which is defined here:

https://cgit.kde.org/kdelibs.git/tree/cmake/modules/MacroOptionalFindPackage.cmake?h=KDE%2F4.14

As the comment there says, "this macro is a combination of OPTION() and FIND_PACKAGE(), it works like FIND_PACKAGE(), but additionally it automatically creates an option name WITH_<name>, which can be disabled via the cmake GUI or via -DWITH_<name>=OFF", so WITH_Mtp should work as expected -- in fact, I've built the port on Poudriere with the MTP option disabled and the logs show src/core-impl/collections/mtpcollection is not being built and nothing is linking against libmtp.

If you feel like something still needs to be patched, please file a new PR so we don't mix FFMPEG 3 support with other adjustments to the port, especially now that this bug has already been closed.
Comment 10 commit-hook freebsd_committer 2016-12-07 16:44:14 UTC
A commit references this bug:

Author: rakuco
Date: Wed Dec  7 16:43:52 UTC 2016
New revision: 428077
URL: https://svnweb.freebsd.org/changeset/ports/428077

Log:
  MFH: r427906 r427909

  Switch to USES=mysql:embedded

  PR:		214184
  Submitted by:	matthew@reztek.cz

  Add several missing dependencies to the port.

  `make stage-qa' was complaining about a lot of missing dependencies:
  - Amarok actually links against MySQL, so we need USES=mysql, not
    USES=mysql:embedded.
  - Add USE_KDE=soprano and USE_GL=gl.
  - Add several missing X11 dependencies that were being pulled indirectly.
  - The IPOD option causes the iPod plugin to link against a few other libraries
    via libgpod-1.0.pc, so add them.
  - The MP3TUNES option needs either libgcrypt or OpenSSL; explicitly disable
    libgcrypt and add USES=ssl as required.
  - Fix a capitalization typo in MP3TUNES_CMAKE_OFF that was preventing the
    MP3Tunes code from being properly disabled in CMake.

  Based on an initial patch sent by Matthey Rezny <matthew@reztek.cz>.

  PR:		214184

  Approved by:	ports-secteam (junovitch)

Changes:
_U  branches/2016Q4/
  branches/2016Q4/audio/amarok-kde4/Makefile
Comment 11 commit-hook freebsd_committer 2016-12-07 16:44:16 UTC
A commit references this bug:

Author: rakuco
Date: Wed Dec  7 16:43:52 UTC 2016
New revision: 428077
URL: https://svnweb.freebsd.org/changeset/ports/428077

Log:
  MFH: r427906 r427909

  Switch to USES=mysql:embedded

  PR:		214184
  Submitted by:	matthew@reztek.cz

  Add several missing dependencies to the port.

  `make stage-qa' was complaining about a lot of missing dependencies:
  - Amarok actually links against MySQL, so we need USES=mysql, not
    USES=mysql:embedded.
  - Add USE_KDE=soprano and USE_GL=gl.
  - Add several missing X11 dependencies that were being pulled indirectly.
  - The IPOD option causes the iPod plugin to link against a few other libraries
    via libgpod-1.0.pc, so add them.
  - The MP3TUNES option needs either libgcrypt or OpenSSL; explicitly disable
    libgcrypt and add USES=ssl as required.
  - Fix a capitalization typo in MP3TUNES_CMAKE_OFF that was preventing the
    MP3Tunes code from being properly disabled in CMake.

  Based on an initial patch sent by Matthey Rezny <matthew@reztek.cz>.

  PR:		214184

  Approved by:	ports-secteam (junovitch)

Changes:
_U  branches/2016Q4/
  branches/2016Q4/audio/amarok-kde4/Makefile
Comment 12 Matthew Rezny freebsd_committer 2016-12-09 04:04:26 UTC
(In reply to Raphael Kubo da Costa from comment #9)

Considering that I don't use Amarok at all and kinda despise it, it's not worth opening another PR. I was merely trying to clear the way for FFmpeg 3.x, and I was fixing other obvious issue encountered on the way. Although I like the idea of separate change sets for logically independent changes, that is just not practical for a contributor when PR handling times are measured in weeks if not months (this one took just over a month). Lumping all the changes for a port into a single PR is the only (somewhat) practical way to make measurable forwards progress.

When I was testing the patch for newer FFmpeg, I built on a live system and I had turned off all the options in Amarok to minimize the dependencies. Of course, this caused me to notice any option that does not properly switch off, because the dependencies were present in the live build environment so the compilation will find and use those which are not properly disabled during the configure phase. Testing in Poudriere will NOT identify such problems unless you go out of your way to ensure the libraries are present in the build jail, i.e. temporarily make the all the depends non-optional. Without the change suggested by tcberner, Amarok is linking against libmtp if it's present regardless of the option state. That is incorrect and I have tested the proposed solution, but I'm already bending over backwards here and don't need to go further. I simply have more productive things to do than atomize fixes to software I'd rather just delete.