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
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
> 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
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
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.
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.
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
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
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
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
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.
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.
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
Woops, I forgot: /usr/lib64/libjsoncpp.so.0.0.0 ^^^^^^ maybe the soname here is not set correctly?
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.
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
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
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
See results of abi-compliance-checker here: http://upstream-tracker.org/versions/jsoncpp.html
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
New Package SCM Request ======================= Package Name: jsoncpp Short Description: JSON library implemented in C++ Owners: wilqu Branches: f17 f18 f19 InitialCC:
Git done (by process-git-requests).
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
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
jsoncpp-0.6.0-0.9.rc2.fc18 has been pushed to the Fedora 18 stable repository.