| Summary: | Review request: moc - Music on Console - Console audio player for Linux/UNIX | ||
|---|---|---|---|
| Product: | Package Reviews | Reporter: | A. Trande (sagitter) <trpost> |
| Component: | Review Request | Assignee: | Björn "besser82" Esser <bjoern.esser> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | bjoern.esser, eldermarco, giallu, rayllu, rpmfusion-package-review, trpost |
| Priority: | P5 | ||
| Version: | Current | ||
| Hardware: | All | ||
| OS: | GNU/Linux | ||
| namespace: | |||
| Attachments: | moc.spec file | ||
|
Description
A. Trande (sagitter)
2012-10-14 12:31:48 CEST
Hello,
Just a few comments:
- You don't need to add the first line (# This file is made...). The identity of the packager is evident from the changelog entry;
- It would be nice if you add one package per line in Requires/BuildRequires;
- You can remove BuidRoot and Groups tags [1]
- Replace %{_make} and %{_rm} with make and rm [2]. Do not use macros for simple things. You never know if/how they are defined;
- You should own the directory %{_datadir}/%{name} [3];
[1] - http://fedoraproject.org/wiki/Packaging:Guidelines#Tags
[2] - http://fedoraproject.org/wiki/Packaging:Guidelines#Macros
[3] - http://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
(In reply to comment #1) > Hello, > > Just a few comments: > > - You don't need to add the first line (# This file is made...). The identity > of the packager is evident from the changelog entry; > - It would be nice if you add one package per line in Requires/BuildRequires; > - You can remove BuidRoot and Groups tags [1] > - Replace %{_make} and %{_rm} with make and rm [2]. Do not use macros for > simple things. You never know if/how they are defined; > - You should own the directory %{_datadir}/%{name} [3]; > > > [1] - http://fedoraproject.org/wiki/Packaging:Guidelines#Tags > [2] - http://fedoraproject.org/wiki/Packaging:Guidelines#Macros > [3] - > http://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership Hi eldermarco. http://sagitter.fedorapeople.org/moc.spec http://sagitter.fedorapeople.org/moc-2.5.0-0.1.beta1.fc17.src.rpm $ rpmlint moc-2.5.0-0.1.beta1.fc17.src.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. $ rpmlint moc-2.5.0-0.1.beta1.fc17.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Thank you. OK. Just a few more comments.
* Use %doc for the documentation [1];
* You still should own the directory %{_datadir}/%{name} [2]. You can do it by adding this line at the %files section:
%dir %{_datadir}/%{name}
or you can own this directory and all files and directories inside it:
%{_datadir}/%{name}
In this case, you don't need to list the files or directories inside it.
* About the Source URL, read [3] and try to change the URL if possible.
Cosmetic changes:
===========
* I don't think it is mandatory but for readability reasons it would be nice if you add 'Requires' tag after 'BuildRequires'. Also, add a blank line between them and after the 'Source0' tag:
Source0: <tarball_url>
<blank line>
BuildRequires: <package1>
BuildRequies: <package2>
....
BuildRequires: <packageN>
<blank line>
Requires: <another_package1>
Requires: <another_package2>
....
Requires: <another_packageN>
<blank line>
[1] - http://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
[2] - http://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
[3] - http://fedoraproject.org/wiki/Packaging:SourceURL
(In reply to comment #3) > OK. Just a few more comments. > > * Use %doc for the documentation [1]; > * You still should own the directory %{_datadir}/%{name} [2]. You can do it by > adding this line at the %files section: > > %dir %{_datadir}/%{name} > > or you can own this directory and all files and directories inside it: > > %{_datadir}/%{name} > If I understood fine, I must include %doc files in a '-doc' package ... Created attachment 978 [details]
moc.spec file
No, you must add documentation files using the %doc section. In other words, remove the directory $RPM_BUILD_ROOT%{_datadir}/doc/%{name}:
...
%install
%make_install DESTDIR=$RPM_BUILD_ROOT
rm -rf $RPM_BUILD_ROOT%{_datadir}/doc/%{name}
...
and add the files in the %doc section:
...
%files
%doc README README_equalizer AUTHORS ChangeLog COPYING config.example keymap.example
%_bindir/*
%_datadir/%{name}/themes/*
%_datadir/doc/%{name}/*.example << remove this line
%_datadir/doc/%{name}/README* << remove this line
%_datadir/doc/%{name}/THANKS << remove this line
%_mandir/*/*
%_libdir/moc/decoder_plugins
...
You don't need to create a subpackage.
http://sagitter.fedorapeople.org/moc.spec http://sagitter.fedorapeople.org/moc-2.5.0-0.1.beta1.fc17.src.rpm Thank you very much. Oops, sorry. The spec file is wrong but it is my fault.
line 54:
rm -rf $RPM_BUILD_ROOT%{_datadir}/doc/%{name}
The correct is
rm -rf $RPM_BUILD_ROOT%{_datadir}/doc
* In the %files section
The line
%dir %_datadir/%{name}
is missing.
(In reply to comment #8) > Oops, sorry. The spec file is wrong but it is my fault. > > > line 54: > > rm -rf $RPM_BUILD_ROOT%{_datadir}/doc/%{name} > > The correct is > > rm -rf $RPM_BUILD_ROOT%{_datadir}/doc > > > * In the %files section > > The line > > %dir %_datadir/%{name} > > is missing. Corrected. rpmlint doesn't provide errors o warnings. http://sagitter.fedorapeople.org/moc.spec http://sagitter.fedorapeople.org/moc-2.5.0-0.1.beta1.fc17.src.rpm OK, thanks :) I will do an informal review tomorrow. Unfortunately, I am just a packager not a sponsor. So, I can't sponsor you. (In reply to comment #10) > OK, thanks :) > > I will do an informal review tomorrow. Unfortunately, I am just a packager not > a sponsor. So, I can't sponsor you. Thank for your fundamental help. I must learn still many things. I hope to see accepted soon MOC in RPM Fusion. :) This is an informal review since I can't sponsor you.
------------------------
Key:
[+] - OK
[-] - FIX/Needs work.
[x] - Not applicable
-------------------------
MUST ITEMS
===========
[+] MUST: rpmlint must be run on the source rpm and all binary rpms the build
produces. The output should be posted in the review.
$ rpmlint moc-2.5.0-0.1.beta1.fc17.src.rpm moc-2.5.0-0.1.beta1.fc17.x86_64.rpm moc-debuginfo-2.5.0-0.1.beta1.fc17.x86_64.rpm
moc-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/moc-2.5.0-beta1/decoder_plugins/mp3/xing.h
moc-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/moc-2.5.0-beta1/decoder_plugins/mp3/xing.c
3 packages and 0 specfiles checked; 2 errors, 0 warnings.
You can ignore these errors. You don't need to fix them to get this package
approved. See ADITIONAL ITEMS below.
[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}, in the format
%{name}.spec unless your package has an exemption.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license and meet
Licensing Guidelines.
[-] MUST: The License field in the package spec file must match the actual license.
-- License field in spec file doesn't match the actual license. If you look
into the headers of the source code you will see "or (at you option) any" later
version. This means GPLv2+ instead of GPLv2 [1]. The files md5.c and md5.h is
licensed under GPLv3+. See [2] for more information.
[+] MUST: 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 must be included in %doc.
[+] MUST: The spec file must be written in American English.
[-] MUST: The spec file for the package MUST be legible.
-- Add a blank line after 'autoreconf -i'
[-] MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL. Reviewers should use sha256sum for this task as it
is used by the sources file once imported into git. If no upstream URL can be
specified for this package, please see the Source URL Guidelines for how to deal
with this.
-- sha256sum: 076816da9c6d1e61a386a1dda5f63ee2fc84bc31e9011ef70acc1d391d4c46a6
It is correct, but you can use this URL as Source URL:
Source0: ftp://ftp.daper.net/pub/soft/moc/unstable/%{name}-%{version}-beta1.tar.z2
[+] MUST: The package MUST successfully compile and build into binary rpms on at
least one primary architecture.
[x] MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
bugzilla, describing the reason that the package does not compile/build/work on
that architecture. The bug number MUST be placed in a comment, next to the
corresponding ExcludeArch line.
[-] MUST: All build dependencies must be listed in BuildRequires, except for any
that are listed in the exceptions section of the Packaging Guidelines; inclusion
of those as BuildRequires is optional. Apply common sense.
-- You need to add libtool as BuildRequires. I can't build this package in
mock without libtool.
[x] MUST: The spec file MUST handle locales properly. This is done by using the
%find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
[+] MUST: Every binary RPM package (or subpackage) which stores shared library
files (not just symlinks) in any of the dynamic linker's default paths, must call
ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[x] MUST: If the package is designed to be relocatable, the packager must state
this fact in the request for review, along with the rationalization for
relocation of that specific package. Without this, use of Prefix: /usr is
considered a blocker.
[+] MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory.
[+] MUST: A Fedora package must not list a file more than once in the spec file's
%files listings. (Notable exception: license texts in specific situations)
[+] MUST: Permissions on files must be set properly. Executables should be set
with executable permissions, for example.
[-] MUST: Each package must consistently use macros.
-- Use %{_datadir}, %{_bindir}, %{_mandir} and %{_libdir} rather than
%_datadir, %_bindir, %_mandir and %_libdir
[+] MUST: The package must contain code, or permissable content.
[x] MUST: Large documentation files must go in a -doc subpackage. (The definition
of large is left up to the packager's best judgement, but is not restricted to
size. Large can refer to either size or quantity).
[+] MUST: If a package includes something as %doc, it must not affect the runtime
of the application. To summarize: If it is in %doc, the program must run
properly if it is not present.
[x] MUST: Static libraries must be in a -static package.
[x] MUST: Development files must be in a -devel package.
[x] MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency:
Requires: %{name}%{?_isa} = %{version}-%{release}
[-] MUST: Packages must NOT contain any .la libtool archives, these must be
removed in the spec if they are built.
-- Libtool archives (*.la files) should not be included. The may need to
removed before packaging. See [3] (second paragraph) for more information.
You can find an examples here:
http://pkgs.fedoraproject.org/cgit/libogg.git/tree/libogg.spec
[x] MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed with desktop-file-install in the
%install section. If you feel that your packaged GUI application does not need
a .desktop file, you must put a comment in the spec file with your explanation.
[+] MUST: Packages must not own files or directories already owned by other
packages. The rule of thumb here is that the first package to be installed should
own the files or directories that other packages may rely upon. This means, for
example, that no package in Fedora should ever share ownership with any of the
files or directories owned by the filesystem or man package. If you feel that you
have a good reason to own a file or directory that another package owns, then
please present that at package review time.
[+] MUST: All filenames in rpm packages must be valid UTF-8.
SHOULD ITEMS
=============
[x] SHOULD: 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] SHOULD: The description and summary sections in the package spec file should
contain translations for supported Non-English languages, if available.
[-] SHOULD: The reviewer should test that the package builds in mock.
-- You must add libtool as BuildRequires. See MUST ITEMS above.
[+] SHOULD: The package should compile and build into binary rpms on all
supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described. A
package should not segfault instead of running, for example.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane. This is
vague, and left up to the reviewers judgement to determine sanity.
[x] SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.
[x] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and
this is usually for development purposes, so should be placed in a -devel pkg.
A reasonable exception is that the main pkg itself is a devel tool not installed
in a user runtime, e.g. gcc or gdb.
[x] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin consider requiring the package which provides the file
instead of the file itself.
[x] SHOULD: your package should contain man pages for binaries/scripts. If it
doesn't, work with upstream to add them where they make sense.
ADITIONAL ITEMS
===============
* Add NEWS in %doc;
* line 45:
%setup -q -n moc-2.5.0-beta1
We can use macros here. Use %{name} instead of moc and %{version}
instead of 2.5.0:
%setup -q -n %{name}-%{version}-beta1
* I think it would be better if we add build dependencies as pkgconfig modules,
if possible. For example, the package ncurses-devel provides pkgconfig(ncurses)
(run rpm -q --provides ncurses-devel | grep pkgconfig). So, we can use
BuildRequires: pkgconfig(ncurses)
instead of
BuildRequires: ncurses-devel
But it is just an idea. You can find an example here:
http://pkgs.fedoraproject.org/cgit/gpaste.git/tree/gpaste.spec
* I think it is better if we add some build options explicitly:
%configure --disable-static --without-rcc \
<indentation here>--with-oss --with-alsa --with-jack --with-aac --with-mp3 \
<indentation here>--with-musepack --with-vorbis --with-flac --with-wavpack \
<indentation here>--with-sndfile --with-modplug --with-ffmpeg --with-speex \
<indentation here>--with-samplerate --with-curl
* Tell upstream that FSF for files decoder_plugins/mp3/xing.c
and decoder_plugins/mp3/xing.h (see the first must item above) is outdated.
Or can provide a patch, if you want.
* Don't forget to update your changelog.
That's all :)
-------------------------------------
[1] - https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#.22or_later_version.22_licenses
[2] - https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios
[3] - http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
(In reply to comment #12) > This is an informal review since I can't sponsor you. > > ------------------------ > Key: > > [+] - OK > [-] - FIX/Needs work. > [x] - Not applicable > ------------------------- > > MUST ITEMS > =========== > > It is correct, but you can use this URL as Source URL: > Source0: > ftp://ftp.daper.net/pub/soft/moc/unstable/%{name}-%{version}-beta1.tar.z2 Done but rpmlint provides me an error: $ rpmlint moc-2.5.0-0.1.beta1.fc17.src.rpm moc.src: W: invalid-url Source0: ftp://ftp.daper.net/pub/soft/moc/unstable/moc-2.5.0-beta1.tar.bz2 <urlopen error ftp error: (null) Down: 0 Files (0mb) Up: 0 Files (0mb) 10,000,000:1 CR: LEECH> > > [-] MUST: All build dependencies must be listed in BuildRequires, except for > any > that are listed in the exceptions section of the Packaging Guidelines; > inclusion > of those as BuildRequires is optional. Apply common sense. > -- You need to add libtool as BuildRequires. I can't build this package > in > mock without libtool. Done. I have changed 'BuildRequires:ffmpeg-devel' with ##Build Requires of ffmpeg-devel BuildRequires: pkgconfig(libavcodec) BuildRequires: pkgconfig(libavdevice) BuildRequires: pkgconfig(libavfilter) BuildRequires: pkgconfig(libavformat) BuildRequires: pkgconfig(libavutil) BuildRequires: pkgconfig(libpostproc) BuildRequires: pkgconfig(libswresample) BuildRequires: pkgconfig(libswscale) Is it right ? > > [-] MUST: Packages must NOT contain any .la libtool archives, these must be > removed in the spec if they are built. > -- Libtool archives (*.la files) should not be included. The may need to > removed before packaging. See [3] (second paragraph) for more information. > You can find an examples here: > http://pkgs.fedoraproject.org/cgit/libogg.git/tree/libogg.spec > > SHOULD ITEMS > ============= > > [-] SHOULD: The reviewer should test that the package builds in mock. > -- You must add libtool as BuildRequires. See MUST ITEMS above. Done. > > ADITIONAL ITEMS > =============== > * Add NEWS in %doc; > > * line 45: > %setup -q -n moc-2.5.0-beta1 > > We can use macros here. Use %{name} instead of moc and %{version} > instead of 2.5.0: > %setup -q -n %{name}-%{version}-beta1 > > * I think it would be better if we add build dependencies as pkgconfig modules, > if possible. For example, the package ncurses-devel provides pkgconfig(ncurses) > (run rpm -q --provides ncurses-devel | grep pkgconfig). So, we can use > > BuildRequires: pkgconfig(ncurses) > > instead of > > BuildRequires: ncurses-devel > > But it is just an idea. You can find an example here: > http://pkgs.fedoraproject.org/cgit/gpaste.git/tree/gpaste.spec > > > * I think it is better if we add some build options explicitly: > > %configure --disable-static --without-rcc \ > <indentation here>--with-oss --with-alsa --with-jack --with-aac --with-mp3 \ > <indentation here>--with-musepack --with-vorbis --with-flac --with-wavpack \ > <indentation here>--with-sndfile --with-modplug --with-ffmpeg --with-speex \ > <indentation here>--with-samplerate --with-curl Done. > > * Tell upstream that FSF for files decoder_plugins/mp3/xing.c > and decoder_plugins/mp3/xing.h (see the first must item above) is outdated. > Or can provide a patch, if you want. I will send a mail about this issue to the upstream maintainer. Is it disabling for the request ? > > * Don't forget to update your changelog. > > That's all :) Thank you. I forgot ... .spec file and .src package are available always here: http://sagitter.fedorapeople.org/moc.spec http://sagitter.fedorapeople.org/moc-2.5.0-0.1.beta1.fc17.src.rpm this is the response from trying to build the package, it is generating errors in other packages
$ rpmbuild -bb moc.spec
error: Failed build dependencies:
pkgconfig(jack) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(samplerate) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(taglib) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(speex) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(mad) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(id3tag) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(flac) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(sndfile) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(libmodplug) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(libtimidity) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(wavpack) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
libdb-devel is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
libtool-ltdl-devel is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
pkgconfig(opus) is needed by moc-2.5.0-0.1.beta1.fc17.x86_64
# yum install libtool-ltdl-devel libdb-devel
Loaded plugins: downloadonly, langpacks, priorities
rpmfusion-free-updates | 3.3 kB 00:00
rpmfusion-nonfree-updates | 3.3 kB 00:00
updates/metalink | 2.3 kB 00:00
Resolving Dependencies
--> Running transaction check
---> Package libdb-devel.x86_64 0:5.2.36-5.fc17 will be installed
---> Package libtool-ltdl-devel.x86_64 0:2.4.2-3.1.fc17 will be installed
--> Finished Dependency Resolution
Dependencies Resolved
=====================================================================================================
Package Arch Version Repository Size
=====================================================================================================
Installing:
libdb-devel x86_64 5.2.36-5.fc17 fedora 44 k
libtool-ltdl-devel x86_64 2.4.2-3.1.fc17 updates 165 k
Transaction Summary
=====================================================================================================
Install 2 Packages
Total download size: 210 k
Installed size: 926 k
Is this ok [y/N]: t
Is this ok [y/N]: y
Downloading Packages:
(1/2): libdb-devel-5.2.36-5.fc17.x86_64.rpm | 44 kB 00:00
(2/2): libtool-ltdl-devel-2.4.2-3.1.fc17.x86_64.rpm | 165 kB 00:00
-----------------------------------------------------------------------------------------------------
Total 295 kB/s | 210 kB 00:00
Running Transaction Check
Running Transaction Test
Transaction Check Error:
file /usr/include/db.h from install of libdb-devel-5.2.36-5.fc17.x86_64 conflicts with file from package db4-devel-4.8.30-10.fc17.x86_64
file /usr/include/db_185.h from install of libdb-devel-5.2.36-5.fc17.x86_64 conflicts with file from package db4-devel-4.8.30-10.fc17.x86_64
file /usr/include/db_cxx.h from install of libdb-devel-5.2.36-5.fc17.x86_64 conflicts with file from package db4-devel-4.8.30-10.fc17.x86_64
file /usr/lib64/libdb.so from install of libdb-devel-5.2.36-5.fc17.x86_64 conflicts with file from package db4-devel-4.8.30-10.fc17.x86_64
Error Summary
-------------
Hello Stephen, You must install the dependencies. Open a terminal and run $ yum-builddep /path/to/moc.spec Remove the package db4-devel from your system before. These are the same package, but with different versions. > > It is correct, but you can use this URL as Source URL: > > Source0: > > ftp://ftp.daper.net/pub/soft/moc/unstable/%{name}-%{version}-beta1.tar.z2 > > Done but rpmlint provides me an error: > $ rpmlint moc-2.5.0-0.1.beta1.fc17.src.rpm > moc.src: W: invalid-url Source0: > ftp://ftp.daper.net/pub/soft/moc/unstable/moc-2.5.0-beta1.tar.bz2 <urlopen > error ftp error: (null) Down: 0 Files (0mb) Up: 0 Files (0mb) 10,000,000:1 > CR: LEECH> You can ignore this error, since spectool works fine: $ spectool -g moc.spec Getting ftp://ftp.daper.net/pub/soft/moc/unstable/moc-2.5.0-beta1.tar.bz2 to ./moc-2.5.0-beta1.tar.bz2 % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 557k 100 557k 0 0 80979 0 0:00:07 0:00:07 --:--:-- 123k > I have changed 'BuildRequires:ffmpeg-devel' with > > ##Build Requires of ffmpeg-devel > BuildRequires: pkgconfig(libavcodec) > BuildRequires: pkgconfig(libavdevice) > BuildRequires: pkgconfig(libavfilter) > BuildRequires: pkgconfig(libavformat) > BuildRequires: pkgconfig(libavutil) > BuildRequires: pkgconfig(libpostproc) > BuildRequires: pkgconfig(libswresample) > BuildRequires: pkgconfig(libswscale) > > Is it right ? No, libavformat is enough: BuildRequires: pkgconfig(libavformat) > > > > * Tell upstream that FSF for files decoder_plugins/mp3/xing.c > > and decoder_plugins/mp3/xing.h (see the first must item above) is outdated. > > Or can provide a patch, if you want. > > I will send a mail about this issue to the upstream maintainer. > Is it disabling for the request ? Yes, send a message. Fix FSF address is not really necessary for the review. (In reply to comment #17) > > > I have changed 'BuildRequires:ffmpeg-devel' with > > > > ##Build Requires of ffmpeg-devel > > BuildRequires: pkgconfig(libavcodec) > > BuildRequires: pkgconfig(libavdevice) > > BuildRequires: pkgconfig(libavfilter) > > BuildRequires: pkgconfig(libavformat) > > BuildRequires: pkgconfig(libavutil) > > BuildRequires: pkgconfig(libpostproc) > > BuildRequires: pkgconfig(libswresample) > > BuildRequires: pkgconfig(libswscale) > > > > Is it right ? > No, libavformat is enough: > > BuildRequires: pkgconfig(libavformat) > Done. http://sagitter.fedorapeople.org/moc.spec http://sagitter.fedorapeople.org/moc-2.5.0-0.1.beta1.fc17.src.rpm The db-devel inconsistency is listed as https://bugzilla.redhat.com/show_bug.cgi?id=758491 (In reply to comment #17) > > > * Tell upstream that FSF for files decoder_plugins/mp3/xing.c > > > and decoder_plugins/mp3/xing.h (see the first must item above) is outdated. > > > Or can provide a patch, if you want. > > > > I will send a mail about this issue to the upstream maintainer. > > Is it disabling for the request ? > Yes, send a message. Fix FSF address is not really necessary for the review. Added a patch to fix FSF address. $ rpmlint moc-debuginfo-2.5.0-0.1.beta1.fc17.x86_64.rpm moc-2.5.0-0.1.beta1.fc17.x86_64.rpm 2 packages and 0 specfiles checked; 0 errors, 0 warnings. http://sagitter.fedorapeople.org/moc.spec http://sagitter.fedorapeople.org/moc-2.5.0-0.1.beta1.fc17.src.rpm (In reply to comment #19) > The db-devel inconsistency is listed as > https://bugzilla.redhat.com/show_bug.cgi?id=758491 Thanks! :) (In reply to comment #20) > Added a patch to fix FSF address. > It was not necessary, but there is no problem either. Please send this patch to upstream. * fix License field (see MUST ITEMS above) * Add a new entry in your spec file whenever you change something. * line 71: use %{_datadir} instead of %_datadir, just for consistency. In my opinion, your spec file is good. You need to wait for a sponsor now. (In reply to comment #21) > (In reply to comment #19) > > The db-devel inconsistency is listed as > > https://bugzilla.redhat.com/show_bug.cgi?id=758491 > > Thanks! :) > > (In reply to comment #20) > > Added a patch to fix FSF address. > > > It was not necessary, but there is no problem either. Please send this patch to > upstream. The patch comes directly from upstream maintainer. > > * fix License field (see MUST ITEMS above) > * Add a new entry in your spec file whenever you change something. > * line 71: use %{_datadir} instead of %_datadir, just for consistency. > Done. http://sagitter.fedorapeople.org/moc.spec http://sagitter.fedorapeople.org/moc-2.5.0-0.1.beta1.fc17.src.rpm > In my opinion, your spec file is good. You need to wait for a sponsor now. Thank you again. Awaiting for your kind reply. Ops, sorry. lines 4 and 78: * You have to increment the release number [1]. In this case, from '0.1.beta1' to '0.2.beta1'. Also, add a blank line between changelog entries. [1] - http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag (In reply to comment #23) > Ops, sorry. > > lines 4 and 78: > * You have to increment the release number [1]. In this case, from '0.1.beta1' > to '0.2.beta1'. Also, add a blank line between changelog entries. > > [1] - http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag Release number updated. http://sagitter.fedorapeople.org/moc.spec http://sagitter.fedorapeople.org/moc-2.5.0-0.2.beta1.fc17.src.rpm http://sagitter.fedorapeople.org/moc.spec http://sagitter.fedorapeople.org/moc-2.5.0-0.3.beta1.fc18.src.rpm Added LibRCC library support (it fixes encoding in broken mp3 tags). - Removed DESTDIR from %%make_install
- Changed source link with a public one
- Set up a new macro to define MOC's 'mocp' executable
- Added %%{name} prefix to the patch
SPEC: http://sagitter.fedorapeople.org/moc/moc.spec
SRPMS: http://sagitter.fedorapeople.org/moc/moc-2.5.0-0.4.beta1.fc18.src.rpm
Now, I'm in Fedora Packager Group. I think that everything is ok to complete this review. :) You don't need the librcc explicit require. It is added automatically since you BR librcc-devel (In reply to comment #28) > You don't need the librcc explicit require. It is added automatically since you > BR librcc-devel Hi Gianluca. Thank for your note. http://sagitter.fedorapeople.org/moc/moc.spec http://sagitter.fedorapeople.org/moc/moc-2.5.0-0.5.beta1.fc18.src.rpm - Removed autoreconf task from %%build section http://sagitter.fedorapeople.org/moc/moc.spec http://sagitter.fedorapeople.org/moc/moc-2.5.0-0.6.beta1.fc18.src.rpm Can someone volunteer to be assigned for the review ? (In reply to comment #31) > Can someone volunteer to be assigned for the review ? Hi Nicolas. Thanks for your consideration. Just another change: - Dist tag changed to %%{?dist} http://sagitter.fedorapeople.org/moc/moc.spec http://sagitter.fedorapeople.org/moc/moc-2.5.0-0.7.beta1.fc18.src.rpm INFO: No upstream for (Source0): moc-2.5.0-beta1.tar.bz2 Why don't you use Source0: http://ftp.daper.net/pub/soft/moc/unstable/%{name}-%{version}-beta1.tar.bz2 ? Will assign for review as soon as my request for joining "cvsextras" was approved, since I'm a fedora-packager already... 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.
[!]: Development (unversioned) .so files in -devel subpackage, if present.
Note: Unversioned so-files in private %_libdir subdirectory (see
attachment). Verify they are not in ld path.
--> they get picked up by ldconfig!
Get in touch with upstream to fix this issue, please.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
I'll take another shot on this after fixing.
(In reply to comment #33) > INFO: No upstream for (Source0): moc-2.5.0-beta1.tar.bz2 > > Why don't you use Source0: > http://ftp.daper.net/pub/soft/moc/unstable/%{name}-%{version}-beta1.tar.bz2 ? Ops ! This is an inheritance from an old SVN release. (In reply to comment #34) > [!]: Development (unversioned) .so files in -devel subpackage, if present. > Note: Unversioned so-files in private %_libdir subdirectory (see > attachment). Verify they are not in ld path. I'm working with upstream maintainer to fix this issue. (In reply to comment #34) > [!]: Development (unversioned) .so files in -devel subpackage, if present. > Note: Unversioned so-files in private %_libdir subdirectory (see > attachment). Verify they are not in ld path. This seems a false issue. The http://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages says: >As an additional complication, some software generates unversioned shared objects >which are not intended to be used as system libraries. These files are usually >plugins or modular functionality specific to an application, and are not located >in the ld library paths or cache. This means that they are not located directly >in /usr/lib or /usr/lib64, or in a directory listed as a library path in >/etc/ld.so.conf (or an /etc/ld.so.conf.d/config file). Usually, these unversioned >shared objects can be found in a dedicated subdirectory under /usr/lib or >/usr/lib64 (e.g. /usr/lib/purple-2/ is the plugin directory used for libpurple >applications). In these cases, the unversioned shared objects do not need to be >placed in a -devel package. In fact: $ rpm -q --list moc | grep decoder_plugins /usr/lib64/moc/decoder_plugins /usr/lib64/moc/decoder_plugins/libffmpeg_decoder.so /usr/lib64/moc/decoder_plugins/libflac_decoder.so /usr/lib64/moc/decoder_plugins/libmodplug_decoder.so /usr/lib64/moc/decoder_plugins/libmp3_decoder.so /usr/lib64/moc/decoder_plugins/libmusepack_decoder.so /usr/lib64/moc/decoder_plugins/libsndfile_decoder.so /usr/lib64/moc/decoder_plugins/libspeex_decoder.so /usr/lib64/moc/decoder_plugins/libtimidity_decoder.so /usr/lib64/moc/decoder_plugins/libvorbis_decoder.so /usr/lib64/moc/decoder_plugins/libwavpack_decoder.so $ ldconfig -p | grep _decoder.so libdirac_decoder.so.0 (libc6,x86-64) => /lib64/libdirac_decoder.so.0 http://sagitter.fedorapeople.org/moc/moc.spec http://sagitter.fedorapeople.org/moc/moc-2.5.0-0.8.beta1.fc18.src.rpm (In reply to comment #36) > This seems a false issue. > http://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages > > In fact: > > $ ldconfig -p | grep _decoder.so > libdirac_decoder.so.0 (libc6,x86-64) => /lib64/libdirac_decoder.so.0 Sorry, my bad... was a bit tired yesterday... hope you don't mind... ##### Everything is fine so far. Just one thing: There is not need for explicit Requires: curl, jack-audio-connection-kit, ncurses, speex They are picked up by rpm's automagick Requires. I leave it up to you fixing this in SCM. There are some proposals/remarks as commented in review, too. ##### 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]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files in private %_libdir subdirectory (see attachment). Verify they are not in ld path. ---> all fine as discussed above. [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]: %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 requires other packages for directories it uses. [x]: Package uses nothing in %doc for runtime. [x]: Package is not known to require ExcludeArch. [x]: Package complies to the Packaging Guidelines [x]: License field in the package spec file matches the actual license. [x]: Package consistently uses macro is (instead of hard-coded directory names). ---> Is that `%global exec` really needed? Up to your decision... [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [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]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [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. [-]: Large documentation must go in a -doc subpackage. Note: Documentation size is 112640 bytes in 8 files. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Each %files section contains %defattr if rpm < 4.4 [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Fully versioned dependency in subpackages, if present. [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 %doc. [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package do not use a name that already exist [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 [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: No rpmlint messages. ===== 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. [!]: Final provides and requires are sane (see attachments). [x]: Package functions as described. [x]: Latest version is packaged. ---> pre-release 2.5.0 beta1 [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. ---> you should ask upstream to update/include license-headers/info from latest GPLv2+ revision. [x]: Scriptlets must be sane, if used. [-]: 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. ---> there could be %check, but it's quiet exhaustive. Full tests take est. >= 15 min on i7-2860QM [x]: Packages should try to preserve timestamps of original installed files. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [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]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Uses parallel make. [x]: SourceX tarball generation or download is documented. [x]: SourceX is a working URL. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: moc-2.5.0-0.8.beta1.fc20.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint moc 1 packages and 0 specfiles checked; 0 errors, 0 warnings. # echo 'rpmlint-done:' Requires -------- moc (rpmlib, GLIBC filtered): /sbin/ldconfig curl ffmpeg jack-audio-connection-kit libFLAC.so.8()(64bit) libasound.so.2()(64bit) libasound.so.2(ALSA_0.9)(64bit) libasound.so.2(ALSA_0.9.0rc4)(64bit) libavcodec.so.54()(64bit) libavcodec.so.54(LIBAVCODEC_54)(64bit) libavformat.so.54()(64bit) libavformat.so.54(LIBAVFORMAT_54)(64bit) libavutil.so.52()(64bit) libavutil.so.52(LIBAVUTIL_52)(64bit) libc.so.6()(64bit) libcurl.so.4()(64bit) libdb-5.3.so()(64bit) libid3tag.so.0()(64bit) libjack.so.0()(64bit) libltdl.so.7()(64bit) libm.so.6()(64bit) libmad.so.0()(64bit) libmodplug.so.1()(64bit) libncursesw.so.5()(64bit) libogg.so.0()(64bit) libpthread.so.0()(64bit) librcc.so.0()(64bit) libsamplerate.so.0()(64bit) libsamplerate.so.0(libsamplerate.so.0.0)(64bit) libsndfile.so.1()(64bit) libsndfile.so.1(libsndfile.so.1.0)(64bit) libspeex.so.1()(64bit) libtimidity-0.1.so.0()(64bit) libtinfo.so.5()(64bit) libvorbis.so.0()(64bit) libvorbisfile.so.3()(64bit) libwavpack.so.1()(64bit) libz.so.1()(64bit) ncurses opus rtld(GNU_HASH) speex Provides -------- moc: libffmpeg_decoder.so()(64bit) libflac_decoder.so()(64bit) libmodplug_decoder.so()(64bit) libmp3_decoder.so()(64bit) libsndfile_decoder.so()(64bit) libspeex_decoder.so()(64bit) libtimidity_decoder.so()(64bit) libvorbis_decoder.so()(64bit) libwavpack_decoder.so()(64bit) moc moc(x86-64) Unversioned so-files -------------------- moc: /usr/lib64/moc/decoder_plugins/libffmpeg_decoder.so moc: /usr/lib64/moc/decoder_plugins/libflac_decoder.so moc: /usr/lib64/moc/decoder_plugins/libmodplug_decoder.so moc: /usr/lib64/moc/decoder_plugins/libmp3_decoder.so moc: /usr/lib64/moc/decoder_plugins/libsndfile_decoder.so moc: /usr/lib64/moc/decoder_plugins/libspeex_decoder.so moc: /usr/lib64/moc/decoder_plugins/libtimidity_decoder.so moc: /usr/lib64/moc/decoder_plugins/libvorbis_decoder.so moc: /usr/lib64/moc/decoder_plugins/libwavpack_decoder.so ---> as discussed above. Source checksums ---------------- http://ftp.daper.net/pub/soft/moc/unstable/moc-2.5.0-beta1.tar.bz2 : CHECKSUM(SHA256) this package : 076816da9c6d1e61a386a1dda5f63ee2fc84bc31e9011ef70acc1d391d4c46a6 CHECKSUM(SHA256) upstream package : 076816da9c6d1e61a386a1dda5f63ee2fc84bc31e9011ef70acc1d391d4c46a6 Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29 Buildroot used: fedora-rawhide-x86_64 Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64-rpmfusion_free -u https://bugzilla.rpmfusion.org/show_bug.cgi?id=2531 ##### This one is APPROVED! *** Bug 789 has been marked as a duplicate of this bug. *** > Sorry, my bad... was a bit tired yesterday... hope you don't mind... No problem :) > there could be %check, but it's quiet exhaustive. > Full tests take est. >= 15 min on i7-2860QM Not only. Sox don't able to manage .mp3 files so the tests fail in part. I prefer exclude it for now. http://sagitter.fedorapeople.org/moc/moc.spec http://sagitter.fedorapeople.org/moc/moc-2.5.0-0.9.beta1.fc18.src.rpm Thank you very much, Björn. Package CVS request ====================== Package Name: moc Short Description: Music on Console - Console audio player for Linux/UNIX Owners: sagitter Branches: f18 f19 ---------------------- License tag: [free] The package has been successfully built. |