Bug 809086

Summary: Review Request: mp3val - Tool for Validating and Fixing MPEG Audio Files
Product: [Fedora] Fedora Reporter: Leonid Podolny <leonid>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED DEFERRED QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: j, lemenkov, msuchy, package-review, tcallawa
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard: AwaitingSubmitter
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-07-21 13:30:18 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 201449    

Description Leonid Podolny 2012-04-02 12:59:34 UTC
Spec URL: http://dl.dropbox.com/u/6441389/mp3val.spec
SRPM URL: http://dl.dropbox.com/u/6441389/mp3val-0.1.8-1.fc16.src.rpm
Description: 
MP3val is a small, high-speed, free software tool for checking MPEG audio files' 
integrity. It can be useful for finding corrupted files (e.g. incompletely downloaded,
truncated, containing garbage). MP3val is also able to fix most of the problems. Being 
a multiplatform application, MP3val can be runned both under Windows and under Linux 
(or BSD).

The most common MPEG audio file type is MPEG 1 Layer III (mp3), but MP3val supports also 
other MPEG versions and layers. The tool is also aware of the most common types of tags 
(ID3v1, ID3v2, APEv2).

Comment 1 Mario Blättermann 2012-04-02 19:07:47 UTC
Fedora doesn't ship any mp3 related stuff:
http://fedoraproject.org/wiki/Multimedia/MP3

That's why there is no real chance to get your package into the official Fedora repositories. You can try to get it into RPMfusion, but because you are new to the packagers group and not sponsored yet, it could become somewhat difficult. For your first package, you should choose a software which *not* possibly violates patents or has any other restrictions which avoids an integration in Fedora.

Comment 2 Peter Lemenkov 2012-04-03 06:41:30 UTC
(In reply to comment #1)
> Fedora doesn't ship any mp3 related stuff:
> http://fedoraproject.org/wiki/Multimedia/MP3
> 
> That's why there is no real chance to get your package into the official Fedora
> repositories. You can try to get it into RPMfusion, but because you are new to
> the packagers group and not sponsored yet, it could become somewhat difficult.
> For your first package, you should choose a software which *not* possibly
> violates patents or has any other restrictions which avoids an integration in
> Fedora.

On the contrary it is possible to include this into Fedora. This app does NOT do actual MPEG frame decoding, so I suspect that no nasty MPEG patents are violated. I could be wrong here - I just quickly looked through the sources and I didn't see any MPEG decoding components.

Also take a look at the contents of gstreamer-plugins-bad-free - you'll see the demuxers for MPEG files as well. So demuxing and cheching the integrity of MPEG frames is OK - encoding/decoding isn't.

Comment 3 Michael Schwendt 2012-04-24 14:35:21 UTC
True. I didn't spot any of the maths for decoding the audio data either. Just the parsing of the file format to find frames.

[...]

Other than that, skimming over the file, I see several items that are unusual (and not typical for Fedora packages). Such as, but possibly not limited to:


> License:			GPL

GPLv2+   according to the source files.


> BuildRoot:		        %{_tmppath}/build-%{name}-%{version}

Please consult the related section in the packaging guidelines:
https://fedoraproject.org/wiki/Packaging:Guidelines


> BuildRequires:          	gcc-c++ libstdc++ libstdc++-devel glibc-devel

These are all not needed,
  https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
and make it less easy to rebuild packages in an environment with a replaced minimum build environment.


> %build
> cd %{_builddir}/%{name}-%{version}-src

This 'cd' command is not needed, because the source code's top-level directory is entered automatically. Verify by taking a look at the rpmbuild output.


> %clean
> %{__rm} -rf "%{buildroot}"

> %files
> %defattr(-,root,root)

> Source:			        http://prdownloads.sourceforge.net/mp3val/%{name}-%{version}-src.tar.gz

There's a section in the packaging guidelines about these items.


[...]

Visiting the Fedora Packaging related Wiki pages is highly recommended.

  https://fedoraproject.org/wiki/PackageMaintainers

The Review Guidelines are not just for reviewers, but for package submitters and existing packagers, too:

  https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

Comment 4 Leonid Podolny 2012-04-28 16:38:16 UTC
Michael,
Thank you for taking time and reviewing this spec file.
I believe I have fixed all your remarks. 
I also run rpmlint, which I didn't do on the original spec.

The updated submission is at:
http://dl.dropbox.com/u/6441389/mp3val-0.1.8-2.fc16.src.rpm
http://dl.dropbox.com/u/6441389/mp3val.spec

Comment 5 Jason Tibbitts 2012-06-29 22:38:28 UTC
URLs are 404.  To be honest, I'd want to pass this by legal just in case, but with no actual package to look at I guess there's no point.

Please clear the Whiteboard above if providing something to review.  Otherwise I'll close this ticket out.

Comment 6 Leonid Podolny 2012-07-02 16:42:43 UTC
(In reply to comment #5)
> URLs are 404.  To be honest, I'd want to pass this by legal just in case,
> but with no actual package to look at I guess there's no point.
> 
> Please clear the Whiteboard above if providing something to review. 
> Otherwise I'll close this ticket out.

After several months of silence, I gave up and deleted the files from dropbox. I will be happy if someone does, in fact, review these packages. Thanks a lot!

The files are at:
https://dl.dropbox.com/s/rlgt3poa5arhxld/mp3val-0.1.8-2.fc17.src.rpm?dl=1
https://dl.dropbox.com/s/c1bx39s8mqvh9f9/mp3val.spec?dl=1

Comment 7 Jason Tibbitts 2012-07-03 21:28:30 UTC
OK, adding this to the legal blocker.  Honestly I don't think there's any real problem here but we try to be very careful with this kind of thing.

Comment 8 Tom "spot" Callaway 2012-07-05 13:06:45 UTC
Does not encode or decode, just parses and checks. Lifting FE-Legal.

Comment 9 Jason Tibbitts 2012-07-05 18:47:13 UTC
OK, I grabbed and built the package.  rpmlint has a lot of complaints.  I'll go over some of them, but I'm not sure why you didn't see and fix them when you ran rpmlint yourself.  Also, it's bit of a stretch to call two months and one day "several months".


mp3val.x86_64: E: description-line-too-long C integrity. It can be useful for finding corrupted files (e.g. incompletely downloaded,
mp3val.x86_64: E: description-line-too-long C truncated, containing garbage). MP3val is also able to fix most of the problems. Being
mp3val.x86_64: E: description-line-too-long C a multiplatform application, MP3val can be runned both under Windows and under Linux
mp3val.x86_64: E: description-line-too-long C The most common MPEG audio file type is MPEG 1 Layer III (mp3), but MP3val supports also
mp3val.x86_64: E: description-line-too-long C other MPEG versions and layers. The tool is also aware of the most common types of tags
Please wrap your description to 72 characters.

mp3val.x86_64: W: incoherent-version-in-changelog 0.1.8-1 ['0.1.8-2.fc18', '0.1.8-2']
You should have a changelog entry each time you update the version/release.

mp3val.x86_64: W: spurious-executable-perm /usr/share/doc/mp3val-0.1.8/manual.html
mp3val.x86_64: E: wrong-script-end-of-line-encoding /usr/share/doc/mp3val-0.1.8/manual.html
Why is the manual executable?  DOS-style line endings need to be converted.

mp3val.x86_64: W: no-manual-page-for-binary mp3val
This is OK; you should ask upstream to provide a manual page, or perhaps see if Debian has written one, but it's not expected that you will provide one yourself.

mp3val-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/mp3val-0.1.8-src/main.cpp
mp3val-debuginfo.x86_64: E: wrong-script-end-of-line-encoding /usr/src/debug/mp3val-0.1.8-src/main.cpp
There are loads of these.  The DOS line endings of the source files aren't such a big deal, but why on earth are they all executable.


mp3val-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/mp3val-0.1.8-src/main.cpp
You should tell your upstream that they need to update the license to reflect the fact that the FSF moved quite some time ago.  It does look as if your upstream hasn't done much in three years, though, so this might not be something you can fix but you should still inform them.

(trimmed several repeat versions of the above three complaints)

As for the spec file, it looks quite clean.  However, you should not use %{__install}.  Instead just call "install" normally.  It's not as if we're ever going to rename the install executable or remove it from the path.  See http://fedoraproject.org/wiki/Packaging:Guidelines#Macros

Comment 10 Miroslav SuchĂ˝ 2015-07-21 13:30:18 UTC
Closing due long inactivity. Feel free to reopen if you want to continue.