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 619928 - Review Request: tigase-server - Tigase XMPP Server in Java
Summary: Review Request: tigase-server - Tigase XMPP Server in Java
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Lubomir Rintel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 583643 620168
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-07-30 21:56 UTC by Matěj Cepl
Modified: 2018-04-11 14:14 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-08-17 04:43:17 UTC
Type: ---
Embargoed:
lkundrak: fedora-review+


Attachments (Terms of Use)

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.


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