Bug 819274 - Review Request: radeonhd-power - power settings for radeon cards
Review Request: radeonhd-power - power settings for radeon cards
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
: Reopened
Depends On:
  Show dependency treegraph
Reported: 2012-05-06 03:13 EDT by Victor Costan
Modified: 2014-01-16 04:21 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2013-11-21 11:03:10 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

  None (edit)
Description Victor Costan 2012-05-06 03:13:02 EDT
Spec URL: https://raw.github.com/pwnall/radeonhd-power.fc/master/radeonhd-power.spec
SRPM URL: http://people.csail.mit.edu/costan/fc/radeonhd-power-1.0-1.fc17.src.rpm
Description: I wrote a script to change the radeon power settings on boot, so my fans won't go into high speed. The script currently sets the card into low power, but I can make it try to read a file from /etc/ if that's desirable.

A couple of blog posts describe how to do this manually, so I think the script would be useful.

I'm enjoying Fedora, and I hope I can contribute to it. Thank you for taking the time to review this, and I look forward to your feedback!
Comment 1 Victor Costan 2012-05-06 03:17:17 EDT
I submitted the SRPM to koji.

Comment 2 Michael Schwendt 2012-05-07 15:32:37 EDT
Here's just some feedback as I've run into this review request and found something surprising:

> %prep
> tar -xzf %{_sourcedir}/v%{version}
> mv pwnall-radeonhd-power-* %{name}-%{version}
> %setup -q -T -D -n %{name}-%{version}

Is this an obfuscation contest? ;) This made me curious. First of all, one would use the %setup command for extracting ordinary source tarballs. It can extract tar.gz archives just fine. And the '*' wildcard in your "mv" command is fragile. Consider which directory you've entered when you run "tar -xzf …". So, a closer look:

> Source0:	https://github.com/pwnall/radeonhd-power/tarball/v%{version}

$ rpmls -p radeonhd-power-1.0-1.fc17.src.rpm
-rw-rw-r--  radeonhd-power.spec
-rw-rw-r--  v1.0

Uh? A file "v1.0" indeed? Why would you not give it a slightly more meaningful file name, at least?

$ file v1.0 
v1.0: gzip compressed data, from Unix

$ tar ftz v1.0 

The following %setup invocation would be much more clear:

  %setup -q -n pwnall-radeonhd-power-ee4f3db

Of course, parts of it can be moved into RPM macros to be defined at the top of the spec file (especially if they change often). Instead, you try to hide the truncated git hash of an unnamed snapshot whose archive contents don't refer to "1.0" anywhere either. That's questionable. The Packaging Guidelines refer to snapshots:

Comment 3 Victor Costan 2012-05-07 17:16:13 EDT
Thank you for the very quick turnaround!

I wasn't trying to obfuscate anything, I'm sorry this is how it seems. I'm using github to host my code, and I tagged version 1.0 using the v1.0 tag. When I wget the file from the URL in Source0, it comes out as "v1.0", with no extension. Then, to make things worse, github adds the short commit hash to the directory name, even though I used a named tag to get the tarball. That's why I thought I couldn't use setup, and I have that mess in the RPM file. I'll re-read the guidelines and try to come back with a better solution soon.

Once again, thank you for taking a look at my spec so quickly!
Comment 4 Victor Costan 2013-03-25 11:46:52 EDT
@Michael Schwendt: GitHub has a new archive layout and I took advantage of that to update my package. I hope this addresses your feedback!

Is there any chance you can take another look?

Thank you so much!
Comment 5 Orion Poplawski 2013-10-20 21:57:48 EDT
Please read https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd
Comment 6 Victor Costan 2013-11-21 10:34:54 EST
Given the Radeon DPM support in kernels 3.11 and above, this package isn't useful anymore.

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