Bug 708711 - Review Request: nomnom - The graphical video download tool
Summary: Review Request: nomnom - The graphical video download 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: Volker Fröhlich
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 708554
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-05-29 01:18 UTC by Nicoleau Fabien
Modified: 2011-09-30 18:33 UTC (History)
4 users (show)

Fixed In Version: nomnom-0.1.4-4.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-09-14 22:33:19 UTC
volker27: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Nicoleau Fabien 2011-05-29 01:18:30 UTC
Spec URL: http://rpms.nicoleau-fabien.net/SPECS/nomnom.spec
SRPM URL: http://rpms.nicoleau-fabien.net/SRPMS/nomnom-0.1.3-1.fc15.src.rpm
Description: 
NomNom is an application for downloading videos from Youtube and other
similar video websites that require Adobe Flash to view the video content.
Video streaming or downloading can be started simply by dropping an URL
onto the application window.
NomNom requires "umph" that is not in fedora repos. I opened a review for it : 
https://bugzilla.redhat.com/show_bug.cgi?id=708554

the package builds on koji :
http://koji.fedoraproject.org/koji/taskinfo?taskID=3098618

rpmlint output :
[builder@FEDORABOX rpmbuild]$ rpmlint /home/builder/rpmbuild/SRPMS/nomnom-0.1.3-1.fc15.src.rpm /home/builder/rpmbuild/RPMS/x86_64/nomnom-0.1.3-1.fc15.x86_64.rpm /home/builder/rpmbuild/RPMS/x86_64/nomnom-debuginfo-0.1.3-1.fc15.x86_64.rpm
nomnom.x86_64: W: conffile-without-noreplace-flag /etc/NomNom.conf
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

Comment 1 Susi Lehtola 2011-05-29 03:47:10 UTC
Please don't mix $RPM_BUILD_ROOT and %{buildroot}.

Comment 2 Nicoleau Fabien 2011-05-29 11:23:28 UTC
Updated :
Spec URL: http://rpms.nicoleau-fabien.net/SPECS/nomnom.spec
SRPM URL: http://rpms.nicoleau-fabien.net/SRPMS/nomnom-0.1.3-2.fc15.src.rpm

%changelog
* Sun May 29 2011 Nicoleau Fabien <nicoleau.fabien@gmail.com> 0.1.3-2
- Only use RPM_BUILD_ROOT

Comment 3 Thomas Spura 2011-05-30 21:15:46 UTC
The compression of the man pages could change, so %{_mandir}/man1/%{name}.1.* would be better.

Comment 4 Nicoleau Fabien 2011-05-31 18:48:33 UTC
Updated :
Spec URL: http://rpms.nicoleau-fabien.net/SPECS/nomnom.spec
SRPM URL: http://rpms.nicoleau-fabien.net/SRPMS/nomnom-0.1.3-3.fc15.src.rpm

%changelog
* Tue May 31 2011 Nicoleau Fabien <nicoleau.fabien@gmail.com> 0.1.3-3
- Manage a all compression of the man pages

About the default video player used (the xdg-open command), it was discussed here : http://www.spinics.net/lists/fedora-devel/msg151512.html
But I'm still not sure if it's a better solution than "totem".

Comment 5 Nicoleau Fabien 2011-06-24 20:01:20 UTC
Updated :
Spec URL: http://rpms.nicoleau-fabien.net/SPECS/nomnom.spec
SRPM URL: http://rpms.nicoleau-fabien.net/SRPMS/nomnom-0.1.4-1.fc15.src.rpm

%changelog
* Fri Jun 24 2011 Nicoleau Fabien <nicoleau.fabien@gmail.com> 0.1.4-1
- Update to 0.1.4

Comment 6 Nathan Owe 2011-08-11 03:09:13 UTC
Do you need a sponsor? If so add FE-NEEDSPONSOR to Blocks

Comment 7 Nicoleau Fabien 2011-08-14 17:52:55 UTC
Hi, I don't need a sponsor. I've already been approved. 
https://admin.fedoraproject.org/pkgdb/users/packages/eponyme

Comment 8 Volker Fröhlich 2011-08-27 07:06:23 UTC
License is GPLv3+, according to the headers.

Buildroot, clean section and the rm in the install section are only necessary, when you plan to go for EPEL5 or older.

Defattr is no longer necessary.

Please handle the locales as described in http://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files.

You won't need gettext though, but you'll have to append --with-qt to the %find_lang call.

I recommend putting each BuildRequires and Requires on a separate line.

Comment 9 Nicoleau Fabien 2011-08-27 17:21:10 UTC
Updates :
Spec URL: http://rpms.nicoleau-fabien.net/SPECS/nomnom.spec
SRPM URL: http://rpms.nicoleau-fabien.net/SRPMS/nomnom-0.1.4-2.fc15.src.rpm

Changelog :
- License fix
- Remove unnecessary commands
- Handle locales properly

Comment 10 Volker Fröhlich 2011-08-29 21:58:45 UTC
I don't seem to get the program working with Youtube on Fedora 15.

error: /usr/share/quvi/lua/website/youtube.lua:114: no match: fmt_url_map -->

https://bugzilla.redhat.com/show_bug.cgi?id=728646

Looks like the 2.16.1 doesn't work either: "Expected Perl-style regular expression, e.g. /pattern/flags"

Concerning %config(noreplace): I don't know NomNom, but I don't consider using noreplace a disadvantage. If the user made a change in that case, the old configuration stays in place and the new configuration is saved as .rpmnew. 

http://fedoraproject.org/wiki/Packaging:Guidelines#Configuration_files

Do you think keeping the old version's configuration might stop the new package's version from working?

You can also include a copy of the configuration file as an additional source, instead of specifying it line by line in the spec file. A more elegant way to do it, if you want to keep it in the spec for some reason:

cat > file_to_write_to <<EOF
your text

and even more text
EOF

Two remarks:

- It is common to leave a line blank between changelog entries, like so:

* Sat Aug 27 2011 Nicoleau Fabien <nicoleau.fabien@gmail.com> 0.1.4-2
- License fix
- Remove unnecessary commands
- Handle locales properly

* Fri Jun 24 2011 Nicoleau Fabien <nicoleau.fabien@gmail.com> 0.1.4-1

- "Remove unnecessary commands" is pretty generic and nobody reading only the changelog can guess what changed. Please be more specific!


From a packaging point of view, nothing is blocking the package. I'll make the formal review after we've sorted out, whether it is a quvi or NomNom problem.

Comment 11 Nicoleau Fabien 2011-08-30 11:19:36 UTC
I have the bug too, with nomnom, but not with clive or cclive, so I guess it's a nomnom bug. I'll report it to upstream.

Comment 12 Nicoleau Fabien 2011-09-02 20:11:09 UTC
Update :
Spec URL: http://rpms.nicoleau-fabien.net/SPECS/nomnom.spec
SRPM URL: http://rpms.nicoleau-fabien.net/SRPMS/nomnom-0.1.4-3.fc15.src.rpm

About the bug : there was an error when a configuration parameter had no default value. So I've updated the default configuration file, and the 0.1.5 will fix the bug (no error if the parameter has no default value).

I followed all your advices. Changelog :
- Use noreplace for configuration file
- Update default configuration file

Comment 13 Volker Fröhlich 2011-09-02 20:37:59 UTC
warning: File listed twice: /usr/share/nomnom/tr/nomnom_de.qm
warning: File listed twice: /usr/share/nomnom/tr/nomnom_fi.qm

Once by %{_datadir}/%{name}, which includes everything below that directory and once by findlang. If you just left out %{_datadir}/%{name}, the files are only listed once, but the directories are unowned. So let's make it:

%files -f %{name}.lang
%doc COPYING NEWS README doc/HowtoTranslate
%{_bindir}/%{name}
%dir %{_datadir}/%{name}
%dir %{_datadir}/%{name}/tr
%{_datadir}/applications/%{name}.desktop
%{_datadir}/pixmaps/%{name}.xpm
%config(noreplace) %{_sysconfdir}/NomNom.conf
%{_mandir}/man1/%{name}.1.*

Besides that, all is fine.

Comment 14 Nicoleau Fabien 2011-09-02 21:14:45 UTC
Update :
Spec URL: http://rpms.nicoleau-fabien.net/SPECS/nomnom.spec
SRPM URL: http://rpms.nicoleau-fabien.net/SRPMS/nomnom-0.1.4-4.fc15.src.rpm

Changelog :
- Fix files section to avoid files listed twice

Comment 15 Volker Fröhlich 2011-09-02 22:03:15 UTC
Review:

[+] Good
[-] Needs work
[0] Does not apply

MUST:
=====
[+] rpmlint:
[makerpm@fedora15 rpmbuild]$ rpmlint SRPMS/nomnom-0.1.4-4.fc15.src.rpm RPMS/x86_64/nomnom-*0.1.4-4*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

It's the same after having the package installed.

[+] Naming according to the Package Naming Guidelines
[+] Spec file matches base package name
[+] Packaging guidelines met
[+] License approved for Fedora
[+] License field in spec matches code
[+] License file included, if source package includes it
[+] Spec in American English
[+] Spec is legible
[+] Sources match upstream md5sum: 9104c74746aa1e6c9014c53b4b067bbd
[+] Compiles and builds into binary RPMs on at least one primary architecture: x86_64, i386
[0] ExcludeArch is specified and commented
[+] Locales are handled correctly
[+] All build dependencies listed
[0] Calls ldconfig for its shared libraries
[+] No bundled system libraries
[0] Stated as relocatable package
[+] Owns all its directories or requires package that does
[+] No file listing duplicates
[+] File permissions correct
[+] Consistent use of macros
[+] Code or permissible content
[0] Large documentation in -doc subpackage
[+] No runtime dependency of files listed as %doc
[0] Header files in -devel subpackage
[0] Static files in -static subpackage
[0] Library files without suffix in -devel subpackage
[0] Devel-package requires base package
[0] No .la libtool archives
[+] GUI application includes properly installed %{name}.desktop file
[+] No files or directories owned, that other packages own
[+] Filenames in packages are UTF-8

SHOULD:
=======

[0] Query upstream if no license text is included
[+] Package builds in mock:
fedora-rawhide-x86_64, fedora-15-i386
[+] Package works as described
[0] Scriptlets are sane, if used
[0] Subpackages other than -devel should require base package (versioned)
[0] pkgconfig files in -devel subpackage
[0] Dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider
requiring the package which provides the file instead of the file itself
[+] Contain man pages, where they make sense

===APPROVED===

Comment 16 Nicoleau Fabien 2011-09-02 22:34:58 UTC
Thank you for this review Volker.

New Package SCM Request
=======================
Package Name: nomnom
Short Description: The graphical video download tool
Owners: eponyme
Branches: f15 f16
InitialCC:

Comment 17 Gwyn Ciesla 2011-09-03 16:49:03 UTC
Git done (by process-git-requests).

Comment 18 Fedora Update System 2011-09-04 17:08:31 UTC
nomnom-0.1.4-4.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/nomnom-0.1.4-4.fc16

Comment 19 Fedora Update System 2011-09-04 17:09:53 UTC
nomnom-0.1.4-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/nomnom-0.1.4-4.fc15

Comment 20 Fedora Update System 2011-09-06 18:06:59 UTC
nomnom-0.1.4-4.fc16 has been pushed to the Fedora 16 testing repository.

Comment 21 Fedora Update System 2011-09-14 22:33:13 UTC
nomnom-0.1.4-4.fc15 has been pushed to the Fedora 15 stable repository.

Comment 22 Fedora Update System 2011-09-30 18:33:09 UTC
nomnom-0.1.4-4.fc16 has been pushed to the Fedora 16 stable repository.


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