[Bug 820115] New: Review Request: leptonica - C library for efficient image processing and image analysis operations
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: leptonica - C library for efficient image processing and image analysis operations
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Summary: Review Request: leptonica - C library for efficient image processing and image analysis operations Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: dchen@redhat.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org Classification: Fedora Story Points: --- Type: --- Regression: --- Mount Type: --- Documentation: ---
Spec URL: http://dchen.fedorapeople.org/files/rpms/leptonica.spec SRPM URL: http://dchen.fedorapeople.org/files/rpms/leptonica-1.68-2.fc16.src.rpm Description: The library supports many operations that are useful on * Document images * Natural images
Fundamental image processing and image analysis operations * Rasterop (aka bitblt) * Affine transforms (scaling, translation, rotation, shear) on images of arbitrary pixel depth * Projective and bi-linear transforms * Binary and gray scale morphology, rank order filters, and convolution * Seed-fill and connected components * Image transformations with changes in pixel depth, both at the same scale and with scale change * Pixel<-wise masking, blending, enhancement, arithmetic ops, etc.
Ancillary utilities * I/O for standard image formats (jpg, png, tiff, bmp, pnm, gif, ps) * Utilities to handle arrays of image-related data types (e.g., pixa, boxa, pta) * Utilities for stacks, generic arrays, queues, heaps, lists; number and string arrays; etc.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Michal Nowak mnowak@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |748135
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Volker Fröhlich volker27@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |volker27@gmx.at
--- Comment #1 from Volker Fröhlich volker27@gmx.at --- Please require the base package in the architecture dependent way: http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
Use global instead of define: http://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_...
If you don't want to introduce this package to EPEL 5, remove the buildroot definition, the clean section and the cleaning rm in the install section.
Don't use the rm, make or install macro. Just use the plain commands.
defattr is no longer necessary.
Many of the filenames in bindir are generic. Consider to use a prefix. Since the binaries are 2.9 MB and the library is only 1.6, I suggest to create a libs-sub-package.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #2 from Ding-Yi Chen dchen@redhat.com --- I will suggest that binary files to be packed as -util, as 1) They are mostly ancillary utilities http://code.google.com/p/leptonica/ 2) As you said, the names are generic. 3) Other distro seems to pack in similary way as well, for example, debian pack the binary and man pages in -progs
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #3 from Ding-Yi Chen dchen@redhat.com --- Concern in comment1 addressed:
SPEC: http://dchen.fedorapeople.org/files/rpms/leptonica.spec SRPM: http://dchen.fedorapeople.org/files/rpms/leptonica-1.68-3.fc17.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Kevin Kofler kevin@tigcc.ticalc.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |kevin@tigcc.ticalc.org
--- Comment #4 from Kevin Kofler kevin@tigcc.ticalc.org --- I think those binaries really can't be packaged that way, without some kind of namespacing (name prefix or whatever). Some of the names are extremely generic and likely to cause conflicts. Others at least follow some naming scheme, but there I'm not sure those are useful to package at all, e.g. tests probably don't make sense to install.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #5 from Ding-Yi Chen dchen@redhat.com --- (In reply to comment #4)
I think those binaries really can't be packaged that way, without some kind of namespacing (name prefix or whatever). Some of the names are extremely generic and likely to cause conflicts. Others at least follow some naming scheme, but there I'm not sure those are useful to package at all, e.g. tests probably don't make sense to install.
Although debian and friend do ppackaged "as-is", after test some of the binaries, I would agree you and remove them from release unless someone asks for them.
SPEC: http://dchen.fedorapeople.org/files/rpms/leptonica.spec SRPM: http://dchen.fedorapeople.org/files/rpms/leptonica-1.68-4.fc17.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Ralf Corsepius rc040203@freenet.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |rc040203@freenet.de
--- Comment #6 from Ralf Corsepius rc040203@freenet.de --- (In reply to comment #5)
(In reply to comment #4)
Although debian and friend do ppackaged "as-is", after test some of the binaries, I would agree you and remove them from release unless someone asks for them.
You could pass --program-prefix=<whatever> (e.g. --program-prefix=leptonica-) to configure. Then, unless the package's configuration is broken, the packages should install its binaries as <whatever><program> instead of <program>
Apart of this, I am inclined to believe some of these binaries likely are testing-programs, which never should be installed (This would mean the package's configuration is broken).
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #7 from Ding-Yi Chen dchen@redhat.com --- Upstream update to 1.69. Add --program-prefix=leptonica- to configure
SPEC: http://dchen.fedorapeople.org/files/rpms/leptonica.spec SRPM: http://dchen.fedorapeople.org/files/rpms/leptonica-1.69-1.fc17.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Ryan Curtin ryan@igglybob.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ryan@igglybob.com
--- Comment #8 from Ryan Curtin ryan@igglybob.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #9 from Ding-Yi Chen dchen@redhat.com --- (In reply to comment #8)
$ 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.
It has a .tar.gz download. See http://code.google.com/p/leptonica/downloads/list
You can even use spectool -g leptonica.spec
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.
Mmm, I have read http://www.leptonica.com/about-the-license.html again and it seems that it has its own license. Will contacts legal-list and see what they said.
rm -f %{buildroot}%{_libdir}/liblept.la
Since you've already defined %{libname} as liblept, couldn't you use %{libname}.la?
Will do.
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. :)
This is actually the "offical" way to remove rpath. See:
http://fedoraproject.org/wiki/Packaging:Guidelines#Removing_Rpath
I also think there may be a typo in the description:
- Pixel<-wise masking, blending, enhancement, arithmetic ops,
Should that be 'pixel-wise'?
Thanks, I'll fix the address issues and contact the legal-list about the license.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #10 from Ryan Curtin ryan@igglybob.com --- (In reply to comment #9)
(In reply to comment #8)
$ 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.
It has a .tar.gz download. See http://code.google.com/p/leptonica/downloads/list
How bizarre; that 404'ed last night for me but it is working now.
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. :)
This is actually the "offical" way to remove rpath. See:
http://fedoraproject.org/wiki/Packaging:Guidelines#Removing_Rpath
Ah, I must be asleep at the wheel. That can't be patched easily because it's not modifying the sources, it's modifying the output of the configure script. Oops.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #11 from Ding-Yi Chen dchen@redhat.com --- SPEC: http://dchen.fedorapeople.org/files/rpms/leptonica.spec SRPM: http://dchen.fedorapeople.org/files/rpms/leptonica-1.69-2.fc17.src.rpm
Legal team as include Leptonica, see https://fedoraproject.org/wiki/Licensing/Leptonica
rpmlint might complained: W: invalid-license Leptonica
This warning can be waived until next rpmlint release.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Sergio Monteiro Basto sergio@serjux.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |sergio@serjux.com
--- Comment #12 from Sergio Monteiro Basto sergio@serjux.com --- Hi, we need this to update tesseract-ocr to 3.01 else:
checking for leptonica... configure: error: leptonica not found
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #13 from Ding-Yi Chen dchen@redhat.com --- (In reply to comment #12)
Hi, we need this to update tesseract-ocr to 3.01 else:
checking for leptonica... configure: error: leptonica not found
Mind doing the review for me?
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #14 from Sergio Monteiro Basto sergio@serjux.com --- Created attachment 609462 --> https://bugzilla.redhat.com/attachment.cgi?id=609462&action=edit clean cleans
Looks good, but you may clean some rm(s) . Sorry I don't know how review it, I don't know if I have permissions, also I don't have spare time.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #15 from Ding-Yi Chen dchen@redhat.com --- (In reply to comment #14)
Created attachment 609462 [details] clean cleans
Well, I do intend to put this in EPEL5. :-)
Sorry I don't know how review it, I don't know if I have permissions, also I don't have spare time.
As long as you are a fedora packager, you can review and approve. Even if you are not, you can still do the review, but need others to approve.
Do worry about the review, everyone has his/her first time.
Link for Package Review process http://fedoraproject.org/wiki/Package_Review_Process
Link for Review guideline: http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
And this tool help you to automate some tasks of the review: https://fedorahosted.org/FedoraReview/
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Sergio Monteiro Basto sergio@serjux.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Sergio Monteiro Basto sergio@serjux.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |sergio@serjux.com
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Sergio Monteiro Basto sergio@serjux.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? | Flags| |fedora-review+
--- Comment #16 from Sergio Monteiro Basto sergio@serjux.com --- no MUST items neither other issues
ACCEPT
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Ding-Yi Chen dchen@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
--- Comment #17 from Ding-Yi Chen dchen@redhat.com --- New Package SCM Request ======================= Package Name: leptonica Short Description: C library for efficient image processing and image analysis operations Owners: dchen Branches: f18 f17 InitialCC:
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #18 from Jon Ciesla limburgher@gmail.com --- Git done (by process-git-requests).
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #19 from Fedora Update System updates@fedoraproject.org --- leptonica-1.69-2.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/leptonica-1.69-2.fc18
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #20 from Fedora Update System updates@fedoraproject.org --- leptonica-1.69-2.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/leptonica-1.69-2.fc17
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #21 from Fedora Update System updates@fedoraproject.org --- leptonica-1.69-2.fc18 has been pushed to the Fedora 18 testing repository.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA Last Closed| |2012-09-23 23:20:44
--- Comment #22 from Fedora Update System updates@fedoraproject.org --- leptonica-1.69-2.fc18 has been pushed to the Fedora 18 stable repository.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Sandro Mani manisandro@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |manisandro@gmail.com Flags|fedora-cvs+ |fedora-cvs?
--- Comment #23 from Sandro Mani manisandro@gmail.com --- Package Change Request ====================== Package Name: leptonica New Branches: el6 epel7 Owners: smani
https://bugzilla.redhat.com/show_bug.cgi?id=820115
--- Comment #24 from Jon Ciesla limburgher@gmail.com --- This SCM request method has been deprecated. Please see https://fedoraproject.org/wiki/PackageDB_admin_requests.
https://bugzilla.redhat.com/show_bug.cgi?id=820115
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-cvs? |
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla