[Bug 459444] Review Request: ctdb - Clustered TDB

[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=459444


Tom "spot" Callaway <tcallawa at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tcallawa at redhat.com
               Flag|fedora-review+              |fedora-review?




--- Comment #16 from Tom "spot" Callaway <tcallawa at redhat.com>  2009-01-13 12:02:37 EDT ---
(In reply to comment #6)
> I made the change and re-uploaded the srpms. You can find them in the same
> locations mentioned in comment #4
> 
> Thanks!
> --Abhi

Abhi, there are a few more changes I'd like to see you make, and then I'd be
willing to sponsor you. Chris, please look over this, as you should have caught
some of these before approving this package.

* Please update this package to the latest release, which seems to be 1.0.68
* In Source0, use the full URL to the upstream source. RPM is smart enough to
know how to parse that. (Don't worry, it doesn't try to download it, it uses
the local file)
* For Patch0, please submit it for upstream inclusion, and leave a comment next
to it describing where you sent it (a URL to an open bug is sufficient, for
example). See:
https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
* The License tag is wrong, it should actually be: GPLv3+
* It doesn't look like the upstream source comes with a copy of the license.
You should contact upstream to ask them to add it for future releases (and when
they do, it should be packaged as %doc). See: 
https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
* Flesh out the Summary a bit more, so that it avoids acronyms. I have no idea
what a TDB is. Even simply "A Clustered Database" would be better.
* In your CFLAGS, you're overriding the Fedora optimization flag by passing
-O0. Don't do that. :)
* You're also using ./configure rather than the %configure macro. Please use
%configure instead (it defines all of the flags you're passing in the same
way). In your spec, you can simply do:

CFLAGS="$(echo '%{optflags}') $EXTRA -D_GNU_SOURCE
-DCTDB_VERS=\"%{version}-%{release}\"" %configure

* Be sure you're using "%defattr(-,root,root,-)" in every %files section.
* Also, be sure you're passing four parameters for %attr (if you don't need to
change the fourth field <dir mode>, just use a - ).
* Your %files sections need to be cleaned up. You're duplicating files there.
One trick that should help you out quite a bit: If you specify a directory and
end it with a /, RPM will own that directory, and all the files and directories
below it. So, for example, instead of:

%{_sysconfdir}/ctdb
%{_sysconfdir}/ctdb/events.d/00.ctdb
%{_sysconfdir}/ctdb/events.d/10.interface
%{_sysconfdir}/ctdb/events.d/40.vsftpd
...

Just use:
%{_sysconfdir}/ctdb/

When you're test building your package, RPM will complain about these
duplicated files. Be sure you've got rid of any such warnings.

#########
If you have not done so, please take a moment and read:
https://fedoraproject.org/wiki/Packaging/Guidelines
https://fedoraproject.org/wiki/Packaging/ReviewGuidelines

When you've gotten all of those items resolved, please upload a new SRPM and
put a link in this bugzilla ticket. I'll come back and look at it and then
re-approve and sponsor you.

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