Bug 539983 - Review Request: qjson - qt-based library that maps JSON data to QVariant object
Review Request: qjson - qt-based library that maps JSON data to QVariant object
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
: 544737 (view as bug list)
Depends On:
Blocks: 544739
  Show dependency treegraph
 
Reported: 2009-11-21 15:36 EST by Eli Wapniarski
Modified: 2010-09-24 16:44 EDT (History)
5 users (show)

See Also:
Fixed In Version: qjson-0.7.1-2.fc13
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-27 15:26:29 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Eli Wapniarski 2009-11-21 15:36:44 EST
Spec URL: http://orbsky.homelinux.org/packages/qjson.spec
SRPM URL: http://orbsky.homelinux.org/packages/qjson-0.6.3-1.fc11.src.rpm
Description: JSON is a lightweight data-interchange format. It can represents integer, real number, string, an ordered sequence of value, and a collection of name/value pairs.QJson is a qt-based library that maps JSON data to QVariant objects.
Comment 1 Orcan Ogetbil 2009-11-21 16:16:52 EST
Here is my review:

! All relevant doc files should be packaged.
   - I don't think we need the INSTALL file in %doc
   - There is doxygen documentation in the doc directory that can be built and packaged.

* rpmlint says:
   qjson.x86_64: W: name-repeated-in-summary QJson
   qjson.x86_64: W: invalid-license LGPL
   qjson.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libqjson.so
   qjson.x86_64: W: devel-file-in-non-devel-package /usr/include/qjson/serializer.h
   qjson.x86_64: W: devel-file-in-non-devel-package /usr/include/qjson/parser.h
   qjson.x86_64: W: devel-file-in-non-devel-package /usr/include/qjson/qjson_export.h
   qjson.x86_64: W: devel-file-in-non-devel-package /usr/include/qjson/serializerrunnable.h
   qjson.x86_64: W: devel-file-in-non-devel-package /usr/lib64/pkgconfig/QJson.pc
   qjson.x86_64: E: library-without-ldconfig-postin /usr/lib64/libqjson.so.0.6.3
   qjson.x86_64: E: library-without-ldconfig-postun /usr/lib64/libqjson.so.0.6.3
   qjson.x86_64: W: devel-file-in-non-devel-package /usr/include/qjson/parserrunnable.h
   qjson-debuginfo.x86_64: W: invalid-license LGPL
   qjson.src: W: name-repeated-in-summary QJson
   qjson.src: W: invalid-license LGPL

   -The header files, .pc file and the *.so file should go to the devel package. Also the doxygen doc, if built, should go to the devel package. The devel package must require pkgconfig in addition to %{name} = %{version}-%{release}
   -Summary can simply be "qt-based library that maps JSON data to QVariant objects"
   -There is GPLv2+ code in the src/ directory, making the effective license GPLv2+. Please verify this, possibly via upstream, as they claim that the library is LGPL.

! The tests in the tests/ directory can be run in %check

! If you can, please make the description more descriptive. Not just a copy of the summary.

* Every binary RPM package which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.

? Do we need to keep those commented out lines in the specfile?

* %{_includedir}/qjson/ is not owned.

* What package owns the directory?
   %{_datadir}/apps/cmake/modules/
That package needs to be in Requires.
Comment 2 Eli Wapniarski 2009-11-22 02:19:00 EST
OK... Its gonna be obvious now that I'm no developer. I've managed to address some of the issues as detailed below.

- Split off development libraries to its own package
- Modified licensing in spec file to reflect GPL2 code though docs state that
  qjson licensed under LPGL
- Uncommeted and corrected sed line in this spec file


I'm open to any suggestions and comments you may have regarding the following issues:

- I have no idea how to run the tests.
- How do I compile the doxygen doc? Can that be done during rpmbuild?
- Since this is a development package, it would probably be best if this
  was a multilib package. How do I accomplish that?
- I thought the %defattr macro conferred ownership to files and directories
  being installed. Please correct me.
- Where can I find an example to properly add FindQJSON.cmake as a requirment?
  Or better yet and quicker if you could detail the correct line.

Looking very forward to your feedback and thankyou very much for your help


Spec URL: http://orbsky.homelinux.org/packages/qjson.spec
SRPM URL: http://orbsky.homelinux.org/packages/qjson-0.6.3-2.fc11.src.rpm
Comment 3 Eli Wapniarski 2009-11-22 02:49:15 EST
Oh... One other thing. I think that the "test" is not neccessary as it isn't mentioned at all in the INSTALL doc.
Comment 4 Orcan Ogetbil 2009-11-26 01:31:40 EST
(In reply to comment #2)
> OK... Its gonna be obvious now that I'm no developer. I've managed to address
> some of the issues as detailed below.

It's all okay. Nobody is born as a programmer. I will try to be as explicit as possible.

> 
> - Split off development libraries to its own package
> - Modified licensing in spec file to reflect GPL2 code though docs state that
>   qjson licensed under LPGL
> - Uncommeted and corrected sed line in this spec file
> 
> 
> I'm open to any suggestions and comments you may have regarding the following
> issues:
> 
> - I have no idea how to run the tests.

When you are running "%{cmake} ..", you can add the flag "-DQJSON_BUILD_TESTS=1" so the tests are built. i.e.
   %{cmake} .. -DQJSON_BUILD_TESTS=1

Then in the %check section you will execute these tests. Something like
   %check
   LD_PRELOAD=%{_target_platform}/%{_lib}/libqjson.so %{_target_platform}/tests/testparser
   LD_PRELOAD=%{_target_platform}/%{_lib}/libqjson.so %{_target_platform}/tests/testserializer

If you don't use LD_PRELOAD, the tests will fail because it can't access the library you just built.


> - How do I compile the doxygen doc? Can that be done during rpmbuild?

Add BR: doxygen. Then in the doc/ directory, just run
   doxygen
It will create an html/ direcory inside doc/ add this directory to the %doc of the devel subpackage.


> - Since this is a development package, it would probably be best if this
>   was a multilib package. How do I accomplish that?

You don't need to do anything about multilib. Releng takes care of that.

> - I thought the %defattr macro conferred ownership to files and directories
>   being installed. Please correct me.

Think of it this way: All files and directories are some entries in the filesystem. Directories are special kind of "files". Now each RPM must either own the directory it puts a file inside, or it must require a package that owns a directory. This package puts a file inside %{_includedir}/qjson/ but this directory is not owned by any package! You must own this directory with the devel subpackage. So replace the line
   %{_includedir}/qjson/*.h
with either
   %{_includedir}/qjson/
or
   %dir %{_includedir}/qjson
   %{_includedir}/qjson/*.h
so that the directory is owned. I hope you got the idea. This must be satisfied by any package in Fedora.

> - Where can I find an example to properly add FindQJSON.cmake as a requirment?
>   Or better yet and quicker if you could detail the correct line.
> 

I don't know exactly. You might want to do some research. However, I see that cmake package put a lot of modules into /usr/share/cmake/Modules/ so you might want to move that file into that directory in %install. Also I believe that this file must go to the devel package. Note that you will need to require cmake (for the devel package) for directory ownership.

But as I said, I don't know exactly. Please check this.

> Looking very forward to your feedback and thankyou very much for your help
> 
> 
> Spec URL: http://orbsky.homelinux.org/packages/qjson.spec
> SRPM URL: http://orbsky.homelinux.org/packages/qjson-0.6.3-2.fc11.src.rpm  

You are welcome. One more thing (at least) need to be fixed. In Fedora the files

   %{_libdir}/libfoo.so.*
go to the main library package, whereas the single file, which is a symlink
   %{_libdir}/libfoo.so
goes to the devel package. Please fix this in your package accordingly.
Comment 5 Eli Wapniarski 2009-12-04 03:17:05 EST
Sorry about the delay in getting this looked after. I have had an extremely distracting couple of weeks.


(In reply to comment #4)
> (In reply to comment #2)
> When you are running "%{cmake} ..", you can add the flag
> "-DQJSON_BUILD_TESTS=1" so the tests are built. i.e.
>    %{cmake} .. -DQJSON_BUILD_TESTS=1
> 
> Then in the %check section you will execute these tests. Something like
>    %check
>    LD_PRELOAD=%{_target_platform}/%{_lib}/libqjson.so
> %{_target_platform}/tests/testparser
>    LD_PRELOAD=%{_target_platform}/%{_lib}/libqjson.so
> %{_target_platform}/tests/testserializer


Done


> Add BR: doxygen. Then in the doc/ directory, just run
>    doxygen
> It will create an html/ direcory inside doc/ add this directory to the %doc of
> the devel subpackage.


Done.

This should probably go in the base package shouldn't it? Its documentation not header files and I would assume that the docs contains how to use.


> a directory. This package puts a file inside %{_includedir}/qjson/ but this
> directory is not owned by any package! You must own this directory with the
> devel subpackage. So replace the line
>    %{_includedir}/qjson/*.h
> with either
>    %{_includedir}/qjson/
> or
>    %dir %{_includedir}/qjson
>    %{_includedir}/qjson/*.h
> so that the directory is owned. I hope you got the idea. This must be satisfied
> by any package in Fedora.

Done

> > - Where can I find an example to properly add FindQJSON.cmake as a requirment?
> >   Or better yet and quicker if you could detail the correct line.
> > 
> 
> I don't know exactly. You might want to do some research. However, I see that
> cmake package put a lot of modules into /usr/share/cmake/Modules/ so you might
> want to move that file into that directory in %install. Also I believe that
> this file must go to the devel package. Note that you will need to require
> cmake (for the devel package) for directory ownership.
> 

Cmake was added as Requirment in a the devel package as well and the module was moved over to the devel package. I don't believe that it should be included as a requirment because if it is required and it isn't yet built and installed then I would not be able to build the package. Please correct me if I'm wrong.

>    %{_libdir}/libfoo.so.*
> go to the main library package, whereas the single file, which is a symlink
>    %{_libdir}/libfoo.so
> goes to the devel package. Please fix this in your package accordingly.  

Done
Comment 6 Eli Wapniarski 2009-12-04 03:19:03 EST
Ooops... Forgot to add urls to the new spec and srpm

Spec URL: http://orbsky.homelinux.org/packages/qjson.spec
SRPM URL: http://orbsky.homelinux.org/packages/qjson-0.6.3-3.fc11.src.rpm
Comment 7 Eli Wapniarski 2009-12-04 03:46:27 EST
(In reply to comment #5)
> In Fedora the files
> 
>    %{_libdir}/libfoo.so.*
> go to the main library package, whereas the single file, which is a symlink
>    %{_libdir}/libfoo.so
> goes to the devel package. Please fix this in your package accordingly.  

Oh. I think this must be an error because, if an app should make reference to the symlink and the development package is not installed it won't find the libarary. So I left all the libraries including he mentioned symlink in the base package. Please correct me if I'm wrong about that.
Comment 8 Orcan Ogetbil 2009-12-05 17:23:40 EST
(In reply to comment #7)
> (In reply to comment #5)
> > In Fedora the files
> > 
> >    %{_libdir}/libfoo.so.*
> > go to the main library package, whereas the single file, which is a symlink
> >    %{_libdir}/libfoo.so
> > goes to the devel package. Please fix this in your package accordingly.  
> 
> Oh. I think this must be an error because, if an app should make reference to
> the symlink and the development package is not installed it won't find the
> libarary. So I left all the libraries including he mentioned symlink in the
> base package. Please correct me if I'm wrong about that.  

I don't think I made a mistake. This is the standard way we handle shared libraries in Fedora. 
- .so goes to devel
- .so.* go to the main package (or to the -libs subpackage if there is a 
  further split)

You can verify this by checking any shared library installed in your system.
    rpm -ql libogg
    rpm -ql libogg-devel
    rpm -ql libXinerama
    rpm -ql libXinerama-devel
etc.

When you are building a shared library, the compiler (or ld) needs the .so file. During runtime, on the other hand, the linker takes care of this and it will point your application to the correct .so.* file. So the .so file is only needed during building.
Comment 9 Eli Wapniarski 2009-12-06 00:26:09 EST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > In Fedora the files
> > > 
> > >    %{_libdir}/libfoo.so.*
> > > go to the main library package, whereas the single file, which is a symlink
> > >    %{_libdir}/libfoo.so
> > > goes to the devel package. Please fix this in your package accordingly.  
> > 
> > Oh. I think this must be an error because, if an app should make reference to
> > the symlink and the development package is not installed it won't find the
> > libarary. So I left all the libraries including he mentioned symlink in the
> > base package. Please correct me if I'm wrong about that.  
> 
> I don't think I made a mistake. This is the standard way we handle shared
> libraries in Fedora. 
> - .so goes to devel
> - .so.* go to the main package (or to the -libs subpackage if there is a 
>   further split)
> 
> You can verify this by checking any shared library installed in your system.
>     rpm -ql libogg
>     rpm -ql libogg-devel
>     rpm -ql libXinerama
>     rpm -ql libXinerama-devel
> etc.
> 
> When you are building a shared library, the compiler (or ld) needs the .so
> file. During runtime, on the other hand, the linker takes care of this and it
> will point your application to the correct .so.* file. So the .so file is only
> needed during building.  

I defer to your judgement and request.... And so done :)
Comment 11 Orcan Ogetbil 2009-12-08 05:13:50 EST
Thanks for the update. We are getting there.The SRPM link above is broken but I regenerated it from the SPEC file

* Please don't just use the summary here
   %description devel
   %{summary}.
You can use something like
   The %{name}-devel package contains libraries and header files for developing
   applications that use %{name}.

* These need to be in one line each
   LD_PRELOAD=%{_target_platform}/%{_lib}/libqjson.so
           %{_target_platform}/tests/testparser
   LD_PRELOAD=%{_target_platform}/%{_lib}/libqjson.so
           %{_target_platform}/tests/testserializer

so just add a \ at the end of the first lines.

* This issue still needs to be addressed:

> > - Where can I find an example to properly add FindQJSON.cmake as a requirment?
> >   Or better yet and quicker if you could detail the correct line.
> > 
> 
> I don't know exactly. You might want to do some research. However, I see that
> cmake package put a lot of modules into /usr/share/cmake/Modules/

I found that you can pass this flag to cmake
   -DCMAKE_MODULES_INSTALL_DIR=%{_datadir}/cmake/Modules/
to make it install the cmake module to the correct location. Note that you will need to update the %files section accordingly.

* I fixed the \ issue myself to build the package otherwise it will fail to build. The new package has the rpmlints:
   - qjson.x86_64: E: description-line-too-long JSON is a lightweight 
     data-interchange format. It can represents integer, real number, string,

   Please make the description fit 80 columns.

   - qjson.x86_64: W: incoherent-version-in-changelog 0.6.3-3 ['0.6.3-4.fc12', 
     '0.6.3-4']
   - qjson-devel.x86_64: W: summary-not-capitalized qjson Development Files

   These can be fixed easily

   - qjson.x86_64: W: spurious-executable-perm /usr/share/doc/qjson-0.6.3/html
     /installdox
   - qjson.x86_64: W: doc-file-dependency /usr/share/doc/qjson-0.6.3/html
     /installdox /usr/bin/perl

   I don't think this installdox file should be installed.

   - qjson-devel.x86_64: W: no-documentation

   There are multiple mistakes here:
      . The doxygen documentation you built normally belongs to the devel 
        package. It is supposed to be API documentation.
      . The doxygen documentation is not built properly. Check the generated
        index.html file. It is blank. You need the build the doxygen 
        documentation that is inside the doc/ directory.

Please make a test build in mock and check for rpmlints before you do the next update.
Comment 12 Eli Wapniarski 2009-12-08 15:28:22 EST
I hope we're done.

I think that I made all the changes requested (please double check) and ran rpmlint on the SPEC and SRPM files with zero errors on both files.

The error you got with the doxygen index.html was funny. When I looked at the insides of the built RPMS, everything seemed fine. Could you please double check this with the provided files that I'm linking to here.

Thanks


SPEC URL: http://orbsky.homelinux.org/packages/qjson.spec
SRPM URL: http://orbsky.homelinux.org/packages/qjson-0.6.3-5.fc12.src.rpm
Comment 13 Orcan Ogetbil 2009-12-09 00:30:51 EST
Please change BR: qt to BR-qt-devel. Otherwise the package won't even build in mock, which means it won't build in koji.

Moreover, I made a scratch build in koji with the above BR corrected. You can download the RPM's that are generated. We still have rpmlints:
   qjson-devel.x86_64: W: summary-not-capitalized qjson Development Files
      
      This can be fixed easily.

   qjson-devel.x86_64: W: spurious-executable-perm /usr/share/doc/qjson-devel-0.6.3/html/installdox
   qjson-devel.x86_64: W: doc-file-dependency /usr/share/doc/qjson-devel-0.6.3/html/installdox /usr/bin/perl

      The documentation is still blank. I see this line in the build.log:

      Error: file `/builddir/build/BUILD/qjson/doc/' not found

      Perhaps this will help you finding the problem.

Again, I urge you to build packages on mock. It will make your life easier.
Comment 14 Eli Wapniarski 2009-12-11 02:21:57 EST
(In reply to comment #13)
> Please change BR: qt to BR-qt-devel. Otherwise the package won't even build in
> mock, which means it won't build in koji.
> 
> Moreover, I made a scratch build in koji with the above BR corrected. You can
> download the RPM's that are generated. We still have rpmlints:
>    qjson-devel.x86_64: W: summary-not-capitalized qjson Development Files
> 
>       This can be fixed easily.


Done

>    qjson-devel.x86_64: W: spurious-executable-perm
> /usr/share/doc/qjson-devel-0.6.3/html/installdox
>    qjson-devel.x86_64: W: doc-file-dependency
> /usr/share/doc/qjson-devel-0.6.3/html/installdox /usr/bin/perl
> 

Please see links below and comments regarding blank doxygen html directory:

http://fedoraproject.org/wiki/MinGW/Rpmlint
https://bugzilla.redhat.com/show_bug.cgi?id=467397#c4

The above warning doesn't seem to be of concern to Fedora. The warning is produced because I'm building the doxygen docs during rpmbuild and an installdox script is placed in the html folder on that build.

>       The documentation is still blank. I see this line in the build.log:

Finally clued into this one. Sorry about the misunderstanding. Indeed in the source contained in the SRPM the html folder is indeed blank. However, if you were to examine the SPEC file on line 42 you would see that I build the doxygen during the build process. Which produces the rpmlint warning you mentioned above. Also, the doxygen docs are then successfully installed in the devel package on line 77. So if you were to look in the built devel package, you would see that the doxygen docs are built and in the correct location.

> 
>       Error: file `/builddir/build/BUILD/qjson/doc/' not found
>
>       Perhaps this will help you finding the problem.

If you were to look in both the qjson and qjson-devel package all the documentation specified in the installation goes to the location expected.
 
> Again, I urge you to build packages on mock. It will make your life easier.  

Did that with the same results.

SPEC URL: http://orbsky.homelinux.org/packages/qjson.spec
SRPM URL: http://orbsky.homelinux.org/packages/qjson-0.6.3-6.fc12.src.rpm
Comment 15 Orcan Ogetbil 2009-12-11 14:08:56 EST
1- Please install the devel package and open the index.html that it installs to your /usr/share/doc/qjson-devel-VERSION/ in your browser.

2- Next, unzip the source tarball of qjson manually, go to the doc/ directory. Do a
   doxygen
inside that directory. It will build the docs. Now open the doc/html/index.html that you just generated in your browser.

Do you see a difference between the two index.html's?
Comment 16 Orcan Ogetbil 2009-12-11 20:46:51 EST
Also version 0.7.1 is up. Could you update to the latest version? Thanks.
Comment 17 Orcan Ogetbil 2009-12-11 20:50:10 EST
*** Bug 544737 has been marked as a duplicate of this bug. ***
Comment 18 Eli Wapniarski 2009-12-12 00:04:47 EST
Will do.

Regarding the doxygen thing. I guess I must be a bit thick. Aging will do that to you. :)

Sorry about the confusion.
Comment 19 Eli Wapniarski 2009-12-12 12:27:07 EST
Finally doxygen docs fixed.... I think... I hope.

Thanks for your patience on this.

SPEC URL: http://orbsky.homelinux.org/packages/qjson.spec
SRPM URL: http://orbsky.homelinux.org/packages/qjson-0.7.1-1.fc12.src.rpm
Comment 20 Orcan Ogetbil 2009-12-16 03:29:05 EST
Thanks! I think it is good now.

----------------------------------------
This package (qjson) is APPROVED by oget
----------------------------------------
Comment 21 Eli Wapniarski 2009-12-19 12:33:45 EST
New Package CVS Request
=======================
Package Name: qjson
Short Description: A qt-based library that maps JSON data to QVariant objects
Owners: eliwap oget
Branches: F-11 F-12
InitialCC: oget
Comment 22 Kevin Fenzi 2009-12-21 14:49:20 EST
cvs done.
Comment 23 Fedora Update System 2009-12-22 01:13:42 EST
qjson-0.7.1-1.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/qjson-0.7.1-1.fc12
Comment 24 Fedora Update System 2009-12-22 01:13:46 EST
qjson-0.7.1-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/qjson-0.7.1-1.fc11
Comment 25 Fedora Update System 2009-12-24 15:39:05 EST
qjson-0.7.1-1.fc12 has been pushed to the Fedora 12 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update qjson'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2009-13633
Comment 26 Fedora Update System 2009-12-24 15:39:29 EST
qjson-0.7.1-1.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update qjson'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-13638
Comment 27 Fedora Update System 2009-12-27 15:26:23 EST
qjson-0.7.1-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 28 Fedora Update System 2009-12-27 15:33:30 EST
qjson-0.7.1-1.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 29 Fedora Update System 2010-09-12 02:29:56 EDT
qjson-0.7.1-2.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/qjson-0.7.1-2.fc13
Comment 30 Fedora Update System 2010-09-12 02:30:09 EDT
qjson-0.7.1-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/qjson-0.7.1-2.fc14
Comment 31 Fedora Update System 2010-09-12 02:30:18 EDT
qjson-0.7.1-2.fc12 has been submitted as an update for Fedora 12.
https://admin.fedoraproject.org/updates/qjson-0.7.1-2.fc12
Comment 32 Fedora Update System 2010-09-23 08:51:40 EDT
qjson-0.7.1-2.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 33 Fedora Update System 2010-09-24 16:35:16 EDT
qjson-0.7.1-2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 34 Fedora Update System 2010-09-24 16:44:15 EDT
qjson-0.7.1-2.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

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