[Bug 819274] Review Request: radeonhd-power - power settings for radeon cards

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


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

--- Comment #2 from Michael Schwendt <mschwendt@xxxxxxxxx> 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 
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

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review



[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]