Bug 226217 - Merge Review: opensp
Summary: Merge Review: opensp
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Ville Skyttä
QA Contact: Fedora Package Reviews List
Depends On:
TreeView+ depends on / blocked
Reported: 2007-01-31 20:19 UTC by Nobody's working on this, feel free to take it
Modified: 2008-10-24 07:25 UTC (History)
2 users (show)

Fixed In Version: opensp-1.5.2-9.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2008-10-23 08:57:14 UTC
Type: ---
ville.skytta: fedora-review+
wtogami: fedora-cvs+

Attachments (Terms of Use)
Drop *.la, fix License tag, update comments (1.08 KB, patch)
2008-10-22 16:36 UTC, Ville Skyttä
no flags Details | Diff

Description Nobody's working on this, feel free to take it 2007-01-31 20:19:05 UTC
Fedora Merge Review: opensp

Initial Owner: twaugh@redhat.com

Comment 1 Ondrej Vasik 2007-07-27 08:23:12 UTC
Package Change Request
Package Name: opensp
Updated Fedora Owners: ovasik@redhat.com

Comment 2 Ville Skyttä 2008-10-22 16:36:04 UTC
The package is in a good shape, the only things I think should be done are:

- Drop *.la,
- License: MIT instead of BSD, and
- %check comments could be updated.

Will attach a patch.

Comment 3 Ville Skyttä 2008-10-22 16:36:45 UTC
Created attachment 321176 [details]
Drop *.la, fix License tag, update comments

Comment 4 Ondrej Vasik 2008-10-22 17:19:50 UTC
Thanks for review...
Ok with droping .la and comments, but about license I have a question - as upstream has BSD license on http://sourceforge.net/projects/openjade/ URL page. How you got the feeling that MIT is more suitable for OpenSP?

Comment 5 Ville Skyttä 2008-10-22 18:28:16 UTC
Yes, I think upstream's sf.net License category is a bit inaccurate.  Compare COPYING in the OpenSP tarball with https://fedoraproject.org/wiki/Licensing/MIT and https://fedoraproject.org/wiki/Licensing/BSD

Comment 6 Ondrej Vasik 2008-10-22 19:14:22 UTC
Ok, looks exactly as Modern Style of MIT ... therefore will use MIT, thanks for explanation. Built in Rawhide as opensp-1.5.2-8.fc10.

Comment 7 Ville Skyttä 2008-10-22 19:44:25 UTC
One remaining cosmetic thing I noticed but failed to note in previous comments:

$ rpmlint -i opensp-1.5.2-8.fc9.x86_64.rpm
opensp.x86_64: W: file-not-utf8 /usr/share/doc/opensp-1.5.2/ChangeLog
The character encoding of this file is not UTF-8.  Consider converting it in
the specfile for example using iconv(1).

Not at all a blocker but would be nice to get it fixed nevertheless.  Setting approved already, feel free to close when you've looked into it.

Comment 8 Ondrej Vasik 2008-10-23 08:57:14 UTC
Fixed in opensp-1.5.2-9.fc10, I will ignore last remaining rpmlint warning about missing doc files in opensp-devel subpackage as the documentation is in main opensp package. Closing RAWHIDE, thanks again for review.

Comment 9 Ville Skyttä 2008-10-23 14:53:01 UTC
The conversion should not be done in %build because that will result it being done multiple times and thus in corrupted result when doing multiple successive --short-circuit builds.  %prep would be the proper place to do it (I'll update rpmlint's message to reflect that).

Also, there's little reason to convert HTML files that have the correct encoding in their meta info - HTML files are opened with browsers that should grok the encoding just fine.  This is the reason why rpmlint does not complain about HTML (and some other) files' encodings even when they're not UTF-8.

Comment 10 Ondrej Vasik 2008-10-24 07:25:15 UTC
Thanks, ok, have to upgrade my rpmlint, because my version complained about ".html" file as well - and because it was created at build time, I used iconv in build section. Will move it to prep again without releasenotes.html conversion.

Note You need to log in before you can comment on or make changes to this bug.