Bug 427618

Summary:	Review Request: nvclock - Utility that allows users to overclock NVIDIA based video cards
Product:	[Fedora] Fedora	Reporter:	Adel Gadllah <adel.gadllah>
Component:	Package Review	Assignee:	Dominik 'Rathann' Mierzejewski <dominik>
Status:	CLOSED NEXTRELEASE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	fedora-package-review, notting
Target Milestone:	---	Keywords:	Reopened
Target Release:	---	Flags:	dominik: fedora-review+ kevin: fedora-cvs+
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2008-01-07 15:32:55 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:

Description Adel Gadllah 2008-01-05 14:08:00 UTC

Spec URL: http://tgmweb.at/gadllah/nvclock.spec
SRPM URL: http://tgmweb.at/gadllah/nvclock-0.8-0.1.b3.fc8.src.rpm
Description: 
Nvclock allows you to tweak your Nvidia card under Linux.
It features overclocking, hardware monitoring, tweaking
of OpenGL settings, and much more.

Comment 1 Dominik 'Rathann' Mierzejewski 2008-01-05 14:40:55 UTC

Taking review.

Comment 2 Dominik 'Rathann' Mierzejewski 2008-01-05 17:34:31 UTC

rpmlint output clean:
$ rpmlint /var/lib/mock//fedora-development-i386/result
nvclock-gtk.i386: W: no-documentation

Source md5 matches upstream:
eab0a35a79f0165310be759b043deedb  nvclock0.8b3.tar.gz

Builds in mock: devel/i386, F-7/x86_64

Problems:
1. License field says GPLv2+, but
src/libc_wrapper.c has MIT license (GPLv2+ compatible)
src/backend/i2c.c and nv40.c have some additional (apart from the GPL) clauses -
can you verify that they don't impose any restrictions over GPL terms?

2. This part could use a little more spacing for legibility:
[...]
%package gtk
Summary: Nvclock gtk frontend
Group: Applications/System
%description gtk
This package contains the nvclock gui.
[...]

3. make is called without %{?_smp_mflag} and no comment is present explaining why.

4. Since the icon you add is 48x48, why not put it in
%{_datadir}/icons/hicolor/48x48/apps/
?
Of course, then you need to add
%post gtk
touch --no-create %{_datadir}/icons/hicolor
if [ -x %{_bindir}/gtk-update-icon-cache ]; then
  %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
fi

%postun gtk
touch --no-create %{_datadir}/icons/hicolor
if [ -x %{_bindir}/gtk-update-icon-cache ]; then
  %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
fi

5. Why aren't you building the QT interface? AFAICT it builds and works just as
well as the GTK interface.

Comment 3 Adel Gadllah 2008-01-05 18:38:08 UTC

(In reply to comment #2)

> Problems:
> 1. License field says GPLv2+, but
> src/libc_wrapper.c has MIT license (GPLv2+ compatible)
so should I add GPLv2+ and MIT ?
> src/backend/i2c.c and nv40.c have some additional (apart from the GPL) clauses -
> can you verify that they don't impose any restrictions over GPL terms?

I asked upstream and got this response 
"it are just some pieces from xfree86, the same stuff that is now in the xorg nv
driver"

> 2. This part could use a little more spacing for legibility:
> [...]
> %package gtk
> Summary: Nvclock gtk frontend
> Group: Applications/System
> %description gtk
> This package contains the nvclock gui.
> [...]

ok

> 3. make is called without %{?_smp_mflag} and no comment is present explaining why.

because the build fails; have not yet investigated why .. will add a comment.

> 4. Since the icon you add is 48x48, why not put it in
> %{_datadir}/icons/hicolor/48x48/apps/
> ?
> Of course, then you need to add
> %post gtk
> touch --no-create %{_datadir}/icons/hicolor
> if [ -x %{_bindir}/gtk-update-icon-cache ]; then
>   %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
> fi
> 
> %postun gtk
> touch --no-create %{_datadir}/icons/hicolor
> if [ -x %{_bindir}/gtk-update-icon-cache ]; then
>   %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
> fi

OK, will do.

> 5. Why aren't you building the QT interface? AFAICT it builds and works just as
> well as the GTK interface.
> 

Because its unmaintained and upstream is planning to drop it completly.

Comment 4 Adel Gadllah 2008-01-05 18:46:20 UTC

Fixed SPEC/SRPM:

http://tgmweb.at/gadllah/nvclock.spec
http://tgmweb.at/gadllah/nvclock-0.8-0.2.b3.fc8.src.rpm

Comment 5 Dominik 'Rathann' Mierzejewski 2008-01-06 00:42:44 UTC

Well then, everything seems to be in order. No further comments.

APPROVED.

Comment 6 Adel Gadllah 2008-01-06 01:23:19 UTC

(In reply to comment #5)
> Well then, everything seems to be in order. No further comments.
> 
> APPROVED.

Ok, thx for the review!

==============

New Package CVS Request
=======================
Package Name: nvclock
Short Description: Utility that allows users to overclock NVIDIA based video cards
Owners: drago01
Branches: F-7 F-8
Cvsextras Commits: yes

Comment 7 Kevin Fenzi 2008-01-06 17:39:39 UTC

cvs done.

Comment 8 Dominik 'Rathann' Mierzejewski 2008-01-07 15:32:30 UTC

Reopening to reassign to myself.