Bug 670298 - Review Request: yasdi - Handle communications with SMA solar/wind inverters
Review Request: yasdi - Handle communications with SMA solar/wind inverters
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-17 15:00 EST by Dave Ludlow
Modified: 2012-04-25 21:34 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-04-25 21:34:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Dave Ludlow 2011-01-17 15:00:56 EST
Spec URL: http://adsllc.fedorapeople.org/rpmbuild/SPECS/yasdi.spec
SRPM URL: http://adsllc.fedorapeople.org/rpmbuild/SRPMS/yasdi-1.8.1build9-1.fc14.src.rpm
Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=2726920
Description: Library and application to communicate with SMA solar/wind inverters
Comment 1 Golo Fuchert 2011-01-23 14:13:10 EST
Hello Dave,

here are some initial comments to improve this package and make it ready for a review.

- The base package is empty, this makes no sense. As far as I understand right now, the libraries are absolutely necessary, so why not put them in the base package (I didn't have a very deep look yet, so maybe this is not true. But still, something has to be in the base package!)

- Right now, the doc subpackage is empty, put something in there or remove it completely.

- the %doc files (README, ...) belong in the base package here.

- I think the "defines" at the beginning are not needed. Why do you do this?
%name, %release and %version are defined automatically by the corresponding fields, %libmajor, %libminor and %libversion are not needed.

- the "build9" belongs to the release tag, not version (see https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Version_Tag )

- Please use the %cmake macro and its definition of the cflags (see http://fedoraproject.org/wiki/Packaging/cmake ) 

- You might want to reconsider the "Yet another" at the beginning of the summary. It seems very informal to me (yes, I realized it's part of the name).

- The URL doesn't link to the homepage of the software ( http://www.sma.de/de/produkte/software/yasdi.html )

- the require field of the devel subpackage is not complete.

- The long sed section should be put in a patch and sent upstream, do you agree?

- the chmods could be shortened with a find.

- it would be better to use pushd and popd instead of cd in %build

- You have to use macros, not absolute paths!

- dos2unix changes the timestamps of the files, this could be done better:
http://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Convert_encoding_to_UTF-8 

- The directory %{includedir}/libyasdi/ has to be owned by the package

It may seem like a lot, but I think most issues can be fixed very quickly!
Comment 2 Jason Tibbitts 2012-04-25 21:34:14 EDT
It's been 15 months since the above commentary with no response; I'll just close this out.

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