Bug 753513 - Review Request: minetest - Multiplayer infinite-world block sandbox with survival mode
Summary: Review Request: minetest - Multiplayer infinite-world block sandbox with surv...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Gwyn Ciesla
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-13 02:18 UTC by Aleksandra Fedorova
Modified: 2011-12-18 19:48 UTC (History)
8 users (show)

Fixed In Version: minetest-0.3.1-6.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-12-18 19:46:41 UTC
Type: ---
Embargoed:
gwync: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Aleksandra Fedorova 2011-11-13 02:18:30 UTC
Spec URL: https://raw.github.com/RussianFedora/minetest/fedora/minetest.spec
SRPM URL: http://koji.russianfedora.ru/koji/getfile?taskID=15770&name=minetest-0.3.1-3.gitbc0e5c0.fc16.src.rpm
Description: 
Game of mining, crafting and building in the infinite world of cubic
blocks with optional hostile creatures, features both single and the
network multiplayer mode. There are no in-game sounds yet

* This is my first rpm package and I need a sponsor for it.

* This is the early release of the game, which means it has not so many features (for ex. no sound). Nevertheless all the functions implemented are stable, gameplay is enjoyable, game has low video requirements and other unique features (for ex. almost infinite world height). There are already several public servers available and gamers community grows very fast.

* I've already contacted the upstream developers team and I am planning to participate in the project from the side of translations, docs and other related topics.

As for the package itself:
* Currently there are no man-pages for both binaries, but they are planned for the next release.

* I've split the package into server part "minetest-server", which doesn't require the X-server to be installed, and the client part "minetest" which depends on minetest-server to be able to run single player game locally. So I've put all the docs and license into minetest-server package.

* The upstream server binary is called "minetestserver", while the package is called "minetest-server". So I've dropped suffix and used the word "minetest" for all the system config files.

* Server subpackage has systemd unit, rsyslog and logrotate configs. It seems that rsyslog needs to be restarted after the package installation, but I didn't find any guidelines, how to deal with log settings. Should I just add "systemctl restart rsyslog.service" to the post% section ?

* Also I am not sure about permissions of all the system config files created. Right now they are just defaults, please correct me if I am wrong.

Comment 1 Aleksandra Fedorova 2011-11-14 22:50:34 UTC
I've fixed the %clean section and %defattr according to guidelines.

New URLs:

Spec URL: https://raw.github.com/RussianFedora/minetest/fedora/minetest.spec
SRPM URL: http://koji.russianfedora.ru/koji/getfile?taskID=16122&name=minetest-0.3.1-4.gitbc0e5c0.fc16.src.rpm

$ rpmlint minetest
minetest.x86_64: W: spelling-error Summary(en_US) Multiplayer -> Multiplier, Multiplexer
minetest.x86_64: W: spelling-error %description -l en_US multiplayer -> multiplier, multiplexer
minetest.x86_64: W: no-documentation
minetest.x86_64: W: no-manual-page-for-binary minetest
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

$ rpmlint minetest-server
minetest-server.x86_64: W: spelling-error Summary(en_US) multiplayer -> multiplier, multiplexer
minetest-server.x86_64: W: spelling-error %description -l en_US multiplayer -> multiplier, multiplexer
minetest-server.x86_64: E: incoherent-logrotate-file /etc/logrotate.d/minetest
minetest-server.x86_64: W: non-standard-uid /var/lib/minetest minetest
minetest-server.x86_64: W: non-standard-gid /var/lib/minetest minetest
minetest-server.x86_64: W: no-manual-page-for-binary minetestserver
1 packages and 0 specfiles checked; 1 errors, 5 warnings.

$ rpmlint ~/rpmbuild/SRPMS/minetest-0.3.1-4.gitbc0e5c0.fc16.src.rpm 
minetest.src: W: spelling-error Summary(en_US) Multiplayer -> Multiplier, Multiplexer
minetest.src: W: spelling-error %description -l en_US multiplayer -> multiplier, multiplexer
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

Comment 2 Henrik Nordström 2011-11-29 01:50:04 UTC
Isn't "systemctl reload rsyslog.service" sufficient?

Comment 3 Aleksandra Fedorova 2011-11-29 02:02:06 UTC
> Isn't "systemctl reload rsyslog.service" sufficient?

It seems rsyslog can not be reloaded:

# systemctl reload rsyslog.service
Failed to issue method call: Job type reload is not applicable for unit rsyslog.service.

Comment 4 Gwyn Ciesla 2011-12-01 23:58:59 UTC
Starting. . .

Comment 5 Gwyn Ciesla 2011-12-02 00:43:29 UTC
rpmlint:  Some bogus spelling errors and:

minetest.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

Include the doc/ folder.

minetest.x86_64: W: no-manual-page-for-binary minetest
Each executable in standard binary directories should have a man page.

minetest-server.x86_64: E: incoherent-logrotate-file /etc/logrotate.d/minetest
Your logrotate file should be named /etc/logrotate.d/<package name>.

Can this not match?

minetest-server.x86_64: W: non-standard-uid /var/lib/minetest minetest
A file in this package is owned by a non standard user. Standard users are:
abrt, adm, amandabackup, apache, arpwatch, ats, avahi, avahi-autoipd, bacula,
beagleindex, bin, clamav, condor, cyrus, daemon, dbus, desktop, distcache,
dovecot, exim, fax, frontpage, ftp, games, gdm, glance, gopher, haldaemon,
halt, hsqldb, ident, jonas, ldap, lp, luci, mail, mailman, mailnull,
majordomo, mysql, named, netdump, news, nfsnobody, nobody, nocpulse, nova,
nscd, nslcd, ntp, nut, operator, oprofile, ovirt, pegasus, piranha, pkiuser,
polkituser, postfix, postgres, prelude-manager, privoxy, pulse, puppet, pvm,
qemu, quagga, radiusd, radvd, retrace, rhevagent, rhevm, ricci, root, rpc,
rpcuser, rpm, rtkit, sabayon, saned, shutdown, smmsp, snortd, squid, sshd,
stap-server, swift, sync, tcpdump, tomcat, tss, usbmuxd, uucp, vcsa, vdsm,
vhostmd, webalizer, wnn, xfs.

minetest-server.x86_64: W: non-standard-gid /var/lib/minetest minetest
A file in this package is owned by a non standard group. Standard groups are:
abrt, adm, apache, arpwatch, ats, audio, avahi, avahi-autoipd, bacula,
beagleindex, bin, cdrom, clamav, condor, console, daemon, dbus, desktop,
dialout, dip, disk, distcache, dovecot, exim, fax, floppy, frontpage, ftp,
games, gdm, glance, gopher, haldaemon, hsqldb, ident, jonas, kmem, kvm, ldap,
lock, lp, luci, mail, mailman, mailnull, majordomo, man, mem, mysql, named,
netdump, news, nfsnobody, nobody, nocpulse, nova, nscd, ntp, nut, oprofile,
ovirt, pegasus, piranha, pkiuser, polkituser, popusers, postdrop, postfix,
postgres, pppusers, prelude-manager, privoxy, pulse, puppet, pvm, qemu,
quagga, quaggavt, radiusd, radvd, realtime, retrace, rhevagent, rhevm, ricci,
root, rpc, rpcuser, rpm, rtkit, sabayon, saned, saslauth, screen, slipusers,
slocate, smmsp, snortd, squid, sshd, stap-server, swift, sys, tape, tcpdump,
tomcat, tss, tty, usbmuxd, users, utempter, utmp, uucp, vcsa, vhostmd, video,
wbpriv, webalizer, wheel, wine, wnn, xfs.

These two are probably OK.

minetest-server.x86_64: W: no-manual-page-for-binary minetestserver
Each executable in standard binary directories should have a man page.

- package meets naming guidelines
- package meets packaging guidelines

I'd rather the Source0 end in tar.gz, to more clearly indicate its type.  The celereon-minitest<foo> name from the download link is fine.

You can also drop the git checkout info from the version, since it's a released version, not a checkout.

- license ( GPLv2+ ) OK, text in %doc, matches source

Include licence file, which you will when you include doc/

- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 

Running a mock build to test BRs, though I think they're probably OK.

So at present, it looks like you just need to include docs, fix the tarball name, and think about the logrotate filename.  I'll post anything else I run across.

When you do one or two practice reviews, post links here.

Comment 6 Gwyn Ciesla 2011-12-02 01:45:25 UTC
Mock build was good.

However, on installation of minetest and minetest server, my X session restarted.

I was able to start the minetest service.

On running minetest, I got the connection screen, clicked connect, and it segfaulted.

gdb session:

(gdb) run
Starting program: /usr/bin/minetest 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
signal_handler_init()
Using system-wide paths (NOT RUN_IN_PLACE)
path_data = /usr/bin/../share/minetest
path_userdata = /home/limb/.minetest
Debug streams initialized, disable_stderr=0
19:44:28: ACTION[main]: minetest with SER_FMT_VER_HIGHEST=20, VER=0.3.1 RUN_IN_PLACE=0 USE_GETTEXT=1 INSTALL_PREFIX=/usr BUILD_TYPE=Release
INFO: Initial run of init_mapnode with g_texturesource=NULL. If this segfaults, there is a bug with something not checking for the NULL value.
Irrlicht Engine version 1.7.2
Linux 3.1.1-1.fc16.x86_64 #1 SMP Fri Nov 11 21:47:56 UTC 2011 x86_64
Creating X window...
Visual chosen: : 212
Using renderer: OpenGL 1.3
Mesa DRI R200 (RV280 5964)  TCL DRI2: Tungsten Graphics, Inc.
OpenGL driver version is 1.2 or better.
GLSL not available.
INFO: Full run of init_mapnode with g_texturesource!=NULL
Unsupported texture format
Loaded texture: /usr/share/minetest/mud.png
Loaded texture: /usr/share/minetest/menulogo.png
locale has been set to:en_US.UTF-8
locale has been set to:C
locale has been set to:en_US.UTF-8
locale has been set to:C
locale has been set to:en_US.UTF-8
locale has been set to:C
INFO: Full run of init_mapnode with g_texturesource!=NULL
[New Thread 0x7fffee985700 (LWP 16958)]
AuthManager: loading from /home/limb/.minetest/world/auth.txt
AuthManager: failed loading from /home/limb/.minetest/world/auth.txt
WARNING: AuthManager: creating /home/limb/.minetest/world/auth.txt
BanManager: loading from /home/limb/.minetest/world/ipban.txt
BanManager: failed loading from /home/limb/.minetest/world/ipban.txt
WARNING: BanManager: creating /home/limb/.minetest/world/ipban.txt
[New Thread 0x7fffee184700 (LWP 16959)]
10: Bind failed: Address already in use

In thread 7fffee985700:
/home/limb/rpmbuild/BUILD/celeron55-minetest-bc0e5c0/src/connection.cpp:577: virtual void* con::Connection::Thread(): Assertion '0' failed.
Debug stacks:
DEBUG STACK FOR THREAD 7fffee184700:
#0  virtual void* ServerThread::Thread()
#1  void Server::Receive()
(Leftover data: #2  void Server::SendBlocks(float))
DEBUG STACK FOR THREAD 7ffff721d780:
#0  int main(int, char**)
(Leftover data: #1  void Server::start(short unsigned int))
*** glibc detected *** /usr/bin/minetest: corrupted double-linked list: 0x0000000000fb2e90 ***

Program received signal SIGABRT, Aborted.
[Switching to Thread 0x7fffee985700 (LWP 16958)]
0x0000003444236285 in __GI_raise (sig=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
64	  return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
(gdb) bt full
#0  0x0000003444236285 in __GI_raise (sig=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
        resultvar = 0
        pid = <optimized out>
        selftid = 16958
#1  0x0000003444237b9b in __GI_abort () at abort.c:91
        save_stage = 2
        act = {__sigaction_handler = {sa_handler = 0x1, sa_sigaction = 0x1}, 
          sa_mask = {__val = {224482312683, 224481274952, 140737343990160, 
              140737196346464, 140737196346256, 0, 224481734499, 4294967295, 
              1, 10236148, 6020358, 8425376, 0, 6013936, 16464960, 0}}, 
          sa_flags = 1138812420, sa_restorer = 0x5}
        sigs = {__val = {32, 0 <repeats 15 times>}}
#2  0x00000000004a43e9 in assert_fail (assertion=0x5bdd06 "0", 
    file=0x5bc3f0 "/home/limb/rpmbuild/BUILD/celeron55-minetest-bc0e5c0/src/connection.cpp", line=577, 
    function=0x5bd000 "virtual void* con::Connection::Thread()")
    at /usr/src/debug/celeron55-minetest-bc0e5c0/src/debug.cpp:81
No locals.
#3  0x00000000004b5892 in con::Connection::Thread (this=0xfb3b88)
    at /usr/src/debug/celeron55-minetest-bc0e5c0/src/connection.cpp:577
        e = <optimized out>
        curtime = <optimized out>
---Type <return> to continue, or q <return> to quit---
        lasttime = <optimized out>
        __PRETTY_FUNCTION__ = "virtual void* con::Connection::Thread()"
#4  0x00007ffff76551b7 in JThread::TheThread (param=0xfb3b88)
    at pthread/jthread.cpp:166
        jthread = 0xfb3b88
        ret = <optimized out>
#5  0x0000003444607d90 in start_thread (arg=0x7fffee985700)
    at pthread_create.c:309
        __res = <optimized out>
        pd = 0x7fffee985700
        now = <optimized out>
        unwind_buf = {cancel_jmp_buf = {{jmp_buf = {1, 5491793290374575194, 
                140737488333728, 140737196349888, 0, 3, -5491829863683296166, 
                5502919522497492058}, mask_was_saved = 0}}, priv = {pad = {
              0x0, 0x0, 0x0, 0x0}, data = {prev = 0x0, cleanup = 0x0, 
              canceltype = 0}}}
        not_first_call = 0
        pagesize_m1 = <optimized out>
        sp = <optimized out>
        freesize = <optimized out>
        __PRETTY_FUNCTION__ = "start_thread"
#6  0x00000034442eed0d in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115
---Type <return> to continue, or q <return> to quit---
No locals.
(gdb)

Comment 7 Peter Lemenkov 2011-12-02 17:04:26 UTC
(In reply to comment #0)

> * Server subpackage has systemd unit, rsyslog and logrotate configs. It seems
> that rsyslog needs to be restarted after the package installation, but I didn't
> find any guidelines, how to deal with log settings. Should I just add
> "systemctl restart rsyslog.service" to the post% section ?

It mustn't be restarted automatically. That's a sysadmin's job to (re)start daemons especially such important ones as syslog. So please install config-file but don't try to do anything with running daemons.

Comment 8 Aleksandra Fedorova 2011-12-03 01:58:43 UTC
(In reply to comment #6)

There are two ways of doing things:
1) start the service, then run the client, set Server field to 'localhost' and connect. Then minetest will use the existing server.

2) stop the server, run the client only, leave the Server field blank, and Connect. Then client will run its own minetestserver instance on the same port 30000.

If you run both the server and the client, and then leave the Server field blank, then minetest tries to create its own instance. It fails and gives the following error:
=========================================
INFO: Full run of init_mapnode with g_texturesource!=NULL
AuthManager: loading from /home/bookwar/.minetest/world/auth.txt
BanManager: loading from /home/bookwar/.minetest/world/ipban.txt
6: Bind failed: Address already in use

In thread 7f8723576700:
/builddir/build/BUILD/celeron55-minetest-bc0e5c0/src/connection.cpp:577: virtual void* con::Connection::Thread(): Assertion '0' failed.
Debug stacks:
DEBUG STACK FOR THREAD 7f8722d75700:
#0  virtual void* ServerThread::Thread()
#1  void Server::Receive()
(Leftover data: #2  void Server::SendBlocks(float))
DEBUG STACK FOR THREAD 7f872e683740:
#0  int main(int, char**)
(Leftover data: #1  void Server::start(short unsigned int))
Aborted
=========================================

This is not the segfault, just normal error. Here the "6: Bind failed: Address already in use" is the main error string, and last is the debug output of the second thread.

So this is not really the internal bug, just interface design flaw, which is handled in a rather bad way though. I filed the corresponding issue here https://github.com/celeron55/minetest/issues/27

And I will add README.fedora file to the docs/ directory with detailed explanation of the server behavior.

> However, on installation of minetest and minetest server, my X session
> restarted.
>
> (...)
>
> On running minetest, I got the connection screen, clicked connect, and it
> segfaulted.
>
> (...)
>
> *** glibc detected *** /usr/bin/minetest: corrupted double-linked list:
> 0x0000000000fb2e90 ***
> 
> Program received signal SIGABRT, Aborted.
> [Switching to Thread 0x7fffee985700 (LWP 16958)]

Regarding these X-restarting and segfaults, minetest package can't be the reason for such errors. My guess is that this is the glibc issue. Check for example https://bugzilla.redhat.com/show_bug.cgi?id=754434 I stepped into this recently with glibc 2.14.90-16 and yum, and had to update glibc running yum in valgrind environment.

Comment 9 Aleksandra Fedorova 2011-12-03 02:12:44 UTC
(In reply to comment #5)

> Running a mock build to test BRs, though I think they're probably OK.
> 
> So at present, it looks like you just need to include docs, 

There is the docs directory in the minetest-server subpackage. Since minetest explicitly requires the minetest-server to be installed, according to http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing
 it seems for me that there is no need to add one more doc directory for it. Am I wrong?

> fix the tarball name, and think about the logrotate filename.

I will post the updated spec tomorrow.
 
> When you do one or two practice reviews, post links here.

Ok.

Comment 10 Aleksandra Fedorova 2011-12-05 13:51:55 UTC
New URLs:

Spec URL: https://raw.github.com/RussianFedora/minetest/fedora/minetest.spec
SRPM URL:
http://koji.russianfedora.ru/koji/getfile?taskID=17323&name=minetest-0.3.1-5.fc16.src.rpm

I've changed the tarball and logrotate names, removed git commit tag and added
README.fedora file.

docs directory is still in the minetest-server package only. I can change this
if needed.

Comment 11 Gwyn Ciesla 2011-12-06 14:32:12 UTC
The docs need to be in the %docs section, not simply included in the package.  This will put the docs in the standard location.

It runs for me now, I think the problem might be graphics related, my brother has some OpenGL code that runs fine on an F-16 VM but segfaults horribly on my main machine. :)

This is shaping up.  Any practice reviews yet?

Comment 13 Gwyn Ciesla 2011-12-07 16:50:13 UTC
Good. :)

Post an updated SRPM/SPEC fixing %doc, and let me know your FAS account name.

Comment 14 Aleksandra Fedorova 2011-12-08 23:05:58 UTC
Ok, here are new URLs with fixed %docs:

Spec URL: https://raw.github.com/RussianFedora/minetest/fedora/minetest.spec
SRPM URL: http://koji.russianfedora.ru/koji/getfile?taskID=17871&name=minetest-0.3.1-6.fc16.src.rpm

And my FAS account is "bookwar"

Comment 15 Gwyn Ciesla 2011-12-08 23:43:41 UTC
You can actually just put doc/ in the %docs section rather than individually listing files, but that's up to you.

APPROVED, and I've sponsored you.  Feel free to contact me with questions, etc.  You can now also go back and do official reviews on the packages you started on, if you're so inclined.  I'm sure the submitters would appreciate it. :)

Comment 16 Aleksandra Fedorova 2011-12-09 00:32:13 UTC
Thanks, Jon!

New Package SCM Request
=======================
Package Name: minetest
Short Description: Multiplayer infinite-world block sandbox with survival mode
Owners: bookwar
Branches: f15 f16
InitialCC:

Comment 17 Gwyn Ciesla 2011-12-09 01:28:34 UTC
Git done (by process-git-requests).

Comment 18 Fedora Update System 2011-12-09 18:31:12 UTC
minetest-0.3.1-6.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/minetest-0.3.1-6.fc15

Comment 19 Fedora Update System 2011-12-09 18:31:22 UTC
minetest-0.3.1-6.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/minetest-0.3.1-6.fc16

Comment 20 Fedora Update System 2011-12-10 19:30:45 UTC
minetest-0.3.1-6.fc15 has been pushed to the Fedora 15 testing repository.

Comment 21 Fedora Update System 2011-12-18 19:46:41 UTC
minetest-0.3.1-6.fc15 has been pushed to the Fedora 15 stable repository.

Comment 22 Fedora Update System 2011-12-18 19:48:45 UTC
minetest-0.3.1-6.fc16 has been pushed to the Fedora 16 stable repository.


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