Bug 819274
Summary: | Review Request: radeonhd-power - power settings for radeon cards | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Victor Costan <costan> |
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
Status: | CLOSED WONTFIX | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | collura, orion, package-review |
Target Milestone: | --- | Keywords: | Reopened |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2013-11-21 16:03:10 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
Victor Costan
2012-05-06 07:13:02 UTC
I submitted the SRPM to koji. http://koji.fedoraproject.org/koji/taskinfo?taskID=4057617 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 pwnall-radeonhd-power-ee4f3db/ pwnall-radeonhd-power-ee4f3db/LICENSE pwnall-radeonhd-power-ee4f3db/Makefile pwnall-radeonhd-power-ee4f3db/src/ pwnall-radeonhd-power-ee4f3db/src/radeonhd-power pwnall-radeonhd-power-ee4f3db/src/radeonhd-power.service 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: https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Version 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! @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! Given the Radeon DPM support in kernels 3.11 and above, this package isn't useful anymore. |