Spec URL: http://www.kobold.org/~wart/fedora/cyphesis.spec SRPM URL: http://www.kobold.org/~wart/fedora/cyphesis-0.5.8-1.src.rpm Description: Cyphesis is a WorldForge server suitable running small games. It is also designed by be used as an AI subsystem in a network of distributed servers. It includes a terrain engine based on the Mercator library, a persistence system based on PostgreSQL, and an AI engine using goal trees implemented in Python. It is the server used in most current WorldForge games. This package includes a SELinux policy module following the draft policy guidelines: http://fedoraproject.org/wiki/PackagingDrafts/SELinux/PolicyModules
Updated package that fixes a borked -init patch: http://www.kobold.org/~wart/fedora/cyphesis.spec http://www.kobold.org/~wart/fedora/cyphesis-0.5.8-2.src.rpm
Before I start the review, can I ask... %{_datadir}/%{name}/rulesets/*/*.py %{_datadir}/%{name}/rulesets/*/*.pyc %ghost %{_datadir}/%{name}/rulesets/*/*.pyo There are lots of these. Would it not be better for the package to just own those %{_datadir}/%{name}/rulesets - this would negate the need for the multiple datadirs Unless I've misunderstood things that is.
(In reply to comment #2) > Before I start the review, can I ask... > > %{_datadir}/%{name}/rulesets/*/*.py > %{_datadir}/%{name}/rulesets/*/*.pyc > %ghost %{_datadir}/%{name}/rulesets/*/*.pyo > > There are lots of these. Would it not be better for the package to just own > those %{_datadir}/%{name}/rulesets - this would negate the need for the multiple > datadirs > > Unless I've misunderstood things that is. Yes, that would make things simpler. But you might have missed that the *.pyo files are marked as %ghost. AFAIK, when you have nested directories with python files like this, you have to list each one individually in order to properly %ghost the .pyo files. But if you know of a shorthand to accomplish the same thing, I'd love to hear it. :)
Can you not just claim the directorys and then add a series of %ghost files? It would certainly make things easier on the eye...
(In reply to comment #4) > Can you not just claim the directorys and then add a series of %ghost files? It > would certainly make things easier on the eye... You learn something new every day. :) Yes, that does seem to work without producing any rpmlint warnings. Here's an updated package with a cleaned up %files section: http://www.kobold.org/~wart/fedora/cyphesis-0.5.8-3.src.rpm http://www.kobold.org/~wart/fedora/cyphesis.spec
Haven't tested, but doesn't that result in "file included twice" warnings from rpmbuild?
(In reply to comment #6) > Haven't tested, but doesn't that result in "file included twice" warnings from > rpmbuild? I thought it might, but it didn't when I ran it through mock.
rpm-4.4.2-15.2 [From test.spec] %files %{python_sitelib} %ghost %{python_sitelib}/foo2 [From rpmbuild -ba test.spec output] Processing files: test-1.0-1 warning: File listed twice: /usr/lib/python2.4/site-packages/foo2 Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 [What' actually in the file?] $ fedora-extract test-1.0-1.x86_64.rpm test-1.0-1/usr/lib/python2.4/site-packages $ So FC5's rpm is issuing a warning when it builds the package but it is creating the package with the file %ghost'ed.
Another approach to the python files issue is described at the bottom of this page: http://fedoraproject.org/wiki/Packaging/Python
I'm actually quite concerned about the number of File listed twice warnings given during the %build stage rpmlint warnings - srpm W: cyphesis macro-in-%changelog files building under mock - will report back them. In the meantime, please refer back to report #9 and rebuild with the changes recommended in there.
(In reply to comment #10) > rpmlint warnings - srpm > W: cyphesis macro-in-%changelog files My bad. I'll fix this in the next package. > In the meantime, please refer back to report #9 and rebuild with the changes > recommended in there. If I use the steps in comment #9 then I've essentially replaced a slightly messy looking %files section with an even messier scriptlet in %install. I'm not so sure that it makes the spec file any more readable. But I won't argue too much on this point. If you prefer how the spec looks with the scriptlet from comment #9, then I'll use it. I should have a new package ready in a few hours after a little more testing (assuming mock behaves this time).
New packages with the %install scriptlet for generating the python file list. This also replaces fedora-usermgmt with useradd -r, as a fix id is not necessary for this package: http://www.kobold.org/~wart/fedora/cyphesis-0.5.8-4.src.rpm http://www.kobold.org/~wart/fedora/cyphesis.spec FC6-i386 builds fail for me right now due to repo problems, so I've only been able to test building in mock on FC6-x86_64. I haven't been able to test the useradd bits in the package, though, as I don't have a FC6-x86_64 machine to install into.
(In reply to comment #11) > (In reply to comment #10) > > rpmlint warnings - srpm > > W: cyphesis macro-in-%changelog files > > My bad. I'll fix this in the next package. > > > In the meantime, please refer back to report #9 and rebuild with the changes > > recommended in there. > > If I use the steps in comment #9 then I've essentially replaced a slightly messy > looking %files section with an even messier scriptlet in %install. I'm not so > sure that it makes the spec file any more readable. But I won't argue too much > on this point. If you prefer how the spec looks with the scriptlet from comment > #9, then I'll use it. If you only have three or fewer directories containing .py* files then it doesn't really make sense to use the script-based approach as the files list is quite clear. The script approach really has benefits for bigger packages like moin and twisted, where there can be dozens of directories involved.
Builds fine in mock, but on installing, gives the error error: Failed dependencies: postgresql-server is required by cyphesis-0.5.8-4.i386 rpmlint is clean on the packages (uninstalled)
#14 - there should be a catch incase the user is not using selinux (I had it turned off for this test)
(In reply to comment #14) > Builds fine in mock, but on installing, gives the error > > error: Failed dependencies: > postgresql-server is required by cyphesis-0.5.8-4.i386 Well sure, because it requires postgresql-server to be installed, hence the line in the spec file: Requires: postgresql-server If you install this via yum then yum would pick up this dependency and install it for you. If you install this via rpm, then you'll have to install postgresql-server yourself.
(In reply to comment #15) > #14 - there should be a catch incase the user is not using selinux (I had it > turned off for this test) What's this about? Are there problems building or installing on a system with SElinux disabled? I don't have any such systems myself so I'd like to know, so that I can fix the SELinux policy module packaging guidelines document.
(In reply to comment #17) > (In reply to comment #15) > > #14 - there should be a catch incase the user is not using selinux (I had it > > turned off for this test) > > What's this about? Are there problems building or installing on a system with > SElinux disabled? > > I don't have any such systems myself so I'd like to know, so that I can fix the > SELinux policy module packaging guidelines document. I believe this comes from the stderr of the semanage commands that I use to define the port contexts for the application: /usr/sbin/semanage port -a -t %{name}_port_t -p tcp 6767 || : When the -selinux subpackage is installed on a system with selinux disabled, then semanage will spit out error messages of the sort: libsepol.context_from_record: MLS is enabled, but no MLS context found libsepol.context_from_record: could not create context structure libsepol.port_from_record: could not create port structure for range 6767:6767 (tcp) libsepol.sepol_port_modify: could not load port range 6767 - 6767 (tcp) libsemanage.dbase_policydb_modify: could not modify record value libsemanage.semanage_base_merge_components: could not merge local modifications into policy /usr/sbin/semanage: Could not add port tcp/6767 Redirecting the output of semanage to /dev/null should silence these warnings. The use of semanage isn't described in the selinux module guidelines, but perhaps it should be, with a note to redirect stderr.
#17 - the problem is installing with selinux disabled, but #16 has sorted this (though I'd have preferred a better system to catch it. For users not using selinux - for whatever reason - installing postgresql-server seems un-needed) One thing I can't quite see is if it is IPv6 enabled
(In reply to comment #19) > #17 - the problem is installing with selinux disabled, but #16 has sorted this > (though I'd have preferred a better system to catch it. For users not using > selinux - for whatever reason - installing postgresql-server seems un-needed) The need for postgresql-server is a separate issue from the semanage warnings. Regardless of whether selinux is enabled or not, postgresql-server is needed because the application stores persistent data in postgres. The semanage warnings are easily avoided by redirecting stderr from the semanage command. I'll have a new package shortly that fixes this. > One thing I can't quite see is if it is IPv6 enabled I'm not sure if it is or not. I don't recall seeing any documentation to that effect, but I wouldn't rule it out until someone has tried it.
(In reply to comment #18) > When the -selinux subpackage is installed on a system with selinux disabled, > then semanage will spit out error messages of the sort: > > libsepol.context_from_record: MLS is enabled, but no MLS context found > libsepol.context_from_record: could not create context structure > libsepol.port_from_record: could not create port structure for range 6767:6767 (tcp) > libsepol.sepol_port_modify: could not load port range 6767 - 6767 (tcp) > libsemanage.dbase_policydb_modify: could not modify record value > libsemanage.semanage_base_merge_components: could not merge local modifications > into policy > /usr/sbin/semanage: Could not add port tcp/6767 > > Redirecting the output of semanage to /dev/null should silence these warnings. > > The use of semanage isn't described in the selinux module guidelines, but > perhaps it should be, with a note to redirect stderr. Perhaps that sort of thing should be on the parent page (SELinux) rather than the SELinux/PolicyModules page since it's not really specific to use with modules. The parent page will need a fair bit of editing as much of its content is now in the PolicyModules page.
(In reply to comment #21) > (In reply to comment #18) > > When the -selinux subpackage is installed on a system with selinux disabled, > > then semanage will spit out error messages of the sort: > > > > libsepol.context_from_record: MLS is enabled, but no MLS context found > > libsepol.context_from_record: could not create context structure > > libsepol.port_from_record: could not create port structure for range 6767:6767 > (tcp) > > libsepol.sepol_port_modify: could not load port range 6767 - 6767 (tcp) > > libsemanage.dbase_policydb_modify: could not modify record value > > libsemanage.semanage_base_merge_components: could not merge local modifications > > into policy > > /usr/sbin/semanage: Could not add port tcp/6767 > > > > Redirecting the output of semanage to /dev/null should silence these warnings. > > > > The use of semanage isn't described in the selinux module guidelines, but > > perhaps it should be, with a note to redirect stderr. > > Perhaps that sort of thing should be on the parent page (SELinux) rather than > the SELinux/PolicyModules page since it's not really specific to use with > modules. The parent page will need a fair bit of editing as much of its content > is now in the PolicyModules page. Putting the use of semanage on the parent page is fine, but the PolicyModules page should probably include an example of its usage. However, using semanage in %post and %preun might not be the best place, as the port contexts won't be set if the admin starts with selinux turned off and later turns it on: (turn off selinux and reboot) # yum install cyphesis cyphesis-selinux (turn on selinux and reboot) # service cyphesis start (look in /var/log/messages: Aug 5 16:09:45 localhost kernel: audit(1154819384.688:23): avc: denied { name_bind } for pid=2420 comm="cyphesis" src=6767 scontext=user_u:system_r:cyphesis_t:s0 tcontext=system_u:object_r:port_t:s0 tclass=tcp_socket # semanage port -l | grep cyphesis (no match) Maybe semanage should be called to add/remove the port contexts in the init script instead? Or should semanage be able to set such contexts even if selinux is disabled?
#20 - from memory of the IRC chats, IPv6 has to be enabled for everything in FC6 (inc. FE). Could you check upstream to see if it's enabled?
(In reply to comment #23) > #20 - from memory of the IRC chats, IPv6 has to be enabled for everything in FC6 > (inc. FE). I don't think this was the resolution: http://fedoraproject.org/wiki/Extras/SteeringCommittee/Meeting-20060713 The meeting minutes for the following two FESCo meetings did not discuss this. > Could you check upstream to see if it's enabled? Will do, and I'll test it myself once I figure out how.
(In reply to comment #23) > #20 - from memory of the IRC chats, IPv6 has to be enabled for everything in FC6 > (inc. FE). > > Could you check upstream to see if it's enabled? The Packaging Committee made a decision that was vetoed by FESCo and FESCo hasn't clarified exactly what they would like yet. The potential plan was to enable it if it's an option. If not, add the package to a tracking bug where dwmw2 and his army of ipv6 ninjas could create a patch and also query upstream about adding support. I think the minimum time for something to come of this is a month with a good chance that it will be longer.
I still don't know if this supports IPV6. I'm having a heck of a time getting ipv6 running on my test system. This latest package removes all %ghost lines per the recent Packaging committee decision to not %ghost .pyo files. I know this hasn't been ratified by FESCo/FC groups yet, but I'm hopeful that it will be. This also cleans up some of the semanage issues reported earlier, so there should be no more error/warning messages when installing/uninstalling on a system that isn't running selinux. There should also not be any more problems turning selinux off and on and having the port context get set correctly. http://www.kobold.org/~wart/fedora/cyphesis-0.5.8-5.src.rpm http://www.kobold.org/~wart/fedora/cyphesis.spec
IPv6 should be supported via skstream which I also maintain. I tested using IPv6 with cyphesis at the time I did the development work on skstream. I have not had much chance to test since, but as I work at a site with native IPv6 on the whole network, it is likely my test machines use IPv6 by default.
Updated to latest upstream release: http://www.kobold.org/~wart/fedora/cyphesis-0.5.9-1.src.rpm http://www.kobold.org/~wart/fedora/cyphesis.spec
Okay, this builds happily in mock, rpmlint is clean and I'm happy with the IPv6 statement in #27 Review time... Comment... %package selinux Requires(post): /usr/sbin/ Should be %{_sbindir} really, it's not a blocker though as you've used /usr/sbin throughout. Needs work %files all the %{_bindir} files can be globbed %{_bindir}/cy* You don't have a %files devel (that I can see) yet you have the subpackage defined in the spec. Good Everything else! No dupes in the rpms build The software works Spec file complies with the packaging guidelines Permissions correctly set rpmlint clean mock builds fine (i386) Has fallback if selinux is not available/enabled md5sums correspond Consistent use of macros throughout Fix the needs work section and I'm happy.
(In reply to comment #29) > Needs work > > %files > > all the %{_bindir} files can be globbed > > %{_bindir}/cy* Done. > You don't have a %files devel (that I can see) yet you have the subpackage > defined in the spec. The -devel subpackage isn't needed. I've removed the devel subpackage declaration. http://www.kobold.org/~wart/fedora/cyphesis-0.5.9-2.src.rpm http://www.kobold.org/~wart/fedora/cyphesis.spec
Okay, as that was the only thing I objected to, I'm happy to approve it.
Imported and built. Thanks!