Bug 427618 - Review Request: nvclock - Utility that allows users to overclock NVIDIA based video cards
Review Request: nvclock - Utility that allows users to overclock NVIDIA based...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dominik 'Rathann' Mierzejewski
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-05 09:08 EST by Adel Gadllah
Modified: 2008-01-07 10:32 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-07 10:32:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
dominik: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Adel Gadllah 2008-01-05 09:08:00 EST
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 09:40:55 EST
Taking review.
Comment 2 Dominik 'Rathann' Mierzejewski 2008-01-05 12:34:31 EST
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 13:38:08 EST
(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 5 Dominik 'Rathann' Mierzejewski 2008-01-05 19:42:44 EST
Well then, everything seems to be in order. No further comments.

APPROVED.
Comment 6 Adel Gadllah 2008-01-05 20:23:19 EST
(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 12:39:39 EST
cvs done.
Comment 8 Dominik 'Rathann' Mierzejewski 2008-01-07 10:32:30 EST
Reopening to reassign to myself.

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