Bug 226217 - Merge Review: opensp
Merge Review: opensp
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 15:19 EST by Nobody's working on this, feel free to take it
Modified: 2008-10-24 03:25 EDT (History)
2 users (show)

See Also:
Fixed In Version: opensp-1.5.2-9.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-23 04:57:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
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 12:36 EDT, Ville Skyttä
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:19:05 EST
Fedora Merge Review: opensp

http://cvs.fedora.redhat.com/viewcvs/devel/opensp/
Initial Owner: twaugh@redhat.com
Comment 1 Ondrej Vasik 2007-07-27 04:23:12 EDT
Package Change Request
======================
Package Name: opensp
Updated Fedora Owners: ovasik@redhat.com
Comment 2 Ville Skyttä 2008-10-22 12:36:04 EDT
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 12:36:45 EDT
Created attachment 321176 [details]
Drop *.la, fix License tag, update comments
Comment 4 Ondrej Vasik 2008-10-22 13:19:50 EDT
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 14:28:16 EDT
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 15:14:22 EDT
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 15:44:25 EDT
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 04:57:14 EDT
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 10:53:01 EDT
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 03:25:15 EDT
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.