Bug 882617 - Review Request: jsoncpp - JSON library implemented in C++
Review Request: jsoncpp - JSON library implemented in C++
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 678809 orthanc
  Show dependency treegraph
 
Reported: 2012-12-02 04:55 EST by Sébastien Willmann
Modified: 2013-03-26 20:36 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-03-16 14:17:15 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bugs.michael: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Sébastien Willmann 2012-12-02 04:55:12 EST
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 06:14:33 EST
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 11:05:26 EST
> 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}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> 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
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
?


> 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

https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
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:

  %{_includedir}/%{name}/

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
> %doc AUTHORS LICENSE NEWS.txt README.txt
>
> %files static
> %doc AUTHORS LICENSE NEWS.txt README.txt
>
> %files devel
> %doc AUTHORS LICENSE NEWS.txt README.txt
>
> %files doc
> %doc AUTHORS LICENSE NEWS.txt README.txt

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 09:56:49 EST
Thanks for packaging jsoncpp! We depend on you to pursue this other review:
https://bugzilla.redhat.com/show_bug.cgi?id=888301

Please keep on the good work!

Best,
Mario
Comment 4 Sébastien Willmann 2012-12-21 16:26:59 EST
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 17:21:55 EST
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 04:57:10 EST
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 05:32:21 EST
I also posted a similar bug two months ago, but without any reply:
https://sourceforge.net/tracker/?func=detail&aid=3577800&group_id=144446&atid=758826
Comment 8 Michael Schwendt 2012-12-22 07:18:37 EST
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
http://sourceforge.net/tracker/?func=detail&aid=3420553&group_id=144446&atid=758826

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

$ repoquery --whatprovides /usr/include/json/json.h
json-c-devel-0:0.10-2.fc18.i686
json-c-devel-0:0.10-2.fc18.x86_64
json-c-devel-0:0.10-2.fc18.i686
json-c-devel-0:0.10-2.fc18.x86_64

For current jsoncpp, the file is stored in another subdir:
/usr/include/jsoncpp/json/json.h

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:
https://fedoraproject.org/wiki/Packaging:Conflicts#Potential_Conflicting_Files
https://fedoraproject.org/wiki/Packaging:Conflicts#Header_Name_Conflicts
Comment 9 Sébastien Willmann 2012-12-22 08:10:05 EST
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 10:07:32 EST
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 14:46:11 EST
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 14:34:52 EST
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


Best,

Mario
Comment 13 Mario Ceresa 2013-02-12 14:36:50 EST
Woops, I forgot:

/usr/lib64/libjsoncpp.so.0.0.0
                         ^^^^^^
maybe the soname here is not set correctly?
Comment 14 Sébastien Willmann 2013-02-18 09:23:32 EST
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 11:47:54 EST
Thanks Sebastien, 

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

Best,
Mario
Comment 16 Michael Schwendt 2013-03-04 18:19:24 EST
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.

libjsoncpp.so.0

$ 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. ;)


APPROVED
Comment 17 Michael Schwendt 2013-03-05 06:07:00 EST
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

vim:ft=diff
Comment 18 Andrey Ponomarenko 2013-03-07 08:13:34 EST
See results of abi-compliance-checker here: http://upstream-tracker.org/versions/jsoncpp.html
Comment 19 Mario Ceresa 2013-03-07 10:03:47 EST
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.

Best,
Mario
Comment 20 Sébastien Willmann 2013-03-14 16:47:46 EDT
New Package SCM Request
=======================
Package Name: jsoncpp
Short Description: JSON library implemented in C++
Owners: wilqu
Branches: f17 f18 f19
InitialCC:
Comment 21 Gwyn Ciesla 2013-03-15 08:44:05 EDT
Git done (by process-git-requests).
Comment 22 Fedora Update System 2013-03-16 14:18:59 EDT
jsoncpp-0.6.0-0.9.rc2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/jsoncpp-0.6.0-0.9.rc2.fc18
Comment 23 Fedora Update System 2013-03-16 14:21:09 EDT
jsoncpp-0.6.0-0.9.rc2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/jsoncpp-0.6.0-0.9.rc2.fc17
Comment 24 Fedora Update System 2013-03-26 20:36:26 EDT
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.