Bug 245741

Summary: games/scummvm: Update to 2.2.0, add A52 and CURL options
Product: Ports & Packages Reporter: Andy Mender <andymenderunix>
Component: Individual Port(s)Assignee: Kai Knoblich <kai>
Status: Closed FIXED    
Severity: Affects Only Me CC: kai, lme
Priority: --- Keywords: buildisok
Version: LatestFlags: kai: maintainer-feedback+
kai: merge-quarterly-
Hardware: Any   
OS: Any   
Description Flags
Makefile diff
koobs: maintainer-approval+
Makefile diff
scummvm-2.2.0-with-additional-options.patch kai: maintainer-approval+

Description Andy Mender 2020-04-19 13:01:53 UTC
Created attachment 213561 [details]
Makefile diff

make prints the following lines when building games/scummvm:
====> Running Q/A tests (stage-qa)
Error: /usr/local/bin/scummvm is linked to /usr/local/lib/liba52.so.0 from audio/liba52 but it is not declared as a dependency
Warning: you need LIB_DEPENDS+=liba52.so:audio/liba52
Error: /usr/local/bin/scummvm is linked to /usr/local/lib/libcurl.so.4 from ftp/curl but it is not declared as a dependency
Warning: you need LIB_DEPENDS+=libcurl.so:ftp/curl

The attached diff adds the missing records to LIB_DEPENDS.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2020-04-20 02:01:17 UTC
Comment on attachment 213561 [details]
Makefile diff

Approved by: portmgr (blanket: ports compliance)
MFH: 2020Q2 (blanket: ports compliance)

Pending QA (perhaps these dependencies aren't necessary/expected, or can be made optional, or other changes to the patch would be needed)
Comment 2 Andy Mender 2020-04-20 21:24:22 UTC
(In reply to Kubilay Kocak from comment #1)
I noticed the "configure" script for SCUMMVM contains quite a lot of toggles to switch specific features ON and OFF. I'm not sure whether it's necessary to handle most/all of them, but I'd like to give it a go.
Comment 3 Andy Mender 2020-04-21 22:13:10 UTC
Created attachment 213655 [details]
Makefile diff

I took a bit of liberty with this port:
- audio/liba52 is now an optional dependency and is by default ON (preserving existing functionality)

- ftp/libcurl is now an optional dependency and is by default OFF (does SCUMMVM require network support via cURL?)

- audio/libtremor was re-added as an optional dependency and is by default OFF (was previously disabled entirely)

ScummVM supports quite a lot of different audio, video and image formats, so it's hard to say which combination would be optimal for a good out-of-the-box experience.

As a side note, ScummVM 2.1.2 was released around a month ago so I'd like to update the port soon, but in a separate bug report.
Comment 4 Lars Engels freebsd_committer 2020-04-22 08:46:30 UTC
Andy, thanks a lot for your patch!

I'll have a look at it later.
Comment 5 Automation User 2020-05-11 03:04:21 UTC
Build info is available at https://gitlab.com/swills/freebsd-ports/pipelines/144524976
Comment 6 Kai Knoblich freebsd_committer 2020-12-18 08:51:37 UTC
I became aware of this PR when I checked for possible duplicates of the ScummVM 2.2.0 update. After an private exchange by e-mail with lme@, I take this PR and also assume the maintainership of games/scummvm.

^Triage: Also update the summary of this PR and remove the maintainers of the audio/liba52 and ftp/curl ports because it affects only games/scummvm .
Comment 7 Kai Knoblich freebsd_committer 2020-12-18 08:56:23 UTC
(In reply to Andy Mender from comment #0)

At the outset: Thank you for reporting the issue and the attached patches.

I couldn't initially reproduce the errors with a poudriere build and all options enabled.  However, if audio/liba52 and ftp/curl are already present in the build environment, the "configure" script add both ports as dependencies which then leads to the error messages you reported.

(In reply to Andy Mender from comment #3)

Adding audio/liba52 (aka AC3) as additional default dependency makes sense as it improves the user experience for the "zvision" engine (= Zork Nemesis and Zork Grandinquisitor) regarding audio/video playback.

The ftp/curl port is required if one wants to make use of the Cloud storage (via Box/DropBox/GoogleDrive/OneDrive). This feature was introduced with the 2.1.0 release and has a own tab in the options dialog if enabled. Thus it makes sense to add the option and to leave it as non-default for a while to avoid POLA. It can then still be set as default later on popular demand.

Regarding adding a conditional option for audio/libtremor support: 

Tremor/Vorbis support is mutually exclusive as audio/libtremor is an alternate Vorbis implementation and users would have to choose between one of these two. This would be associated with a little more effort in terms of implementation in the Makefile.

Also audio/libtremor is actually used more for embedded systems that don't have a FPU because the library uses fixed point instead floating point operations.  In view of these facts, I'd suggest to keep Tremor support deactivated unconditionally, IMHO.
Comment 8 Kai Knoblich freebsd_committer 2020-12-18 09:01:34 UTC
Created attachment 220691 [details]

Because quite a lot of time has now passed and the year will soon be over, I combined the initial patch with the ScummVM 2.2.0 update and attached an updated patch for further reviewing/testing.

The updated patch contains following items:

- Update games/scummvm to 2.2.0
- Remove patches for newer versions of FluidSynth that are incorporated by upstream
- Add A52 and CURL options and set A52 as default (based on the initial patch)
- Add converters/fribidi to LIB_DEPENDS as fribidi was introduced as a new dependency since the 2.2.0 release. It's already pulled in via USES=gnome, by the way.
- Rebase the patch for the configure script, adapt to the new preferred location of man pages and avoid hardcoding of ${LOCALBASE}.
- Taking maintainership

- poudriere -> OK (11.4-, 12.1-, 12.2-RELEASE, 13.0-CURRENT@r366935 amd64 i386 with default/all/no options set)
Comment 9 Kai Knoblich freebsd_committer 2020-12-18 09:04:17 UTC
^Triage: The previous maintainer responded in comment #4 and the 2.2.0 release has quite a lot of new features so it doesn't qualify for MFH'ing into the 2020Q4 branch.

Set the maintainer-feedback and MFH flags accordingly.
Comment 10 commit-hook freebsd_committer 2020-12-19 12:30:01 UTC
A commit references this bug:

Author: kai
Date: Sat Dec 19 12:29:05 UTC 2020
New revision: 558446
URL: https://svnweb.freebsd.org/changeset/ports/558446

  games/scummvm: Update to 2.2.0

  * Introduce A52 and CURL options to avoid possible QA stage errors with
    build environments that have already audio/liba52 or ftp/curl installed
    because the configure script automatically adds them as an additional
    dependencies in that case. [1]

    Set the A52 option as default as it improves the user experience with the
    "zvision" engine regarding audio/video playback.

    Leave the CURL option as non-default for now, because the ScummVM cloud
    storage is a fairly new feature that has been introduced since the 2.1.0
    release.  That option can be set as default later if there's great demand.

  * Remove now obsolete patches which were required for newer versions of

  * Rebase the patch and post-processing of the configure script, switch to
    the new preferred location of man pages (since r523104) and avoid
    hardcoded occurences of ${LOCALBASE}.

  * Many thanks to lme@ who maintained the port for over 13 years and offered
    me to take over maintainership.



  PR:		245741
  Submitted by:	Andy Mender [1]
  Approved by:	lme (maintainer, via private e-mail)

Comment 11 Kai Knoblich freebsd_committer 2020-12-19 12:45:39 UTC
(In reply to Andy Mender from comment #0)

Committed, thanks for the report and the patches!