Bug 1284989 - (mumble) Review Request: mumble - Voice chat suite aimed at gamers
Review Request: mumble - Voice chat suite aimed at gamers
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Extras Quality Assurance
:
: 1181366 (view as bug list)
Depends On: 1181365
Blocks: qt-reviews
  Show dependency treegraph
 
Reported: 2015-11-24 10:51 EST by John
Modified: 2015-12-06 20:07 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-12-03 15:20:36 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rdieter: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description John 2015-11-24 10:51:08 EST
Spec URL: http://fedpop.github.io/fedora/mumble/mumble.spec
SRPM URL: http://fedpop.github.io/fedora/mumble/mumble-1.2.10-1.fc23.src.rpm
Description: 
Hi I am attempting to become a maintainer of mumble and unretire the package. I am not a current maintainer and am looking for a sponsor.

Mumble is a voip client server package.  It has been out of the repos for about a year since it became abandoned.  In this build I disabled ice rpc support in order to make things easier on myself and speed up this long overdue process.  The majority of Fedora users will not need ice. If this is successful I plan to add unretire ice next.

I've had many users on my copr of this rpm for about 2 months now with positive feedback. I would like to get it into the official repo.

The dead review request is here: https://bugzilla.redhat.com/show_bug.cgi?id=1181366

Thanks

Fedora Account System Username: fedpop
Comment 1 Rex Dieter 2015-11-24 11:24:31 EST
Hi, I can help out here, I'm glad you submitted this, I too have seen and heard many users asking for mumble.
Comment 2 Raphael Groner 2015-11-24 11:40:03 EST
*** Bug 1181366 has been marked as a duplicate of this bug. ***
Comment 3 Rex Dieter 2015-11-24 11:51:57 EST
Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=11969289



Some initial comments:


Naming: ok

License: ok

Scriptlets: ok

Sources: mostly ok (see item 3 below)
228fff5975a61872b83052be97e2eabf  1.2.10.tar.gz

1.  SHOULD use %{qmake_qt4} macro in favor of %{_qt4_qmake}, the former sets flags automatically for you, so you can avoid manually setting stuff like:
QMAKE_CFLAGS_RELEASE="%{optflags}"
QMAKE_CXXFLAGS_RELEASE="%{optflags}"

2. MUST document why you're not using %{?_smp_mflags}

3. SHOULD follow https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Git_Tags
for better-named source tarballs, in particular, please consider using:
Source0: https://github.com/mumble-voip/mumble/archive/%{version}.tar.gz#/%{name}-%{version}.tar.gz

4.  SHOULD use unambiguous build deps, in particular use:
BuildRequires: qt4-devel
or
BuildRequires: pkgconfig(QtGui)
in favor of
BuildRequires: qt-devel

5.  SHOULD drop some deprecated rpm tags, including:
Group:

6. SHOULD consider dropping -protocol subpkg, it's there only for one file, and arguably for something a bit deprecated (no recent fedora release ships a kde4 desktop, nor is a single .protocol file by itself particularly useful)

7. MUST document the need for these explicit runtime dependencies:
Requires:    celt071, speex, opus
(at least some of these should already be handled via implicit rpm library autodepends)
Comment 4 Rex Dieter 2015-11-24 11:53:24 EST
Why did you add a depends on bug #1181365 ?  the package as-is currently does not need it.

I mean it *can* use it, but it's not currently a dependency, so I would argue it is not strictly a review-blocker.
Comment 5 John 2015-11-24 13:32:27 EST
Ok thanks Rex Dieter,

I addressed all of your suggestions I hope.  

1. done
2. added. old versions did not support parallel make.
3. done 
4. done
5. done
6. deleted
7. Made a note about celt071, deleted others.
Comment 6 Rex Dieter 2015-11-24 15:34:17 EST
8. Remember to always bump Release: tag, and add appropriate %changelog entries, even for review modifications


9. since you addressed removing the -protocol subpkg, may be worth adding an upgrade path for folks you may have that -subpkg installed:
Obsoletes: mumble-protocol < 1.2.10-2


10. Since mumble is being built without ice support at the moment, you can drop this from the .spec:
# Due to missing ice on ppc64
ExcludeArch: ppc64


11. SHOULD use %license tag for license files, ie replace instances of:
%doc LICENSE
with
%license LICENSE


12. SHOULD address rpmlint interesting warning:

mumble.x86_64: W: crypto-policy-non-compliance-openssl /usr/bin/mumble SSL_CTX_set_cipher_list

per looking at the non-trivial code in src/SSL.cpp and recommendations on:
https://fedoraproject.org/wiki/Packaging:CryptoPolicies

It would appear that patching src/SSL.cpp would do it, something like changing:

QString MumbleSSL::defaultOpenSSLCipherString() {
        return QLatin1String("EECDH+AESGCM:AES256-SHA:AES128-SHA");
}

to 

QString MumbleSSL::defaultOpenSSLCipherString() {
        return QLatin1String("PROFILE=SYSTEM");
}

Though I'm definitely not sure about this, may be worth consulting with your upstream on this one, to see if they think this is appropriate (or not).
Comment 7 Rex Dieter 2015-11-24 15:35:28 EST
The wiki about Crypto, also recommended sending queries to:
https://lists.fedoraproject.org/mailman/listinfo/security
Comment 8 Raphael Groner 2015-11-24 15:47:43 EST
IMHO #11 from comment #6 is MUST as fedora-review tool enforces %license.
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
Comment 9 Clinton Minton 2015-11-24 16:25:00 EST
Could the celt codec be removed as well since it was merged into Opus and is no longer being updated? If so we'd need to make a change in the default server config to make opusthreshold=0 a default. And a note that clients with version <= 1.2.3 will not be able to chat.
Comment 10 John 2015-11-24 21:25:40 EST
about the openssl change:

I did a diff of the ciphers and found all to be included on F23 for the default security level. see https://gist.github.com/fedpop/9c890cdd5a17332354fc I am feeling ok about this change. I will message the security mailing list and see what they think. 

About removing Celt:

Celt is kind of a pain point because mumble forked celt because it was unstable format and in development at the time they integrated it.  Celt 0.7.1 is the fallback for all Mumble clients so if people are still running old stuff then it will be bad. I would prefer to use the mumble celt so no problems can occur but due to bundling rules it's currently using the system celt071.  I'm not sure the statistics on the ecosystem. I would be ok with Opus only if we could figure that out.
Comment 11 John 2015-11-25 13:01:19 EST
Spec URL: http://fedpop.github.io/fedora/mumble/mumble.spec
SRPM URL: http://fedpop.github.io/fedora/mumble/mumble-1.2.10-3.fc23.src.rpm

#8 ok sorry wasn't sure when it was appropriate, I received a good answer on IRC.

#9 I'm not entirely sure how this works. I put Obsoletes:      mumble-protocol < 1.2.10-2 in the main package but I get a rpmlint warning. mumble.x86_64: W: obsolete-not-provided mumble-protocol

#10 added the license tag

#11 Discovered that the ciphers can be changed in the config. modified the murmur.ini sslCiphers= config and added it to the sources.
Comment 12 John 2015-11-25 13:08:46 EST
I think the config murmur.ini file change should probably be a patch actually.

I'll fix that up.
Comment 13 John 2015-11-25 15:45:50 EST
Spec URL: http://fedpop.github.io/fedora/mumble/mumble.spec
SRPM URL: http://fedpop.github.io/fedora/mumble/mumble-1.2.10-4.fc23.src.rpm

Hardened murmur.service and changed murmur.ini cipher setting to be applied as Patch3
Comment 14 Rex Dieter 2015-11-28 19:02:37 EST
Thanks!  Looks good to me, approved (and sponsored).  Welcome to fedora.
Comment 15 Rex Dieter 2015-11-28 19:03:37 EST
fyi, adjusted summary of review here to match Summary: tag in mumble.spec
Comment 16 Fedora Update System 2015-11-29 16:22:19 EST
mumble-1.2.10-4.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-934a0702cf
Comment 17 Fedora Update System 2015-11-29 16:23:40 EST
mumble-1.2.10-4.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-789c21d8a6
Comment 18 Fedora Update System 2015-11-30 21:22:54 EST
mumble-1.2.10-4.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update mumble'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-934a0702cf
Comment 19 Fedora Update System 2015-12-02 11:52:48 EST
mumble-1.2.10-4.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update mumble'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-789c21d8a6
Comment 20 Fedora Update System 2015-12-03 15:20:30 EST
mumble-1.2.10-4.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
Comment 21 Fedora Update System 2015-12-06 20:07:32 EST
mumble-1.2.11-1.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-764292844d

Note You need to log in before you can comment on or make changes to this bug.