Bug 619928

Summary: Review Request: tigase-server - Tigase XMPP Server in Java
Product: [Fedora] Fedora Reporter: Matěj Cepl <mcepl>
Component: Package ReviewAssignee: Lubomir Rintel <lkundrak>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, lkundrak, mcepl, notting
Target Milestone: ---Flags: lkundrak: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-08-17 04:43:17 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 583643, 620168    
Bug Blocks:    

Description Matěj Cepl 2010-07-30 21:56:45 UTC
Spec URL: http://mcepl.fedorapeople.org/rpms/tigase-server.spec
SRPM URL: http://mcepl.fedorapeople.org/rpms/tigase-server-5.0.0-0.1.20100527svn.src.rpm
Description:
Tigase is pure Java 1.6.0 XMPP high-performance server based on highly
modular design.

Comment 1 Lubomir Rintel 2010-07-31 17:08:54 UTC
The tigase-utils dependency does not seem to be available.

Comment 2 Matěj Cepl 2010-08-01 12:14:35 UTC
(In reply to comment #1)
> The tigase-utils dependency does not seem to be available.    

Sorry, it is now in a separate package review (bug 620168).

Comment 3 Lubomir Rintel 2010-08-02 07:55:09 UTC
* Package correctly named
* License OK, allowed in Fedora, full text included
* Versioned accordance with guidelines
* VCS snapshot provided, revision number ok, correctly commented
* BuildRequires seem complete
** Mock build not attempted since I'm too lazy to inject tigase-utils =]
** Local rebuild succeeded
* Prebuilt stuff dropped
* Spec file clean and legible, American English used
* Filelists sane
* Requires/Provides ok

1.) RPMlint

I find all of these worth addressing:

tigase-server.noarch: E: zero-length /etc/tigase/database/derby-create-db.sql
tigase-server.noarch: E: incoherent-logrotate-file /etc/logrotate.d/tigase
Your logrotate file should be named /etc/logrotate.d/<package name>.

tigase-server.noarch: W: no-reload-entry /etc/rc.d/init.d/tigase
In your init script (/etc/rc.d/init.d/your_file), you don't have a 'reload'
entry, which is necessary for good functionality.

2.) Why is this needed?

export LANG=en_US

Please comment if it is necessary. Rpmbuilds sets lang to C already:

$ rpm --eval %___build_pre |grep LANG
  LANG=C
  export LANG

This is pending import of tigase-utils and resolution of rpmlint warnings

Comment 4 Matěj Cepl 2010-08-03 19:05:44 UTC
(In reply to comment #3)
> I find all of these worth addressing:
> 
> tigase-server.noarch: E: zero-length /etc/tigase/database/derby-create-db.sql

This is from the upstream tarball. I didn't think it harms anybody, but yes, certainly I can remove all zero-length files in database/.

> tigase-server.noarch: E: incoherent-logrotate-file /etc/logrotate.d/tigase
> Your logrotate file should be named /etc/logrotate.d/<package name>.
> 
> tigase-server.noarch: W: no-reload-entry /etc/rc.d/init.d/tigase
> In your init script (/etc/rc.d/init.d/your_file), you don't have a 'reload'
> entry, which is necessary for good functionality.

This is also caused by upstream. Their original idea was to have project tigase providing server, client, and webclient for XMPP. Currently only tigase-server exists, but we have two upstream packages which are not used by anything else (tigase-xmltools, tigase-utils) and package with too long name tigase-server. 

Given that the "tigase" is commonly used name of this software, I tried to use tigase instead of tigase-server, whenever I could. "tigase-server" seems like an awkward name for the binary.

> 2.) Why is this needed?
> 
> export LANG=en_US

Yes, it isn't. Fixed.

> This is pending import of tigase-utils and resolution of rpmlint warnings    

New src.rpm at http://mcepl.fedorapeople.org/rpms/tigase-server-5.0.0-0.2.20100527svn.src.rpm, new .spec file at the same location with new content

Comment 5 Lubomir Rintel 2010-08-04 05:26:20 UTC
Thanks. A couple more things:

3.) # FIXME Originálně obsahuje groovy-all-1.5.7.jar, groovy-engine.jar
# groovy se zeptat lkundraka

In current version of package you may safely drop them as they are. 

Alternatively, you may choose to package the tools from src/main/groovy if you find them useful. If you choose to do so, you can decide whether to build them into .jars at package build time (preferred) and add groovy BR, or leave them as they are (the source form) and drag groovy interpreter as binary package dependency.

In any case, please replace "rm -v libs/{derby,jdbc}*.jar" with a statement that would drop all jars; and do not install them in %install.

4.) This is not a wise thing to do: "ant -Dlibs=%{_javadir}"

Javadir can grow too big and apart from java being slow, you easily loose track of jars you depend on. A better solution would be to drop the -Dlibs argument and prepend the call to ant with something like:

build-jar-repository libs tigase-utils tigase-xmltools

5.) Please comment on why are you missing the %check section with ant run-unittest command. (It seems to me that it is missing dependency on wttols which provides the unitgen task, right?)

6.) The startup script does not seem to work

bash-4.1# /usr/share/tigase/scripts/tigase.sh 
No params-file.conf given. Using: '/etc/tigase/tigase.conf'
/usr/bin/build-classpath: error: Could not find groovy-all-1.5.7 Java extension for this JVM
/usr/bin/build-classpath: error: Could not find groovy-engine Java extension for this JVM
/usr/bin/build-classpath: error: Some specified jars were not found
JAVA_HOME is not set.
Please set it to correct value before starting the sever.

JAVA_HOME makes little sense in jpackage environment. Please drop the check, or hardcode the value to /usr/lib/jvm.

7.) /etc/tigase/tigase.conf contains both user modifiable and non-user-modifiable settings.

Therefore, in case the user e.g. tunes the VM for higher heap size, and another version of the package adds new jar to the default classpath, an update results in non-functional configuration. Please consider splitting the file into two.

8.) Why does /etc/tigase/database go into /etc?

Seems to me like like it's not configuration, but data; and thus should go into /usr/share.

Hopefully that's all.

Comment 6 Matěj Cepl 2010-08-07 04:26:36 UTC
(In reply to comment #5)
> In current version of package you may safely drop them as they are. 

I did. There is now in %prep

# to make delete verbose don't use -delete
find . -name \*.jar -exec rm -v '{}' \;

and jars are not installed.

> Alternatively, you may choose to package the tools from src/main/groovy if you
> find them useful. If you choose to do so, you can decide whether to build them
> into .jars at package build time (preferred) and add groovy BR, or leave them
> as they are (the source form) and drag groovy interpreter as binary package
> dependency.

The problem I have is that I would like to have tigase-server working on EL5 and there doesn't seem to be groovy on neither EL6 nor EL5 (actually to be sure it works on EL5, I am doing all packaging work on EL5 virtual machine). For now, I have installed groovy scripts into %doc and if anybody wants to have groovy scripts he must to hack groovy support himself. I hope that could be an acceptable solution.

> 4.) This is not a wise thing to do: "ant -Dlibs=%{_javadir}"
> 
> Javadir can grow too big and apart from java being slow, you easily loose track
> of jars you depend on. A better solution would be to drop the -Dlibs argument
> and prepend the call to ant with something like:
> 
> build-jar-repository libs tigase-utils tigase-xmltools

Wov! That's a great tool ... I haven't knew about it. Fixed.

> 5.) Please comment on why are you missing the %check section with ant
> run-unittest command. (It seems to me that it is missing dependency on wttols
> which provides the unitgen task, right?)

Yes, and I've made a comment to the %check section

> 6.) The startup script does not seem to work
> 
> JAVA_HOME makes little sense in jpackage environment.
> Please drop the check, or hardcode the value to /usr/lib/jvm.

I don't think it has anything to do with JAVA_HOME (which is not set) but with asking for groovy* jars. I had them installed from my previous experiments, but they are not packaged anymore. Fixed.

> 7.) /etc/tigase/tigase.conf contains both user modifiable and
> non-user-modifiable settings.
> 
> Therefore, in case the user e.g. tunes the VM for higher heap size, and
> another version of the package adds new jar to the default classpath, an
> update results in non-functional configuration. Please consider splitting
> the file into two.

Created /etc/tigase/tigase.defaults which is sourced by /etc/tigase/tigase.conf, but it shouldn't be changed by the sysadmin

> 8.) Why does /etc/tigase/database go into /etc?

You are right. I was on the fence about switching it back to /usr/share/tigase/database, which I did now.

> Hopefully that's all.    

/me hopes too :)

SPEC file is http://mcepl.fedorapeople.org/rpms/tigase-server.spec
src.rpm is http://mcepl.fedorapeople.org/rpms/tigase-server-5.0.0-0.3.20100527svn.src.rpm

Comment 7 Lubomir Rintel 2010-08-11 07:25:16 UTC
Hm, I remember replying to this, but don't see my response. It was while I was stuck on prague railway station before my battery died, so there probably was a mid-air collision or whatever I did not notice...

I'm happy with the package now, it is

APPROVED

(In reply to comment #6)
> # to make delete verbose don't use -delete
> find . -name \*.jar -exec rm -v '{}' \;

find -name \*.jar -print -delete

;)

Comment 8 Matěj Cepl 2010-08-13 04:21:53 UTC
New Package SCM Request
=======================
Package Name: tigase-server
Short Description: Tigase XMPP Server in Java
Owners: mcepl
Branches: f13 f14 el6 el5
InitialCC:

Comment 9 Jason Tibbitts 2010-08-13 16:10:51 UTC
Git done (by process-git-requests).

Comment 10 Matěj Cepl 2010-08-15 01:48:01 UTC
[matej@tikanga Extras]$ fedpkg clone tigase-server
Initialized empty Git repository in /home/matej/build/Extras/tigase-server/.git/
fatal: '/srv/git/rpms//tigase-server.git' does not appear to be a git repository
fatal: The remote end hung up unexpectedly
fetch-pack from 'ssh://mcepl.org/tigase-server' failed.
Could not clone: Command '['git', 'clone', 'ssh://mcepl.org/tigase-server']' returned non-zero exit status 1
[matej@tikanga Extras]$

Comment 11 Kevin Fenzi 2010-08-16 00:16:56 UTC
Odd, works fine here: 

% fedpkg clone tigase-server
Cloning into tigase-server...
remote: Counting objects: 3, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (3/3), done.

Still not working for you? what version of fedora-packager?

Comment 12 Matěj Cepl 2010-08-16 14:33:16 UTC
(In reply to comment #11)
> Odd, works fine here: 
> 
> % fedpkg clone tigase-server
> Cloning into tigase-server...
> remote: Counting objects: 3, done.
> remote: Compressing objects: 100% (2/2), done.
> remote: Total 3 (delta 0), reused 0 (delta 0)
> Receiving objects: 100% (3/3), done.
> 
> Still not working for you? what version of fedora-packager?

tigase-server got fixed, but tigase-utils has still missing branches

[matej@tikanga Extras]$ fedpkg clone tigase-utils
Initialized empty Git repository in /home/matej/build/Extras/tigase-utils/.git/
remote: Counting objects: 8, done.
remote: Compressing objects: 100% (6/6), done.
remote: Total 8 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (8/8), done.
[matej@tikanga Extras]$ cd tigase-utils/
[matej@tikanga tigase-utils]$ git branch -a
* master
  origin/HEAD
  origin/f14
  origin/master
[matej@tikanga tigase-utils]$

Comment 13 Matěj Cepl 2010-08-16 15:52:42 UTC
tigase-utils fixed by nirik. I am all set.

Comment 14 Matěj Cepl 2010-08-17 04:43:17 UTC
Built on Rawhide and EL-6 (http://koji.fedoraproject.org/koji/packageinfo?packageID=10801). Other branches pending inclusion of tigase-utils into buildroot.