Bugzilla (bugzilla.redhat.com) will be under maintenance for infrastructure upgrades and will not be available on July 31st between 12:30 AM - 05:30 AM UTC. We appreciate your understanding and patience. You can follow status.redhat.com for details.
Bug 882617 - Review Request: jsoncpp - JSON library implemented in C++
Summary: Review Request: jsoncpp - JSON library implemented in C++
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
Depends On:
Blocks: 678809 orthanc
TreeView+ depends on / blocked
Reported: 2012-12-02 09:55 UTC by Sébastien Willmann
Modified: 2013-03-27 00:36 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2013-03-16 18:17:15 UTC
Type: ---
bugs.michael: fedora-review+
gwync: fedora-cvs+

Attachments (Terms of Use)

Description Sébastien Willmann 2012-12-02 09:55:12 UTC
Spec URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp.spec
SRPM URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp-0.6.0-0.1.rc2.fc17.src.rpm
Description: jsoncpp is an implementation of a JSON (http://json.org) reader and writer in C++. JSON (JavaScript Object Notation) is a lightweight data-interchange format. It is easy for humans to read and write. It is easy for machines to parse and generate.

Fedora Account System Username: wilqu

Comment 1 Sébastien Willmann 2012-12-02 11:14:33 UTC
Changed license field to "Public Domain or MIT"

Spec URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp.spec
SRPm URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp-0.6.0-0.2.rc2.fc17.src.rpm

Comment 2 Michael Schwendt 2012-12-09 16:05:26 UTC
> Name:		jsoncpp
> Group:		Development/Libraries

As long as we still fill in the Group Tag, library base packages still enter group "System Environment/Libraries". Group "Development/Libraries" is not for the run-time libs but e.g. -devel packages.

> %package devel
> Requires:	%{name} = %{version}-%{release}


> Requires:	pkgconfig

There is an automatic dependency for a long time.

> %package static
> Summary:	Static libraries for %{name}

What's the reason you don't adhere to

> sed 's/CCFLAGS = "-Wall"/CCFLAGS = "%{optflags}"/' -i SConstruct

As convenient as sed (or Perl) substitutions like this may be, a common mistake is to forget adding a guard. You want to ensure that this expression still matches. Or else it might not apply anymore and you would not apply %optflags then. For example, add a corresponding "grep" command that makes the rpmbuild exit when the 'CCFLAGS = "-Wall"' is not found anymore and could not be substituted.

> %check
> scons platform=linux-gcc check %{?_smp_mflags}

Good! It's always a pleasure to stumble into new packages where the packager uses %check to run available test-suites.

> %install
> install -m 0644 include/json/*.h $RPM_BUILD_ROOT%{_includedir}/%{name}/json

Even if several of the installed files may be fresh builds.

> %{_libdir}/lib%{name}.so.%{src_release}

Have you done a test-build already?
What is the library SONAME and its version?

> %{_libdir}/lib%{name}.a.%{src_release}

Odd. So, with this file name you would link it via its full path instead of just "-ljsoncpp"?

> %{_includedir}/%{name}

The more readable form for directories is the one with a trailing slash:


Not so important for an include dir like this but more readable in general and more explicit where a file entry could also refer to a single file only.

> %files
> %files static
> %files devel
> %files doc

Even if storage space is cheap, these files are duplicated in too many places. Please revisit https://fedoraproject.org/wiki/Packaging:Guidelines#Duplicate_Files

Comment 3 Mario Ceresa 2012-12-19 14:56:49 UTC
Thanks for packaging jsoncpp! We depend on you to pursue this other review:

Please keep on the good work!


Comment 4 Sébastien Willmann 2012-12-21 21:26:59 UTC
Spec URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp.spec
SRPm URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp-0.6.0-0.4.rc2.fc18.src.rpm

It seems that spot already packaged jsoncpp for the chromium repository: http://repos.fedorapeople.org/repos/spot/chromium/fedora-18/SRPMS/

I shamelessly copy/pasted parts of his spec file, to build a proper shared library and to add a pkg-config file.

I think I fixed all the issues. Note that I removed the static and the doc subpackages (the doc is not that big after all)

And I forgot to post rpmlint's output last time:

`--> rpmlint jsoncpp-*
jsoncpp.x86_64: W: no-soname /usr/lib64/libjsoncpp.so.0.0.0
jsoncpp.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/jsoncpp-0.6.0/AUTHORS
jsoncpp-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 3 warnings.

Comment 5 Michael Schwendt 2012-12-21 22:21:55 UTC
No review of that yet, just this:

> jsoncpp.x86_64: W: no-soname /usr/lib64/libjsoncpp.so.0.0.0

That's dangerous.

It's good that Tom did not make up an arbitrary SONAME just to please rpmlint. jsoncpp upstream ought to be involved in decising what SONAME to start with and when, so API/ABI changes are reflected properly in the versioned SONAME.

However, "no-soname" means that there will be no automatic RPM dependency on the right SONAME, but just a weak dependency on the library name. Dependencies, such as Orthanc, could only make that more strict by requiring a specific %version-%release of the jsoncpp package, but updating jsoncpp would require all such dependencies to be rebuilt. Even if one would require a specific jsoncpp %version only (any %release), this could get dangerous, since the Fedora package versioning scheme for pre-/post-releases and snapshots keeps %version constant while the shipped source code may change heavily.

Comment 6 Sébastien Willmann 2012-12-22 09:57:10 UTC
Thanks for all your comments.

I opened a bug on the project's bugtracker for the missing soname: https://sourceforge.net/tracker/?func=detail&aid=3598140&group_id=144446&atid=758826

I also started a topic about the fedora package: https://sourceforge.net/projects/jsoncpp/forums/forum/483465/topic/6513123

Comment 7 Robin Lee 2012-12-22 10:32:21 UTC
I also posted a similar bug two months ago, but without any reply:

Comment 8 Michael Schwendt 2012-12-22 12:18:37 UTC
jsoncpp.pc : 

> Cflags: -I${includedir} -I${includedir}/jsoncpp/

The first  -I${includedir}  should be dropped. It makes no sense, because gcc would ignore it anyway -- and pkg-config drops it, too. ;)

jsoncpp.x86_64 :

> rpmls jsoncpp|less

This base package includes documentation for developers, which should be moved into the -devel package.

The package includes only
-rwxr-xr-x  /usr/lib64/libjsoncpp.so.0.0.0

which is odd, because where is libjsoncpp.so.0?

jsoncpp.spec :

> %check
> scons platform=linux-gcc check %{?_smp_mflags}
> # Now, lets make a proper shared lib. :P
> g++ -o libjsoncpp.so.0.0.0 -shared -Wl,-soname,libjsoncpp.so.0
> buildscons/linux-gcc-*/src/lib_json/*.os -lpthread

I haven't reread the entire spec file, only stumbled into this.

The %check section is the entirely wrong place where to build the shared lib. %check is processed _after_ %install, and it does not (and must not) install any files into the %buildroot either.

Notice also that here it adds a SONAME, starting with libjsoncpp.so.0 which is rude and dangerous. Upstream really ought to be involved here. How is this handled in the Debian package, which is said to create a shared lib, too?

* Just interesting is this upstream bug tracker ticket:

File name conflict

Someone points out a previous conflict between jsoncpp and json-c:

$ repoquery --whatprovides /usr/include/json/json.h

For current jsoncpp, the file is stored in another subdir:

This is a good example about how easy it is to create conflicts and that it's good to watch out for potential conflicts during review:

Comment 9 Sébastien Willmann 2012-12-22 13:10:05 UTC
Spec URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp.spec
SRPM URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp-0.6.0-0.5.rc2.fc18.src.rpm

Well, it seems that I'm bad at copy/pasting :(

The debian package don't seem to do any additional build, and copy the lib to /usr/lib/libjsoncpp.so.0.6.0. See http://packages-powell.debian.org/en/wheezy/armhf/libs/libjsoncpp0

Yes, I was aware of the potential conflict with json-c

Comment 10 Michael Schwendt 2013-01-20 15:07:32 UTC
Debian's  libjsoncpp_0.6.0~rc2-3.debian.tar.gz  file ./patches/fix-SConstruct-soname.patch does set a soname:

  $ grep soname fix-SConstruct-soname.patch 
  +    env.Append( LINKFLAGS='-Wl,-soname,libjsoncpp.so.0')

There are several "sh: dot: command not found" errors in the build output, referring to a missing "BuildRequires: graphviz" 

> $ rpmlint *
> jsoncpp.x86_64: W: no-soname /usr/lib64/libjsoncpp.so.0.0.0
> jsoncpp.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/jsoncpp-
> 0.6.0/AUTHORS
> 3 packages and 0 specfiles checked; 0 errors, 2 warnings.

Comment 11 Sébastien Willmann 2013-01-20 19:46:11 UTC
Spec URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp.spec
SRPM URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp-0.6.0-0.7.rc2.fc18.src.rpm

Actually, a library with a soname was generated but I still installed the old library without a soname. Again, this is because I pasted a line without reading it very carefully.

I now install the right lib with a soname. Is it OK like that or do I have to use the patch?

I also added the missing BuildRequires.

Comment 12 Mario Ceresa 2013-02-12 19:34:52 UTC
Hi! It seems that the package is in a good shape. Any plans to finish it soon? We would like to use it in the review of orthanc.

fedora review complains of:

[!]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 3840000 bytes in 293 files.

jsoncpp.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libjsoncpp.so.0.0.0 /lib64/libm.so.6
jsoncpp.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/jsoncpp-0.6.0/AUTHORS



Comment 13 Mario Ceresa 2013-02-12 19:36:50 UTC
Woops, I forgot:

maybe the soname here is not set correctly?

Comment 14 Sébastien Willmann 2013-02-18 14:23:32 UTC
Spec URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp.spec
SRPM URL: http://wilqu.fedorapeople.org/reviews/jsoncpp/jsoncpp-0.6.0-0.8.rc2.fc18.src.rpm

> fedora review complains of:
> [!]: Large documentation must go in a -doc subpackage.
>      Note: Documentation size is 3840000 bytes in 293 files.
The subpackage was there in the beginning, but I removed it because I thought it was useless. I created it again in this release.

> jsoncpp.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libjsoncpp.so.0.0.0 /lib64/libm.so.6
Weird, I don't have this warning.

> /usr/lib64/libjsoncpp.so.0.0.0
>                          ^^^^^^
> maybe the soname here is not set correctly?
Upstream does not provide a soname, so I use 0.0.0. See comment 6.

Comment 15 Mario Ceresa 2013-03-04 16:47:54 UTC
Thanks Sebastien, 

I'm curious if Michael sees any additional problems or if the package can be approved (and used to build orthanc :) )


Comment 16 Michael Schwendt 2013-03-04 23:19:24 UTC
Hmmm, with regard to the shared library this is sort of a grey area. We don't have anything in the packaging guidelines that comments on "making up versioned SONAMEs".

I can only repeat comment 8 and approve this package with a big fat warning only.

You will need to take the full risk and the full responsibility for shipping a libjsoncpp.so.0 that may be incompatible with a future upstream release, other distributions, or even updates of jsoncpp within Fedora. The automatic RPM SONAME dependencies won't help you either within the Fedora package collection. You'll be on your own here, and you'll need to be very careful and check the API/ABI of future updates with the help of tools like diff, rpmsodiff and abi-compliance-checker, for example. If the ABI breaks often, it would be better to use a stricter SONAME version, but with the added penalty that you would need to invent a suitable versioning scheme or rebuild dependencies more often than necessary. As a last resort, you could continue building a shared lib, but with a SONAME that would change [almost] always.

It's also less than ideal that upstream has not responded to the ticket and/or forum thread which you've opened.

In case of doubts, it might also be an idea to consult the Fedora packaging mailing-list for feedback on this.

And, of course, this is an opportunity to team up with the Orthanc packagers and have more people check/co-maintain future jsoncpp updates/upgrades.

> Upstream does not provide a soname, so I use 0.0.0. See comment 6.


$ eu-readelf -d /usr/lib64/libjsoncpp.so.0.0.0 |grep SO
  SONAME            Library soname: [libjsoncpp.so.0]

> # Build the doc
> python doxybuild.py 

Python is only available indirectly because of Scons.

There are two minor packaging issues left, which shouldn't block the package, however:

 * jsoncpp.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/jsoncpp-0.6.0/AUTHORS

 * jsoncpp-doc : /usr/share/doc/jsoncpp/index.html links a few local files, which are only packaged in the base jsoncpp package %doc dir. E.g. LICENSE, *.txt. Those links give 404 not found, of course. Duplicating those %doc files in the -doc package would be acceptable here, IMO. Making jsoncpp-doc depend on the base package would not be good, because Documentation packages usually should stay independent.

> Summary:    An implementation of a JSON reader and writer in C++

And last but not least, now that I've had another look at the spec, I'm not a fan of leading articles at the beginning of the "Summary". When those summaries are displayed by Anaconda and package tools, that looks ugly if many of them start with "An, "A, "The". Nowadays most packages drop leading articles, I think.
Also, mentioning that this is a library or API might be better. Mentioning that reading and writing is implemented might be too much, because a future version might also offer checking/validating. It's okay if the description expands on such details.
  Summary: JSON API for C++
  Summary: JSON library implemented in C++
  Summary: C++ library for reading and writing JSON
Up to you, of course. ;)


Comment 17 Michael Schwendt 2013-03-05 11:07:00 UTC
JFYI, a comparison with spot's package: 3 symbols removed

$ rpmsodiff ./jsoncpp/0.6.0-0.1.20120626svn249.fc18/x86_64/jsoncpp-0.6.0-0.1.20120626svn249.fc18.x86_64.rpm ./jsoncpp/0.6.0-0.8.rc2.fc18/x86_64/jsoncpp-0.6.0-0.8.rc2.fc18.x86_64.rpm
	common sonames:
libjsoncpp.so.0	/usr/lib64/libjsoncpp.so.0.0.0	/usr/lib64/libjsoncpp.so.0.0.0

--- jsoncpp-0.6.0-0.1.20120626svn249.fc18/libjsoncpp.so.0	2013-03-05 12:02:39.785966992 +0100
+++ jsoncpp-0.6.0-0.8.rc2.fc18/libjsoncpp.so.0	2013-03-04 23:09:38.398183855 +0100
@@ -1,3 +1,2 @@
 _ZN4Json10FastWriter10writeValueERKNS_5ValueE	T
-_ZN4Json10FastWriter20dropNullPlaceholdersEv	T
 _ZN4Json10FastWriter23enableYAMLCompatibilityEv	T
@@ -227,3 +226,2 @@
 _ZNK4Json5Value7isArrayEv	T
-_ZNK4Json5Value7isInt64Ev	T
 _ZNK4Json5Value8CZString14isStaticStringEv	T
@@ -241,3 +239,2 @@
 _ZNK4Json5Value8isStringEv	T
-_ZNK4Json5Value8isUInt64Ev	T
 _ZNK4Json5Value9asCStringEv	T

	3 symbols removed
T _ZN4Json10FastWriter20dropNullPlaceholdersEv
T _ZNK4Json5Value7isInt64Ev
T _ZNK4Json5Value8isUInt64Ev


Comment 18 Andrey Ponomarenko 2013-03-07 13:13:34 UTC
See results of abi-compliance-checker here: http://upstream-tracker.org/versions/jsoncpp.html

Comment 19 Mario Ceresa 2013-03-07 15:03:47 UTC
Hello Michael, Sebastien and Andrey,
Thanks for your hard work and contributions to this package. I'm very happy that jsoncpp was finally approved, even if the lack of soname support from upstream could bring some problems later on. Hopefully the link from Andrey will be useful to spot them before pushing any updates.


Comment 20 Sébastien Willmann 2013-03-14 20:47:46 UTC
New Package SCM Request
Package Name: jsoncpp
Short Description: JSON library implemented in C++
Owners: wilqu
Branches: f17 f18 f19

Comment 21 Gwyn Ciesla 2013-03-15 12:44:05 UTC
Git done (by process-git-requests).

Comment 22 Fedora Update System 2013-03-16 18:18:59 UTC
jsoncpp-0.6.0-0.9.rc2.fc18 has been submitted as an update for Fedora 18.

Comment 23 Fedora Update System 2013-03-16 18:21:09 UTC
jsoncpp-0.6.0-0.9.rc2.fc17 has been submitted as an update for Fedora 17.

Comment 24 Fedora Update System 2013-03-27 00:36:26 UTC
jsoncpp-0.6.0-0.9.rc2.fc18 has been pushed to the Fedora 18 stable repository.

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