Spec URL: http://rubenkerkhof.com/packages/csync2.spec SRPM URL: http://rubenkerkhof.com/packages/csync2-1.33-1.src.rpm Description: Csync2 is a cluster synchronization tool. It can be used to keep files on multiple hosts in a cluster in sync. Csync2 can handle complex setups with much more than just 2 hosts, handle file deletions and can detect conflicts. It is expedient for HA-clusters, HPC-clusters, COWs and server farms.
The fact that you install csync2.xinetd makes me think that you must Require xinetd. Also, since the policy in Fedora is to NOT install services in a configuration which leaves them activated by default, I think that you should patch csync2.xinetd and replace "disable=no" with "disable=yes", so that it would not automatically start when the next service xinetd start/restart occurs. The admin should specifically activate the new service.
Hi Manuel, >>The fact that you install csync2.xinetd makes me think that you must require xinetd. Fixed >> Also, since the policy in Fedora is to NOT install services in a >> configuration which leaves them activated by default, I think that you should >> patch csync2.xinetd and replace "disable=no" with "disable=yes", so that it >> would not automatically start when the next service xinetd start/restart occurs. >> The admin should specifically activate the new service. Fixed as well. New versions: http://rubenkerkhof.com/packages/csync2.spec http://rubenkerkhof.com/packages/csync2-1.33-2.src.rpm
Good: - rpmlint checks do not return anything either on the source or on the binary rpm - package meets naming guidelines - package meets packaging guidelines - buildroot has the recommended value - license ( GPL ) is OK, text in %doc, matches source - spec file legible, in am. english - source is last version and matches upstream, sha1sum 8d94eb0c7ff997598be7b1cfc444bb74eae6712d csync2-1.33.tar.gz - package builds in mock for devel (i386) and FC6 (x86_64) - no missing BR - no unnecessary BR - no locales - not relocatable - owns all directories that it creates, does not own foreign files or directories - no duplicate files - permissions ok - %clean ok - macro use consistent - code, not content - available documentation is included, but small so no need for -docs - nothing in %doc affects runtime - no need for .desktop file - no static, headers, libtool or .pc files SHOULD - builds fine in mock (FC6/x86_64, devel/i386) - runs OK on FC6/x86_64 (once the proper instructions -- included in the rpm -- are followed, that is csync2 is added to /etc/services) Suggestion: the changelog entry should describe the actual modifications (especially since they are small - a one liner should suffice) rather then point to a bugzilla entry (it makes life simpler for someone examining the rpms, avoiding the need for an extra web query; not to mention that bugzilla might be down or simply not accessible) As a sidenote, you could have used a 1 line sed command in %install in order to modify the offending line in xinetd.conf, rather then creating a separate patch, but your method is perfectly fine. In this moment I see no blockers, but before approving this package I would like for someone more experienced to comment on the following: In order to start the program (via xinetd), /etc/services must be modified, adding a line specifying the port and protocol used by csync2 and SSL certificates must be generated for each box where the program will run Editing /etc/services could be handled either in a %post entry (in the sources there is a spec file which does the addition, but does not verify if the entry already exists and neither removes it at uninstall time), or (as the packager has chosen) manually by the admin. My question is: which one of this two methods is the proper procedure for Fedora ?
Thanks for the review Manuel. My comments inline: >> As a sidenote, you could have used a 1 line sed command in %install in >> order to modify the offending line in xinetd.conf, rather then creating a >> separate patch, but your method is perfectly fine. I actually changed 2 lines in csync2.xinetd ;-) I prefer clean patches to regular expressions in spec files. That way, if something changes in the source, the patch fails to apply and I can fix it. >> In order to start the program (via xinetd), /etc/services must be modified, >> adding a line specifying the port and protocol used by csync2 and SSL >> certificates must be generated for each box where the program will run There's no need to edit /etc/services, since xinetd doesn't require it. I tried creating the ssl certificates in %install (make cert does this), but in mock, openssl seems to have problems opening a random device. I'm not sure what's the best way to handle this. Maybe just ship a self-signed cert with the rpm?
I fully agree with clean patches, but for very small modifications I prefer one liners to separate patches. And since you could have done "s/default on/default off; s/disable=no/disable=yes" one line would have been enough :) Serious stuff now: In my case, the first attempt to run csync2 on FC6 (without editing /etc/services) triggered this error: Jan 24 03:27:18 wolfy Installed: xinetd.x86_64 2:2.3.14-8 Jan 24 03:27:19 wolfy Installed: csync2.x86_64 1.33-2.fc6 Jan 24 03:27:58 wolfy xinetd[19733]: Port not specified and can't find service: csync2 with getservbyname Jan 24 03:27:58 wolfy xinetd[19733]: xinetd Version 2.3.14 started with libwrap loadavg labeled-networking options compiled in. Jan 24 03:27:58 wolfy xinetd[19733]: Started working: 0 available services As of creating the certs, I'd include the relevant part of the Makefile rather then creating the certificates by default. Maybe because I generate ALL certificates in only one place and transfer them wherever they are needed...
man xinetd(8): xinetd is not limited to services listed in /etc/services. Therefore, anybody can use xinetd to start special-purpose servers. But for that to work, you probably have to set "type" to UNLISTED. I'm at work now, but I'll see what I can come up with tonight.
Just tested. One can either edit /etc/services or add the following two lines to /etc/xinetd.d/csync2: type = UNLISTED port = <desired port value> Which makes me suggest the following - modify a bit your patch as to include these two lines, using the default port value assumed by csync2 ( 30865 ) - add a README.FEDORA pointing this difference towards the included docs - bug upstream to modify the docs (the paper.pdf) What do you think ?
Sounds good to me. Will do that tonight.
New version: http://rubenkerkhof.com/packages/csync2-1.33-3.src.rpm http://rubenkerkhof.com/packages/csync2.spec One rpmlint error though because I changed permissions to 640 on the ssl key, but that's a lot safer than world readable.
I am sorry, but I definitely MUST veto the inclusion of certificates in the binary rpm. The reason is that this would lead to each and every computer where the rpm is installed to have the very same certificates. Which kind of defeats the whole purpose of certificates. If you want to help the users, you could use for instance one of the following approaches: a) - extract the certificate generation part from the Makefile included in sources and save it as a separate mkcert.sh - install this file as a %doc - document it in the README.FEDORA file I have suggested in comment #7 b) - generate the certificates in %post Personally I believe that the first approach is better, allowing for the admins to have the certificates generated only if they feel like needing them. You could take a look at mod_ssl and dovecot. The first one includes the directories where Apache SSL certificates are to be stored, as well as the scripts needed to generate the certificates, but does not include the certificates themselves. In addition a set of sample certificates are generated at install time. A cleaner approach (in my opinion...) is used by dovecot which includes a dovecot-openssl.cnf file (in /etc/pki) and a mkcert.sh script (included as doc/sample).
Hello Manuel, I had a look at how dovecot did it, and that's definitely a better approach Added a mkcert.sh script and a README.fedora file with a short Getting Started text. New release: http://rubenkerkhof.com/packages/csync2.spec http://rubenkerkhof.com/packages/csync2-1.33-4.src.rpm
Excelent job. All tests pass as per the original review (comment #3), the package builds OK (as before) in mock for FC6/x86_64, the new sources are installed cleanly, all permissions are sane, changelog correctly reflects all changes. This package is APPROVED by me.
Thanks for the review Manuel.
*** This bug has been marked as a duplicate of bug 676187 ***