[Bug 670298] Review Request: yasdi - Handle communications with SMA solar/wind inverters

[bugzilla at redhat.com]

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=670298

Golo Fuchert <packages at golotop.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |packages at golotop.de

--- Comment #1 from Golo Fuchert <packages at golotop.de> 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!

-- 
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.