Bug 670298
Summary: | Review Request: yasdi - Handle communications with SMA solar/wind inverters | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Dave Ludlow <dave> |
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
Status: | CLOSED NOTABUG | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | notting, package-review, packages |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2012-04-26 01:34:14 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
Dave Ludlow
2011-01-17 20:00:56 UTC
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! It's been 15 months since the above commentary with no response; I'll just close this out. |