https://bugzilla.redhat.com/show_bug.cgi?id=820115 Ryan Curtin <ryan@xxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ryan@xxxxxxxxxxxx --- Comment #8 from Ryan Curtin <ryan@xxxxxxxxxxxx> --- Hello there, I am an unsponsored reviewer, so this is an unofficial review; I may have written some things that are incorrect... nonetheless, I found a couple issues. Any of the MUST/SHOULD guidelines that were okay I didn't include here (for the sake of space) but I did check them. First I noticed that the package no longer installs any binaries, so the comments addressing that are now no longer applicable, I suppose. >>> MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review. $ rpmlint -v leptonica.spec leptonica.spec: I: checking-url http://leptonica.googlecode.com/files/leptonica-1.69.tar.gz (timeout 10 seconds) leptonica.spec: W: invalid-url Source0: http://leptonica.googlecode.com/files/leptonica-1.69.tar.gz HTTP Error 404: Not Found 0 packages and 1 specfiles checked; 0 errors, 1 warnings. It seems like the actual package URL is a .tar.bz2. I made that substitution to perform the rest of the review and testing. >>> MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines. >>> MUST: The License field in the package spec file must match the actual license. The package is listed as using the ASL 2.0 license but the file leptonica-license.txt does not seem to be the same as the ASL. It seems similar but differs specifically in that clause 4.2 of the ASL 2.0 license ("you must cause any modified files to carry prominent notices stating that You changed the files") does not seem to be present in the Leptonica license. I can't seem to discern based on Wiki resources whether or not calling it ASL 2.0 is okay; after all, the two do seem fairly similar. >>> MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this. This should be okay once the Source0 line is fixed. >>> MUST: Each package must consistently use macros. This may be, and probably is, unnecessary pedantry, but > rm -f %{buildroot}%{_libdir}/liblept.la Since you've already defined %{libname} as liblept, couldn't you use %{libname}.la? >>> SHOULD: The reviewer should test that the package builds in mock. >>> SHOULD: The package should compile and build into binary rpms on all supported architectures. Builds just fine. http://koji.fedoraproject.org/koji/taskinfo?taskID=4343521 >>> SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. I think a patch should be used instead of the sed substitution; if the sed lines no longer become necessary (or perhaps even become harmful after upstream updates), sed will not fail. Patches, on the other hand, will throw errors and the problem is clear at buildtime and does not manifest as a bizarre runtime bug. I have seen some other reviews where people have suggested this (prefer patches to sed) but I can't find a particular guideline indicating it. So I guess this is just my opinion. :) ---- I also think there may be a typo in the description: > * Pixel<-wise masking, blending, enhancement, arithmetic ops, Should that be 'pixel-wise'? Hopefully this is a helpful review. I apologize for any errors I have made. -- You are receiving this mail because: You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review