Bug 619928
Summary: | Review Request: tigase-server - Tigase XMPP Server in Java | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Matěj Cepl <mcepl> |
Component: | Package Review | Assignee: | Lubomir Rintel <lkundrak> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
The tigase-utils dependency does not seem to be available. (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). * 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 (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 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. (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 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 ;) New Package SCM Request ======================= Package Name: tigase-server Short Description: Tigase XMPP Server in Java Owners: mcepl Branches: f13 f14 el6 el5 InitialCC: Git done (by process-git-requests). [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]$ 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? (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]$ tigase-utils fixed by nirik. I am all set. Built on Rawhide and EL-6 (http://koji.fedoraproject.org/koji/packageinfo?packageID=10801). Other branches pending inclusion of tigase-utils into buildroot. |