http://scop.fedorapeople.org/packages/jing.spec http://scop.fedorapeople.org/packages/jing-20091111-3.fc14.src.rpm Jing is a RELAX NG validator written in Java. It implements the RELAX NG 1.0 Specification, RELAX NG Compact Syntax, and parts of RELAX NG DTD Compatibility, specifically checking of ID/IDREF/IDREFS. It also has experimental support for schema languages other than RELAX NG; specifically W3C XML Schema, Schematron 1.5, and Namespace Routing Language. http://www.thaiopensource.com/relaxng/jing.html
I'll do the review
I have a suggestion/question. Wouldn't it make more sense to actually use upstream build system? You are using binary release with included sources that you compile manually...thus possibly creating differences from upstream (unknowingly). I know upstream doesn't do source releases, most packages I have seen/worked with actually use SCM checkouts to get sources and build instructions in those cases. Is there a good reason not to do that in this case?
On a quick look the only thing using the upstream source checkout would buy us would be that the test suite is included in it. I'll have a look at using it, and if I end up doing so, will also quite probably build trang and dtdinst from it since it's one checkout that contains them all (there's a separate review request for trang which would become obsolete).
http://scop.fedorapeople.org/packages/jing-trang.spec http://scop.fedorapeople.org/packages/jing-trang-20091111-4.fc13.src.rpm It took some effort to get this built from upstream svn checkout, but here goes. Unfortunately not all of the test suite could be done because (generating) it has dependencies to an old version of saxon which we don't have. All rpmlint messages are ignorable: dtdinst.noarch: W: no-manual-page-for-binary dtdinst jing-trang.src: W: strange-permission jing-trang-prepare-tarball.sh 0755L jing-trang.src: W: invalid-url Source0: jing-trang-20091111.tar.xz jing.noarch: W: spelling-error Summary(en_US) validator -> invalidator, validation, validate jing.noarch: W: spelling-error %description -l en_US validator -> invalidator, validation, validate jing.noarch: W: dangling-symlink /usr/share/doc/jing-20091111-4.fc14/doc/api /usr/share/javadoc/jing jing.noarch: W: no-manual-page-for-binary jing trang.noarch: W: spelling-error Summary(en_US) Multi -> Mulch, Mufti trang.noarch: W: no-manual-page-for-binary trang 5 packages and 0 specfiles checked; 0 errors, 9 warnings.
*** Bug 655581 has been marked as a duplicate of this bug. ***
Package Review ============== Key: - = N/A x = Check ! = Problem ? = Not evaluated === REQUIRED ITEMS === [x] Rpmlint output: dtdinst.noarch: W: no-manual-page-for-binary dtdinst jing.noarch: W: spelling-error Summary(en_US) validator -> invalidator, validation, validate jing.noarch: W: spelling-error %description -l en_US validator -> invalidator, validation, validate jing.noarch: W: dangling-symlink /usr/share/doc/jing-20091111-4.fc15/doc/api /usr/share/javadoc/jing jing.noarch: W: no-manual-page-for-binary jing jing-trang.src: W: strange-permission jing-trang-prepare-tarball.sh 0755L jing-trang.src: W: invalid-url Source0: jing-trang-20091111.tar.xz trang.noarch: W: spelling-error Summary(en_US) Multi -> Mulch, Mufti trang.noarch: W: no-manual-page-for-binary trang 5 packages and 0 specfiles checked; 0 errors, 9 warnings. The jing-trang-prepare-tarball.sh doesn't have to be included in the srpm since it already includes the tarball. Generally we just include it in repository close to spec file [x] Package is named according to the Package Naming Guidelines[1]. [x] Spec file name must match the base package name, in the format %{name}.spec. [x] Package meets the Packaging Guidelines[2]. [x] Package successfully compiles and builds into binary rpms. [!] Buildroot definition is not present According to new guidelines Buildroot definitions are to be omitted (automatically provided by recent rpmbuild) [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines[3,4]. [x] License field in the package spec file matches the actual license. License type: BSD [x] If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. But why do all the (from my POV) unnecessary directory creating in %{_docdir} instead of using %doc macro in %files section. No need to do all that copying during %install Something like this can do the trick: -- %doc readme.html doc/ sample/ -- [x] All independent sub-packages have license of their own [x] Spec file is legible and written in American English. [x] Sources used to build the package matches the upstream source, as provided in the spec URL. [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines[5]. [x] Package must own all directories that it creates. [x] Package requires other packages for directories it uses. [x] Package does not contain duplicates in %files. [x] Permissions on files are set properly. [!] Package does NOT have a %clean section which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). (not needed anymore) %clean and rm -rf at the beginning of %install is not needed and can be omitted [x] Package consistently uses macros (no %{buildroot} and $RPM_BUILD_ROOT mixing) [x] Package contains code, or permissable content. [x] Fully versioned dependency in subpackages, if present. [-] Package contains a properly installed %{name}.desktop file if it is a GUI application. [x] Package does not own files or directories owned by other packages. [x] Javadoc documentation files are generated and included in -javadoc subpackage [x] Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlinks) [x] Packages have proper BuildRequires/Requires on jpackage-utils [x] Javadoc subpackages have Require: jpackage-utils Not really but through a dependency.. [-] Package uses %global not %define [x] If package uses tarball from VCS include comment how to re-create that tarball (svn export URL, git clone URL, ...) [x] If source tarball includes bundled jar/class files these need to be removed prior to building [x] All filenames in rpm packages must be valid UTF-8. [x] Jar files are installed to %{_javadir}/%{name}.jar (see [6] for details) [-] If package contains pom.xml files install it (including depmaps) even when building with ant [-] pom files has correct add_to_maven_depmap call which resolves to the pom file (use "JPP." and "JPP-" correctly) === Other suggestions === [x] If possible use upstream build method (maven/ant/javac) [x] Avoid having BuildRequires on exact NVR unless necessary [x] Package has BuildArch: noarch (if possible) [x] Latest version is packaged. [x] Reviewer should test that the package builds in mock. Tested on: fedora-rawhide-x86_64 === Issues === 1. (optional) jing-trang-prepare-tarball.sh from srpm can be removed from srpm 2. simplify documentation installation by using %doc macro 3. remove %clean section and rm -rf from %install section Other than these three things, everything seems to be OK to me. I must say I am impressed with the patch and bugs you filed upstream to fix the build system a bit. Great work.
> 1. (optional) jing-trang-prepare-tarball.sh from srpm can be removed from srpm All the packages I've worked with include it in the source rpm. It doesn't cost anything to have it there, and the upside is that people can actually verify the contents of the srpm as well as roll updates just with the information in it without having to hunt missing bits from elsewhere. > 2. simplify documentation installation by using %doc macro I intentionally didn't do that initially because for some reason I remembered that the special %doc macro would do a "cp --parents" and would thus include unwanted subdirs below the doc dirs. But that's incorrect, simplified in -5. > 3. remove %clean section and rm -rf from %install section These (and the Group tags) are intentionally left there because I want the package to be cleanly buildable as is for example on EL-5 (assuming dependencies are available). They don't cause any problems, and I can easily live with them around. > I must say I am impressed with the patch and bugs you filed upstream to fix > the build system a bit. Great work. Thanks, and thanks for the review which got quite a bit bigger than what you initially signed up for. Next revision with the %doc stuff simplified: http://scop.fedorapeople.org/packages/jing-trang.spec http://scop.fedorapeople.org/packages/jing-trang-20091111-5.fc14.src.rpm Some things I plan to look into whether they'd make sense and be feasible to implement in the future for this package, let me know if you'd like them or some of them done during the review, or have any opinions about them, in no particular order: - Try to port the missing test suite generation bits to some other XSLT processor than the old saxon, and if successful, run the whole test suite. - Install dtdinst's schemas and XSLs as non-%doc and register them with the system XML catalog. - Review jing's %docs; isorelax.copying.txt and xerces.copying.txt could/should maybe be dropped because they're not shipped in this package (and copying.html maybe modified not to link to them) and maybe datatype-sample.jar should be dropped as well.
(In reply to comment #7) > > 1. (optional) jing-trang-prepare-tarball.sh from srpm can be removed from srpm > > All the packages I've worked with include it in the source rpm. It doesn't > cost anything to have it there, and the upside is that people can actually > verify the contents of the srpm as well as roll updates just with the > information in it without having to hunt missing bits from elsewhere. Sure, no problem. > > 2. simplify documentation installation by using %doc macro > > I intentionally didn't do that initially because for some reason I remembered > that the special %doc macro would do a "cp --parents" and would thus include > unwanted subdirs below the doc dirs. But that's incorrect, simplified in -5. Good > > 3. remove %clean section and rm -rf from %install section > > These (and the Group tags) are intentionally left there because I want the > package to be cleanly buildable as is for example on EL-5 (assuming > dependencies are available). They don't cause any problems, and I can easily > live with them around. I though that might be the case. OK, no problem then. > > I must say I am impressed with the patch and bugs you filed upstream to fix > > the build system a bit. Great work. > > Thanks, and thanks for the review which got quite a bit bigger than what you > initially signed up for. Next revision with the %doc stuff simplified: > > http://scop.fedorapeople.org/packages/jing-trang.spec > http://scop.fedorapeople.org/packages/jing-trang-20091111-5.fc14.src.rpm Looking into this now. > Some things I plan to look into whether they'd make sense and be feasible to > implement in the future for this package, let me know if you'd like them or > some of them done during the review, or have any opinions about them, in no > particular order: > > - Try to port the missing test suite generation bits to some other XSLT > processor than the old saxon, and if successful, run the whole test suite. I think the whole building process for Fedora and other distributions would be better off with no XSLT generation at all...but maybe I am wrong, since I just had a glimpse at what you had to do to make this work. > - Install dtdinst's schemas and XSLs as non-%doc and register them > with the system XML catalog. Yes this should happen too. You probably know how to do it, but just in case...you can look at log4j spec file which installs log4j dtds > - Review jing's %docs; isorelax.copying.txt and xerces.copying.txt > could/should maybe be dropped because they're not shipped in this package > (and copying.html maybe modified not to link to them) and maybe > datatype-sample.jar should be dropped as well. Good idea, also asking upstream about it might help.
Ok, sorry about not noticing it right away, but javadoc sub-package doesn't include license. There was recent change in packaging guidelines (~2-3 months ago) that requires all independent packages to include license. Since your javadoc doesn't Require either of the other sub-packages it has to include license. Fix that and you have my "+".
Created attachment 463835 [details] For reference: port of test suite generation to new Saxon, XSLT 2.0 http://scop.fedorapeople.org/packages/jing-trang.spec http://scop.fedorapeople.org/packages/jing-trang-20091111-6.fc14.src.rpm - Patch test suite generation to use Xalan. - Include license texts in jing-javadoc. - Make datatype-sample buildable out of the box, drop prebuilt jar. Porting the test suite generation turned out to be surprisingly painful. Porting it to "new" Saxon did not work out - it worked with the upstream saxon9.jar but no longer does with the newer Saxon open source "HE" versions which is what we have in Fedora due to missing features in it, patch (which ports it to XSLT 2.0) attached just for future reference. Using EXSLT didn't work either - new Saxon HE does not have support for that stuff, and Xalan implements only a subset of it which isn't enough. Switching it to XSLT 1.1 + Xalan extensions worked though, but I had to disable one test case which triggers an apparent Xalan bug; see comments in the xalan patch. Xalan doesn't currently support XSLT 2.0. I decided not to do anything about the license stuff yet because I don't have a good feeling about modifying upstream license texts (copying.html). dtdinst schema+xml moving from %doc and cataloguing was also postponed, will take a look later. Both are TODO comments in the specfile.
Hmm, the build fails in my local mock and even koji now: http://koji.fedoraproject.org/koji/taskinfo?taskID=2636658
Hm, it builds fine for F-13 and F-14 mock here, but IIRC it ends up being built with openjdk for me and not gcj like the koji log indicates. There's a leftover buglet from my Saxon tries which I suppose may cause this; "export CLASSPATH=$(build-classpath saxon)" should be replaced by "export CLASSPATH=$(build-classpath xalan-j2)" at start of %build. Can't test right now, but probably can tomorrow.
http://scop.fedorapeople.org/packages/jing-trang.spec http://scop.fedorapeople.org/packages/jing-trang-20091111-7.fc14.src.rpm http://koji.fedoraproject.org/koji/taskinfo?taskID=2639853 - Put Xalan instead of Saxon in build path (regression in -6). - Build with OpenJDK. Comment 12 was correct, but it's not enough to fix the build with GCJ, there are other issues. This one's built with OpenJDK, I don't plan to spend time to get the build working with GCJ.
Package seems good now, builds fine and is all in all very nice :-) APPROVED
New Package SCM Request ======================= Package Name: jing-trang Short Description: Schema validation and conversion based on RELAX NG Owners: scop Branches: f14 InitialCC:
Git done (by process-git-requests).
jing-trang-20091111-7.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/jing-trang-20091111-7.fc14
jing-trang-20091111-7.fc14 has been pushed to the Fedora 14 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 jing-trang'. You can provide feedback for this update here: https://admin.fedoraproject.org/updates/jing-trang-20091111-7.fc14
jing-trang-20091111-7.fc14 has been pushed to the Fedora 14 stable repository. If problems still persist, please make note of it in this bug report.
How come there's no build of this in Rawhide?
I thought that the rawhide compose would pull it automatically in from previous release updates like it tends to do if a package hasn't been built for rawhide and there's a newer version in previous released updates, but perhaps it doesn't work the same way for new packages. Anyway, it's built for rawhide now as well.
I think inheritance was deliberately broken between F14 and Rawhide for something related to deltarpm creation and the new xz file format in Rawhide.
I see. I've just posted to the devel list asking for clarification.