Bug 459444 - Review Request: ctdb - Clustered TDB
Summary: Review Request: ctdb - Clustered TDB
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Chris Feist
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-08-18 23:20 UTC by Abhijith Das
Modified: 2013-08-20 19:36 UTC (History)
14 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2009-02-24 14:51:37 UTC
Type: ---
Embargoed:
katzj: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Abhijith Das 2008-08-18 23:20:57 UTC
Spec URL: <spec info here>
SRPM URL: <srpm info here>
Description: ctdb is the clustered database used by samba

This is my first package and I'm seeking a sponsor. I also need webspace to host my specfile and srpm.

Comment 1 Peter Lemenkov 2008-08-23 13:23:12 UTC
Feel free to host necessary files at fedorapeople.org

Comment 2 Abhijith Das 2008-08-25 04:41:13 UTC
This is my first package. I was told that I would get fedorapeople.org space only after submitting my first package. Anyhow, I should be able borrow some webspace and upload the package. Will post a link here.

Thanks!
--Abhi

Comment 3 Abhijith Das 2008-08-25 20:00:56 UTC
http://people.redhat.com/rpeterso/Experimental/RHEL5.x/samba/

Please find the srpm and ctdb.spec at the above location. This package currently builds in F9; we're still working out a couple of errors building with rawhide. Please review and offer your comments.

Thanks much!
--Abhi

Comment 4 Abhijith Das 2008-09-09 16:57:37 UTC
The rawhide issue is resolved and the srpms based on the latest upstream ctdb release are here:

For F9:
http://people.redhat.com/rpeterso/Experimental/Fedora/samba/ctdb-1.0.58-1.fc9.src.rpm

For Rawhide:
http://people.redhat.com/rpeterso/Experimental/Fedora/samba/ctdb-1.0.58-1.fc10.src.rpm

Comment 5 Chris Feist 2008-09-22 21:36:36 UTC
rpmlint runs cleanly.

- MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. Refer to the  Guidelines for examples.

Looks like you need to add '%{_sysconfdir}/ctdb' into the file list.

Otherwise everything else looks good.

Comment 6 Abhijith Das 2008-09-22 22:12:14 UTC
I made the change and re-uploaded the srpms. You can find them in the same locations mentioned in comment #4

Thanks!
--Abhi

Comment 7 Chris Feist 2008-09-23 19:54:19 UTC
Everything looks good, now that the changes have been made.

Approved.

Comment 8 Kevin Fenzi 2008-11-14 05:29:58 UTC
A few quick questions: 

Upstream seems to have: 
http://ctdb.samba.org/packages/redhat/RHEL5/ctdb-1.0.65.tgz

Any reason not to use that for Source url or to update to it?

This doesn't seem to build on ppc/ppc64. See: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=933174
for the scratch build I did. 

Finally, you need a cvs template here. See: 
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

Comment 9 Kevin Fenzi 2008-11-16 20:13:44 UTC
Clearing cvs flag for now. Reset it once everything is ready.

Comment 10 Chris Feist 2008-12-15 22:09:46 UTC
New Package CVS Request
=======================
Package Name: ctdb
Short Description: Clustered TDB used by Samba
Owners: cfeist adas
Branches: F-9 F-10
InitialCC: cfeist adas

Comment 11 Chris Feist 2008-12-15 22:11:10 UTC
I'll make sure we get upgraded to the latest version of ctdb & fix build issues (if possible) on ppc/ppc64 once we can build this package in the build system.  (We don't have access to F-10 ppc/ppc64 systems currently).

Thanks,
Chris

Comment 12 Fabio Massimo Di Nitto 2008-12-16 05:22:42 UTC
Hi Chris,

I can provide limited (across the 24h) access to F10 ppc/ppc64. Please let me know if you need it.

Comment 13 Kevin Fenzi 2008-12-17 21:51:46 UTC
I don't see Abhijith in the packager group. 
Is this your first fedora package? You will need a sponsor...

Comment 14 Chris Feist 2008-12-22 19:52:01 UTC
Kevin,

Can you make me the owner (at least for now until Abhi gets a sponsor)?

Thanks,
Chris

Comment 15 Kevin Fenzi 2008-12-22 23:59:52 UTC
Sadly no, unless you get someone else to review this package. 
You can't both be reviewer and maintainer of a new package. ;(

Comment 16 Tom "spot" Callaway 2009-01-13 17:02:37 UTC
(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.

Comment 17 Tom "spot" Callaway 2009-01-13 18:31:15 UTC
(In reply to comment #16)

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

This is wrong. I forgot that %attr only takes three parameters. :) Sorry for the confusion.

Comment 18 Guenther Deschner 2009-02-06 16:56:56 UTC
Sumit Bose <sbose> and me, Guenther (samba co-maintainer) had a look
at this again, and we couldn't find a recent srpm, just the one from August
last year.
Is there actually a more recent one somewhere ?

So, we just took the existing one, updated it to the recent ctdb release and
looked into all the discussed issues in this thread.

A fixed and updated package can now be found here:
http://sbose.fedorapeople.org/ctdb-1.0.71-2.fc10.src.rpm

Both of us, Sumit and me, really want to get this into Fedora as soon as
possible in order to get Samba use and link against it.

How can we help to speed up this process ? I think Sumit would agree to become
ctdb maintainer if necessary, and I can help reviewing it if still required.

What are the next steps?

Comment 19 Abhijith Das 2009-02-06 17:43:47 UTC
Guenther,
Thanks for fixing the srpm. I was gonna update it myself shortly, but you beat me to it.

I took a brief look at the spec file and I spotted a couple of things:
a. You're missing changelog info between -58 and -71. I usually pull this stuff from the upstream srpm's spec file: http://ctdb.samba.org/packages/redhat/RHEL5/ctdb-1.0.71-1.src.rpm. It might also be worthwhile (if you've not already done so) to check for any changes upstream made to their spec file since -58 that we might've missed.

b. I ran rpmlint -i on the ctdb.spec and:
[adas@radium sbose]$ rpmlint -i ctdb.spec 
ctdb.spec:82: E: use-of-RPM_SOURCE_DIR
You use $RPM_SOURCE_DIR or %{_sourcedir} in your spec file. If you have to use
a directory for building, use $RPM_BUILD_ROOT instead.

c. I also ran rpmlint -i on the binaries:
ctdb.x86_64: W: incoherent-version-in-changelog 1.0.58-2 1.0.71-2.fc10
The last entry in %changelog contains a version identifier that is not
coherent with the epoch:version-release tuple of the package.
^^ This is the changelog stuff I mentioned above

ctdb.x86_64: W: unstripped-binary-or-object /usr/sbin/ctdbd
ctdb.x86_64: W: unstripped-binary-or-object /usr/bin/smnotify
ctdb.x86_64: W: unstripped-binary-or-object /usr/bin/ctdb_ipmux
ctdb.x86_64: W: unstripped-binary-or-object /usr/bin/ctdb
ctdb.x86_64: W: service-default-enabled /etc/rc.d/init.d/ctdb
The service is enabled by default after "chkconfig --add"; for security
reasons, most services should not be. Use "-" as the default runlevel in the
init script's "chkconfig:" line and/or remove the "Default-Start:" LSB keyword
to fix this if appropriate for this service.

ctdb-devel.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.
^^ I was told that this was ok, before... maybe spot can clarify.

2 packages and 0 specfiles checked; 0 errors, 7 warnings.


The next step is that we wait for spot to review, approve and sponsor, so this package gets into Fedora.

Comment 20 Sumit Bose 2009-02-06 19:16:56 UTC
Hi Abhi,

I have uploaded a new version: 
http://sbose.fedorapeople.org/ctdb-1.0.71-3.fc10.src.rpm

a. done
b. I use %{_sourcedir} to include a license file, it can be removed from the spec file as soon as upstream will include it.
c. done, I do not get the unstripped-binary-or-object here

Comment 21 Tom "spot" Callaway 2009-02-07 06:08:47 UTC
I'm not going to have a chance to look at this until I get back from FOSDEM, which means, sometime next week.

Comment 22 Sumit Bose 2009-02-08 18:13:03 UTC
Ronnie Sahlberg added a COPYING file upstream. I have include the patch and uploaded the latest version to:
http://sbose.fedorapeople.org/ctdb-1.0.71-4.fc10.src.rpm

Comment 23 Jeremy Katz 2009-02-17 15:33:38 UTC
Okay, picking up the review, there are still some things that aren't quite right here

There are two that are definitely blockers
* What is the canonical upstream location?  The repo mentioned on http://ctdb.samba.org/download.html doesn't have the version being packaged or the included COPYING file.  
* In the %postun, you should be doing condrestart, not restart -- otherwise, an upgrade will end up always starting ctdb for people.  


These are smaller things, but really should be fixed also
* If your source is really just a git archive, please follow the guidelines for those (https://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control) as opposed to a one-off directory which isn't keeping old versions of the tarballs
* Rather than copying docs into docdir directly, you can just list them with
    %doc COPYING
    %doc doc/ctdb
or similar in the %files section -- note that these paths are then relative to the source dir
* There still needs to be a comment for the patches
* Why is /etc/ctdb/statd-callout in /etc?  General purpose scripts generally shouldn't be in /etc
* Is the build process not safe for make -j?  


And the final thing, who is actually intending to own this package now and is a sponsor still needed?

Comment 24 Sumit Bose 2009-02-17 22:24:13 UTC
Thank you for the review.

I have addressed: 
- upstream location/source from git archive issue by adding a description about how to create the tar file from the git repo as describes in the guidelines
- changed restart to condrestart
- added comments to the patches
- use make %{_smp_mflags}

I have not
- changed the copying of docs, because the makefile already creates a docdir with some files and this seems to conflict with using %doc in the %files section
- moved statd-callout, because I think that people who are currently using the upstream version are used to the /etc location, but I will talk to the upstream maintainer about moving it.

The new version can be found under http://sbose.fedorapeople.org/ctdb-1.0.71-5.fc10.src.rpm

If adas does not mind I can take the ownership for this package, but I would need a sponsor.

Comment 25 Jeremy Katz 2009-02-18 03:34:49 UTC
Okay, looks good now.  Approved and I can sponsor you.  Find me on IRC tomorrow (jeremy) and we can go through those bits.

Also, minor point to save you some work in the future -- you can use the tag in the git archive command to avoid having to use the hash of the commit.

Comment 26 Sumit Bose 2009-02-18 20:46:09 UTC
New Package CVS Request
=======================
Package Name: ctdb
Short Description: A Clustered Database based on Samba's Trivial Database (TDB)
Owners: sbose
Branches: F-9 F-10
InitialCC: adas gd simo

Comment 27 Kevin Fenzi 2009-02-19 20:21:00 UTC
cvs done.

Comment 28 Sumit Bose 2009-05-11 18:07:57 UTC
Package Change Request
======================
Package Name: ctdb
Owners: sbose
New Branches: EL-5

To test the integration with samba on RHEL it would be very helpful to have ctdb in 5E-epel.

Comment 29 Kevin Fenzi 2009-05-13 05:15:05 UTC
cvs done.


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