Bug 1664399 - Review Request: mp3gain - Lossless MP3 volume adjustment tool
Summary: Review Request: mp3gain - Lossless MP3 volume adjustment tool
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Robert-André Mauchin
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-01-08 16:47 UTC by Karel Volný
Modified: 2019-10-04 23:06 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-09-14 00:06:07 UTC
zebob.m: fedora-review+


Attachments (Terms of Use)

Description Karel Volný 2019-01-08 16:47:14 UTC
Spec URL: https://kvolny.fedorapeople.org/mp3gain.spec
SRPM URL: https://kvolny.fedorapeople.org/mp3gain-1.6.2-2.fc29.src.rpm
Description: 
MP3Gain analyzes and adjusts mp3 files so that they have the same
volume. It does not just do peak normalization, as many normalizers
do. Instead, it does some statistical analysis to determine how loud
the file actually sounds to the human ear. Also, the changes MP3Gain
makes are completely lossless. There is no quality lost in the change
because the program adjusts the mp3 file directly, without decoding
and re-encoding.

Fedora Account System Username: kvolny

Comment 1 Xavier Bachelot 2019-01-09 09:16:09 UTC
From a cursory glance, the spec looks generally clean.
A couple comments though:
- Drop Group: tag
- Add comments for Source1, Source2 and Patch2 explaining where they come from.
- Does the manpage need to be pre-compressed ? The build could handle the compression and thus the .gz suffix can be replaced by a wildcard in %files in case the compression method changes someday.

Comment 2 Andrea Musuruane 2019-01-09 09:32:44 UTC
You can avoid to manually edit the following line for each new release:
%define tarball_version 1_6_2

Just change it to:
%define tarball_version %(echo %version|sed s/\\\\\./_/g)

Comment 3 Xavier Bachelot 2019-01-09 09:42:08 UTC
Thanks to Andrea's comment, I noticed I missed another nit-pick ;-)
- Replace %define by %global

An alternative to sed would be to use tr :
%global tarball_version %(echo %version | tr _ .)

In any case, you need to either BR: coreutils (for tr) or BR: sed

Comment 4 Karel Volný 2019-01-09 11:24:09 UTC
(In reply to Xavier Bachelot from comment #1)
> - Add comments for Source1, Source2 and Patch2 explaining where they come
> from.

if only I knew :-) ... these predate git history, and I had touched the package only three years _after_ it was imported you-know-where

the manpage seems to be extracted from the Debian package

and the same for the README.method, probably - the only trace I was able to found is here:
https://github.com/rbrito/pkg-mp3gain/blob/master/debian/README.method

IMHO, we might as well drop the file, as it doesn't convey any important information about the usage; these things are described on the project's web

...?

> - Does the manpage need to be pre-compressed ? The build could handle the
> compression and thus the .gz suffix can be replaced by a wildcard in %files
> in case the compression method changes someday.

to be honest, I don't know how this works ... simply copying uncompressed manpage to workdir in %prep will trigger its compression in %build?
how does rpmbuild know about the manpage file?

- I'd prefer not to touch it if it ain't broke

(In reply to Andrea Musuruane from comment #2)
> You can avoid to manually edit the following line for each new release:
> %define tarball_version 1_6_2
...
(In reply to Xavier Bachelot from comment #3)
> In any case, you need to either BR: coreutils (for tr) or BR: sed

the package already uses sed so let's stick to it ... and rpmbuild requires it, so no need to put that into BR(*)

(*) btw, it won't help anyways:

"Because SRPMs are built without the package’s BuildRequires installed" [https://docs.fedoraproject.org/en-US/packaging-guidelines/#_source_rpm_buildtime_macros]


I've uploaded the new version with these changes (I don't think we need to update release and add changelog entries for this WIP):

--- mp3gain.spec~       2019-01-08 16:47:00.000000000 +0100
+++ mp3gain.spec        2019-01-09 12:18:01.136284939 +0100
@@ -1,14 +1,15 @@
-%define tarball_version 1_6_2
-
 Name:          mp3gain
 Version:       1.6.2
+
+%global tarball_version %(echo %version|%{__sed} 's/\\./_/g')
+
 Release:       2%{?dist}
 Summary:       Lossless MP3 volume adjustment tool
 
-Group:         Applications/Multimedia
 License:       LGPLv2+
 URL:           http://mp3gain.sourceforge.net/
 Source0:       https://sourceforge.net/projects/%{name}/files/%{name}/%{version}/%{name}-%{tarball_version}-src.zip
+# copied from Debian
 Source1:       %{name}.1.gz
 Source2:       README.method
 Patch2:                %{name}-cflags-1.5.2.patch
@@ -53,7 +54,9 @@
 %changelog
 * Tue Jan 08 2019  Karel Volný <kvolny@redhat.com> - 1.6.2-2
 - Import to Fedora (rhbz#1664399)
-- Fixed license tag
+- Fixed License tag
+- Dropped Group tag
+- Improved tarball_version definition
 
 * Wed Jan 02 2019 Sérgio Basto <sergio@serjux.com> - 1.6.2-1
 - Update to 1.6.2

Comment 5 Robert-André Mauchin 2019-02-08 15:53:07 UTC
 - %{__sed} → sed

 - Provide the manpaqe unzipped: the build will take care of compressing it. Also glob the .gz extension as the compression method might change in the future.



Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "GNU Lesser General Public License (v2.1 or later)", "Unknown
     or generated". 16 files have unknown license. Detailed output of
     licensecheck in /home/bob/packaging/review/mp3gain/review-
     mp3gain/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in mp3gain-
     debuginfo , mp3gain-debugsource
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: mp3gain-1.6.2-2.fc30.x86_64.rpm
          mp3gain-debuginfo-1.6.2-2.fc30.x86_64.rpm
          mp3gain-debugsource-1.6.2-2.fc30.x86_64.rpm
          mp3gain-1.6.2-2.fc30.src.rpm
mp3gain.x86_64: W: spelling-error Summary(en_US) Lossless -> Loss less, Loss-less, Loveless
mp3gain.x86_64: W: spelling-error %description -l en_US normalizers -> normalizes, normalize rs, normalize-rs
mp3gain.src: W: spelling-error Summary(en_US) Lossless -> Loss less, Loss-less, Loveless
mp3gain.src: W: spelling-error %description -l en_US normalizers -> normalizes, normalize rs, normalize-rs
4 packages and 0 specfiles checked; 0 errors, 4 warnings.

Comment 6 Xavier Bachelot 2019-08-07 16:00:07 UTC
Ping, it seems this review got forgotten.

Comment 7 Karel Volný 2019-08-16 09:01:13 UTC
ah, sorry, thanks for ping

new files:
https://kvolny.fedorapeople.org/mp3gain.spec
https://kvolny.fedorapeople.org/mp3gain-1.6.2-2.fc30.src.rpm

changes:
--- mp3gain.spec~       2019-01-09 12:18:01.000000000 +0100
+++ mp3gain.spec        2019-08-16 10:54:26.930040545 +0200
@@ -1,7 +1,7 @@
 Name:          mp3gain
 Version:       1.6.2
 
-%global tarball_version %(echo %version|%{__sed} 's/\\./_/g')
+%global tarball_version %(echo %version|sed 's/\\./_/g')
 
 Release:       2%{?dist}
 Summary:       Lossless MP3 volume adjustment tool
@@ -10,7 +10,7 @@
 URL:           http://mp3gain.sourceforge.net/
 Source0:       https://sourceforge.net/projects/%{name}/files/%{name}/%{version}/%{name}-%{tarball_version}-src.zip
 # copied from Debian
-Source1:       %{name}.1.gz
+Source1:       %{name}.1
 Source2:       README.method
 Patch2:                %{name}-cflags-1.5.2.patch
 
@@ -32,7 +32,7 @@
 %setup -q -c %{name}-%{tarball_version}
 %patch2 -p0 -b .cflags
 install -p -m 644 %{SOURCE2} .
-%{__sed} -i 's/\r//' lgpl.txt
+sed -i 's/\r//' lgpl.txt
 
 
 %build
@@ -41,22 +41,23 @@
 
 %install
 install -Dp -m 755 %{name} $RPM_BUILD_ROOT%{_bindir}/%{name}
-install -Dp -m 644 %{SOURCE1} $RPM_BUILD_ROOT%{_mandir}/man1/%{name}.1.gz
+install -Dp -m 644 %{SOURCE1} $RPM_BUILD_ROOT%{_mandir}/man1/%{name}.1
 
 
 %files
 %doc README.method
 %license lgpl.txt
 %{_bindir}/%{name}
-%{_mandir}/man1/%{name}.1.gz
+%{_mandir}/man1/%{name}.1*
 
 
 %changelog
-* Tue Jan 08 2019  Karel Volný <kvolny@redhat.com> - 1.6.2-2
+* Fri Aug 16 2019  Karel Volný <kvolny@redhat.com> - 1.6.2-2
 - Import to Fedora (rhbz#1664399)
 - Fixed License tag
 - Dropped Group tag
 - Improved tarball_version definition
+- Unzipped manpage
 
 * Wed Jan 02 2019 Sérgio Basto <sergio@serjux.com> - 1.6.2-1
 - Update to 1.6.2

Comment 8 Robert-André Mauchin 2019-08-16 14:05:42 UTC
LGTM, package approved.

Comment 9 Gwyn Ciesla 2019-08-27 13:21:20 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/mp3gain

Comment 10 Fedora Update System 2019-08-27 13:32:28 UTC
FEDORA-2019-239774aa43 has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2019-239774aa43

Comment 11 Karel Volný 2019-08-27 13:53:50 UTC
damn, this one day late :-(

https://bodhi.fedoraproject.org/updates/FEDORA-2019-239774aa43#comment-1015474

Comment 12 Fedora Update System 2019-08-29 21:01:19 UTC
mp3gain-1.6.2-2.fc31 has been pushed to the Fedora 31 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-239774aa43

Comment 13 Fedora Update System 2019-09-14 00:06:07 UTC
mp3gain-1.6.2-2.fc31 has been pushed to the Fedora 31 stable repository. If problems still persist, please make note of it in this bug report.

Comment 14 Fedora Update System 2019-09-14 16:30:31 UTC
mp3gain-1.6.2-2.fc31 has been pushed to the Fedora 31 stable repository. If problems still persist, please make note of it in this bug report.

Comment 15 Xavier Bachelot 2019-09-16 08:25:44 UTC
Hi Karel,

The repo that was originally shipping mp3gain when it was not allowed in Fedora currently has mp3gain-1.6.2-3.fc31, while Fedora has mp3gain-1.6.2-2.fc31.
Could you please bump the release to 4 in order to have a proper upgrade path ?
Also, the package needs to be properly retired in that other repo. I can take care of that if you wish, just let me know.

Regards,
Xavier

Comment 16 Karel Volný 2019-09-16 10:31:42 UTC
(In reply to Xavier Bachelot from comment #15)
> The repo that was originally shipping mp3gain when it was not allowed in
> Fedora currently has mp3gain-1.6.2-3.fc31, while Fedora has
> mp3gain-1.6.2-2.fc31.
> Could you please bump the release to 4 in order to have a proper upgrade
> path ?
> Also, the package needs to be properly retired in that other repo. I can
> take care of that if you wish, just let me know.

Hi,

we're still in beta ... if you retire it[*] before final release then everything will be okay even without bumping the release, wouldn't it?

[*] yes please

Comment 17 Xavier Bachelot 2019-09-16 13:27:05 UTC
mp3gain has been retired for both Rawhide and F31 in that other famous repo.
I guess it's ok to not bump the release, there are likely very few people who have this package installed for F31 or Rawhide, and people running such a setup likely know how to yum/dnf distro-sync anyway.
If I may abuse you once more, maybe you could also take care of the other branches in Fedora so it can be completely removed on the other side? The remaining branches are F30, F29, EL7 and EL6.

Comment 18 Karel Volný 2019-09-16 14:13:16 UTC
(In reply to Xavier Bachelot from comment #17)
> If I may abuse you once more, maybe you could also take care of the other
> branches in Fedora so it can be completely removed on the other side? The
> remaining branches are F30, F29, EL7 and EL6.

I thought we'll just let it rot until the old versions are phased out ... is there any special reason to move it?

- well, yes, I can do it, it just doesn't fit into my POV that the realeased distro version shouldn't change (that much)

Comment 19 Xavier Bachelot 2019-09-16 15:51:26 UTC
No particular reason to move except keeping all pieces at the same place. Imho, it would be less confusing, shall any need to update arise for older branches.
F29 and F30 both have mp3gain 1.6.2, so it would be mostly a no op.
EL6 and EL7 have the older 1.5.2 r2, so this would indeed be a change. Looking a bit closer, this version is affected by CVE-2017-12911, which is fixed in 1.6.0 and later. I think that brings incentive for the update.

Comment 20 Xavier Bachelot 2019-09-16 15:59:06 UTC
Builds fine for EL7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=37689931

Fails on EL6 because of missing mpg123-devel:
https://koji.fedoraproject.org/koji/taskinfo?taskID=37689933

Comment 21 Karel Volný 2019-09-17 06:26:30 UTC
ok, let's do it halfway - no EL6 as it lacks mpg123, and no F29 as it's going to die in two months

Comment 22 Xavier Bachelot 2019-09-17 09:24:36 UTC
Works for me. EL6 was a moot point anyway, as it's been branched but never built.
I'll retire the EL7 and F30 branches in the other repo once it's been branched and built in Fedora/EPEL.

Thanks again.

Comment 23 Fedora Update System 2019-09-18 12:13:46 UTC
FEDORA-EPEL-2019-8effe82ec2 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-8effe82ec2

Comment 24 Fedora Update System 2019-09-18 12:22:26 UTC
FEDORA-2019-f34acb828e has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-f34acb828e

Comment 25 Fedora Update System 2019-09-18 12:38:42 UTC
FEDORA-EPEL-2019-19eeb2b3ce has been submitted as an update to Fedora EPEL 8. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-19eeb2b3ce

Comment 26 Fedora Update System 2019-09-19 02:20:35 UTC
mp3gain-1.6.2-2.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-f34acb828e

Comment 27 Fedora Update System 2019-09-19 02:38:25 UTC
mp3gain-1.6.2-2.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-8effe82ec2

Comment 28 Fedora Update System 2019-09-19 03:59:15 UTC
mp3gain-1.6.2-2.el8 has been pushed to the Fedora EPEL 8 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-19eeb2b3ce

Comment 29 Fedora Update System 2019-09-27 01:25:35 UTC
mp3gain-1.6.2-2.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.

Comment 30 Fedora Update System 2019-10-04 20:28:41 UTC
mp3gain-1.6.2-2.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

Comment 31 Fedora Update System 2019-10-04 23:06:13 UTC
mp3gain-1.6.2-2.el8 has been pushed to the Fedora EPEL 8 stable repository. If problems still persist, please make note of it in this bug report.


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