Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 200976 - Review Request: cyphesis - WorldForge game server
Summary: Review Request: cyphesis - WorldForge game server
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Paul F. Johnson
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-08-01 22:31 UTC by Wart
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-08-31 16:06:10 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Wart 2006-08-01 22:31:11 UTC
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

Comment 1 Wart 2006-08-01 22:47:33 UTC
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

Comment 2 Paul F. Johnson 2006-08-01 22:51:16 UTC
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.

Comment 3 Wart 2006-08-01 22:53:52 UTC
(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.  :)

Comment 4 Paul F. Johnson 2006-08-01 23:00:55 UTC
Can you not just claim the directorys and then add a series of %ghost files? It
would certainly make things easier on the eye...

Comment 5 Wart 2006-08-01 23:16:58 UTC
(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

Comment 6 Ville Skyttä 2006-08-02 06:31:15 UTC
Haven't tested, but doesn't that result in "file included twice" warnings from
rpmbuild?

Comment 7 Wart 2006-08-02 14:07:19 UTC
(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.

Comment 8 Toshio Kuratomi 2006-08-02 15:36:22 UTC
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.

Comment 9 Paul Howarth 2006-08-03 10:09:30 UTC
Another approach to the python files issue is described at the bottom of this page:

http://fedoraproject.org/wiki/Packaging/Python


Comment 10 Paul F. Johnson 2006-08-03 23:31:22 UTC
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.

Comment 11 Wart 2006-08-03 23:53:57 UTC
(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).

Comment 12 Wart 2006-08-04 00:20:15 UTC
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.

Comment 13 Paul Howarth 2006-08-04 10:26:54 UTC
(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.

Comment 14 Paul F. Johnson 2006-08-05 22:01:52 UTC
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)

Comment 15 Paul F. Johnson 2006-08-05 22:03:23 UTC
#14 - there should be a catch incase the user is not using selinux (I had it
turned off for this test)

Comment 16 Wart 2006-08-05 22:06:07 UTC
(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.

Comment 17 Paul Howarth 2006-08-05 22:10:21 UTC
(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.

Comment 18 Wart 2006-08-05 22:20:00 UTC
(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.

Comment 19 Paul F. Johnson 2006-08-05 22:23:30 UTC
#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

Comment 20 Wart 2006-08-05 22:32:16 UTC
(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.

Comment 21 Paul Howarth 2006-08-05 22:39:22 UTC
(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.

Comment 22 Wart 2006-08-05 23:02:37 UTC
(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?

Comment 23 Paul F. Johnson 2006-08-05 23:16:55 UTC
#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?

Comment 24 Wart 2006-08-05 23:57:28 UTC
(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.

Comment 25 Toshio Kuratomi 2006-08-06 05:35:39 UTC
(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.

Comment 26 Wart 2006-08-11 22:32:52 UTC
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

Comment 27 Alistair Riddoch 2006-08-24 15:25:35 UTC
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.

Comment 29 Paul F. Johnson 2006-08-28 20:39:57 UTC
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.

Comment 30 Wart 2006-08-28 22:26:10 UTC
(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


Comment 31 Paul F. Johnson 2006-08-30 22:49:09 UTC
Okay, as that was the only thing I objected to, I'm happy to approve it.

Comment 32 Wart 2006-08-31 16:06:10 UTC
Imported and built.  Thanks!


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