Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: yasdi - Handle communications with SMA solar/wind inverters
https://bugzilla.redhat.com/show_bug.cgi?id=670298
Summary: Review Request: yasdi - Handle communications with SMA solar/wind inverters Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: dave@adsllc.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
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.r... Koji URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=2726920 Description: Library and application to communicate with SMA solar/wind inverters
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@golotop.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |packages@golotop.de
--- Comment #1 from Golo Fuchert packages@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_en...
- 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!
package-review@lists.fedoraproject.org