Bug 658420

*** Bug 457160 has been marked as a duplicate of this bug. ***

rpmlint output:

zorba.src: W: spelling-error %description -l en_US embeddable -> embedding, embedded, shreddable
zorba.src: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
zorba.src:217: E: files-attr-not-set
zorba.src:218: E: files-attr-not-set
zorba.src:219: E: files-attr-not-set
zorba.src: E: specfile-error sh: php-config: command not found
zorba.src: E: specfile-error error: Macro %php_extdir has empty body
zorba.x86_64: W: spelling-error %description -l en_US embeddable -> embedding, embedded, shreddable
zorba.x86_64: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged
zorba.x86_64: W: shared-lib-calls-exit /usr/lib64/libzorba_simplestore.so.1.4.0 exit.5
zorba.x86_64: W: no-manual-page-for-binary zorba
zorba-devel.x86_64: W: no-documentation
zorba-java.x86_64: W: no-documentation
zorba-php.x86_64: W: no-documentation
zorba-python.x86_64: W: no-documentation
zorba-ruby.x86_64: W: no-documentation
9 packages and 0 specfiles checked; 5 errors, 11 warnings.

- The -php package is missing %defattr, please add it.
- The specfile-errors are caused by me not having php-devel installed.

- You can drop BR: php-devel, since it requires php. 
- ruby-devel only requires ruby-libs, so if you need ruby then you do need to BR it explicitly.

***

Please move the rpm requires filtering part to the top of the specfile, whete the other definitions are.

***

The Java guidelines at http://fedoraproject.org/wiki/Packaging/Java state:

"Installation directory
All JAR files MUST go into %{_javadir} or a Java-version specific directory %{_javadir}-* as appropriate[1]. If the number of provided JAR files exceeds two, you MUST place them into a sub-directory named %{name}."

Is there any reason you're not complying to this in the -java package?

Also, please preserve the timestamp on build/swig/java/zorba.jar in %install.

***

You should be able to disable RPATH altogether within CMake. See
http://www.cmake.org/Wiki/CMake_RPATH_handling

***

Fix the line endings in %prep. And preserve the timestamp while doing it. For instance with

find . -name *.css > cssfiles
for file in `cat cssfiles`; do
 sed "s|\r||g" $file > $file.new && \
 touch -r $file $file.new && \
 mv $file.new $file
done

- I recommend that you make the java requirements explicit with
 BuildRequires: java-devel >= 1:1.6.0
and
 Requires: java >= 1:1.6.0
so that a proper Java compiler is used instead of gcj.

***

# add postfix -zorba to library name, and extend soname by %%{version}
Patch0:  zorba-json.patch

You *do* realize, that you won't be able to do version updates in any branch, since the soname will break every time...?

***

It's a bit funny that ASCII and XML files are placed in %{_libdir}/zorba/.
There are a few ELF files, too, though..

***

Review:

MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. ??
- src/compiler/parser contains GNU Bison files, that permit relicensing when used as a part of a bigger code.
- Majority of files is Apache 2.0 licensed.
- 3 headers are 3 clause BSD.

3 clause BSD states:
 *     * Redistributions in binary form must reproduce the above copyright
 *       notice, this list of conditions and the following disclaimer in the
 *       documentation and/or other materials provided with the distribution.

Since the headers are included in quite many files, I'd mark everything as "ASL 2.0 and BSD" just to be on the safe side..


MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
ee2130e52f9aa7b03b17a91acff33b83  zorba-1.4.0.tar.gz
ee2130e52f9aa7b03b17a91acff33b83  ../SOURCES/zorba-1.4.0.tar.gz

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- There is an "-O3" flag that appears in the build commands and overrides the "-O2" in %{optflags}. Please get rid of it.

MUST: Packages containing shared library files must call ldconfig. OK
MUST: A package must own all directories that it creates or require the package that owns the directory. OK

MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. NEEDSWORK
MUST: Large documentation files must go in a -doc subpackage. OK

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSWORK
- Add ChangeLog to %doc. 

MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK


You're probably not targetting EPEL, so these don't matter:
EPEL: Clean section exists. NEEDSWORK
EPEL: Buildroot cleaned before install. NEEDSWORK
EPEL: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A

Thanks for your thorough review, Jussi. I'll fix the issues in the next days. 


(In reply to comment #2)
> "Installation directory
> All JAR files MUST go into %{_javadir} or a Java-version specific directory
> %{_javadir}-* as appropriate[1]. If the number of provided JAR files exceeds
> two, you MUST place them into a sub-directory named %{name}."
> 
> Is there any reason you're not complying to this in the -java package?

I have to look into this again. It might be related to JNI stuff but I can't find the corresponding .so file at the moment. It's probably a leftover from a previous packaging attempt.


> You should be able to disable RPATH altogether within CMake. See
> http://www.cmake.org/Wiki/CMake_RPATH_handling

Yes, I did try that but it didn't work. The rpaths were still present. Maybe something went wrong. Will try it again.


(In reply to comment #3)
> You *do* realize, that you won't be able to do version updates in any branch,
> since the soname will break every time...?

Yes, I do and I'm not happy with it. I'll try to link this library statically as it seems to be a private one not intended for external usage.


> It's a bit funny that ASCII and XML files are placed in %{_libdir}/zorba/.
> There are a few ELF files, too, though..

Right, but as far as I see, there's no good alternative location to put both text and library files into.

Here are the updated files. I hope all issues have been resolved properly.

Spec URL: http://mgieseki.fedorapeople.org/zorba/zorba.spec
SRPM URL: http://mgieseki.fedorapeople.org/zorba/zorba-1.4.0-2.fc14.src.rpm

I wasn't able to remove the rpaths using the cmake flags only. There must be some stuff in the configuration files that prevents removing them automatically. So I still get rid of them using chrpath.

According to the guidelines [1], Java packages using JNI (and thus containing a .so file) must be installed in %{_libdir}/%{name}. Therefore, I haven't moved the jar yet. If this part of the guidelines doesn't apply here for some reason, please let me know.

[1] http://fedoraproject.org/wiki/Packaging/Java#Packaging_JAR_files_that_use_JNI

koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2747764

(In reply to comment #5)
> According to the guidelines [1], Java packages using JNI (and thus containing a
> .so file) must be installed in %{_libdir}/%{name}. Therefore, I haven't moved
> the jar yet. If this part of the guidelines doesn't apply here for some reason,
> please let me know.
> 
> [1]
> http://fedoraproject.org/wiki/Packaging/Java#Packaging_JAR_files_that_use_JNI

That's certainly true, so this is ok.

***

All the issues seem to have been fixed. However, before approving this review, I still have one final point.

The directory
 external/json
looks a bit troubling, since it is insinuating that a json library is bundled.

However, there are *NO* comments whatsoever what library this is and what its upstream is.

Please ask upstream where the files in external/json have been taken from, and if they have undergone modification. Please ask them also to document this in the relevant directory.

(In reply to comment #6)
> Please ask upstream where the files in external/json have been taken from, and
> if they have undergone modification. Please ask them also to document this in
> the relevant directory.

OK, done. I let you know if I get any information on the bundled json library.

I just got an informative reply from Matthias Brantner, a member of the Zorba team. The bundled json library is an old (public domain) version of jsonxx (https://github.com/hjiang/jsonxx). The latest sources are MIT-licensed. Due to several API changes, Zorba probably can't be built with them.

jsonxx doesn't seem to be intended for dynamic linkage as it lacks a soname definition. Actually, "make" only compiles the single C++ file to an object file. So if jsonxx must be packaged separately, it should probably go to a static library package.

Some additional information:

In 2008, the Zorba team got the permission from the author of jsonxx to bundle the sources with Zorba. They were in public domain at that time. Some months later, jsonxx had been revamped completely and licensed under MIT. The old version from 2008 required here is no longer available upstream. The git repo only contains the history of the new sources. 

In this case it shouldn't be a problem to use the bundled sources.

I guess you mean that it shouldn't be a problem to collect the information the packaging committee requires and request an exemption from the "no bundled libraries" policy.  https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

Yes, of course. ;)
Here's the request: https://fedorahosted.org/fpc/ticket/52

The jsonxx library has been granted a temporary bundling exception:
https://fedorahosted.org/fpc/ticket/52

Here are the updated files:
Spec URL: http://mgieseki.fedorapeople.org/zorba/zorba.spec
SRPM URL: http://mgieseki.fedorapeople.org/zorba/zorba-1.4.0-3.fc14.src.rpm

I also added a patch for Fedora > 14 that replaces calls of deprecated Boost functions.

Whoa, seems we all had forgotten this. As per comment #6, all the issues in the review were attended to, and the only thing blocking the review was the bundling exception that has been granted.

However, there has been a change of guidelines, and you MUST make the Requires architecture specific:
 Requires: %{name} = %{version}-%{release}%{?_isa}

Otherwise I believe there are no other issues, and thus this package must be

APPROVED


Please add the %{?_isa} stuff before import to git.

Not a problem at all. Thanks for the review and the approval, Jussi. 

As the bundling library exception for jsonxx has expired, and Zorba 2.0 isn't released yet, I have to backport the new json stuff from the svn sources. If that doesn't work properly for some reason, I'll try to build a pre-release version of Zorba 2.0. Depending on the amount of required changes, I'll post the new package here. I'm not sure if I'd need a re-review in this case. 

Anyway, I'll wait with the scm request until a working package is ready.

Zorba 2.0 has been released recently, and here's the corresponding package update: 

Spec URL: http://mgieseki.fedorapeople.org/review/zorba.spec
SRPM URL: http://mgieseki.fedorapeople.org/review/zorba-2.0.1-1.fc15.src.rpm

Most of the previously required tweaks are no longer needed. Many extensions, like the JSON module, have been removed from the core and are provided separately now. Thus, they should be packaged separately as well.

Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3325287

Go ahead and commit, a re-review is not necessary.

OK, great. Thanks again for the review.


New Package SCM Request
=======================
Package Name: zorba
Short Description: General purpose XQuery processor
Owners: mgieseki
Branches: f15 f16
InitialCC:

Git done (by process-git-requests).

zorba-2.0.2-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/zorba-2.0.2-1.fc16

zorba-2.0.2-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/zorba-2.0.2-1.fc15

zorba-2.0.2-1.fc15 has been pushed to the Fedora 15 testing repository.

zorba-2.0.2-1.fc15 has been pushed to the Fedora 15 stable repository.

zorba-2.0.2-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/zorba-2.0.2-2.fc16

zorba-2.0.2-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/zorba-2.0.2-2.fc16

zorba-2.0.2-2.fc16 has been pushed to the Fedora 16 stable repository.