Bug 597597

Summary: Review Request: liblouis - A Braille translator and back-translator
Product: [Fedora] Fedora Reporter: Lars Bjorndal <lars>
Component: Package ReviewAssignee: Martin Gieseking <martin.gieseking>
Status: CLOSED DUPLICATE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, martin.gieseking, notting, opensource, supercyper1
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-02-16 05:33:06 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 615783    

Description Lars Bjorndal 2010-05-29 14:06:30 EDT
Spec URL: http://lamasti.net/filer/liblouis.spec
SRPM URL: http://lamasti.net/filer/liblouis-1.8.0-1.fc12.src.rpm
Description: Liblouis is an open-source braille translator and
back-translator based on the translation routines in the BRLTTY
screenreader for Linux. It has, however, gonefar beyond these
routines. It is named in honor of Louis Braille. In Linux and Mac OSX
it is a shared library, and in Windows it is a DLL.
Comment 1 Martin Gieseking 2010-06-14 11:53:56 EDT
Hi Lars,

here are a couple of initial things that need to be fixed:

- remove the last sentence from the %description, as you're creating a binary package for Linux (so other OSes don't matter here)

- Normally, you should call make with %{?_smp_mflags}. If the package doesn't build with it, remove it and add a comment about the issue.

- .la files must not be packaged (remove them in %install and drop the corresponding line in %files)

- add at least one newline between the various sections (here: %post, %postun, %files, %changelog, ...) to increase legibility

- replace /usr/share/liblouis/* with %{_datadir}/liblouis/ and drop the %dir line

- Always try to be specific when adding something in %files in order to avoid packaging unwanted files or directories. Thus, replace %{_includedir}/* with %{_includedir}/liblouis/

- for the same reason, replace 
  %{_mandir}/man1/* with %{_mandir}/man1/lou_*.1*

- replace %{_infodir}/liblouis.info.gz with %{_infodir}/liblouis.info*
because the spec file shouldn't rely on a specific, automatically applied compression format (which could change in future rpm releases)

- add file COPYING.LIB as %doc of the base package

- The base package should only contain the library and its data files, since it's licensed under LGPLv3+. The command-line tools in %{_bindir} are licensed under GPLv3+, so they should go in a separate -tools subpackage together with file COPYING and the man1 manual pages.

- drop .f12 from the %changelog entry

That's all for now. :)
Comment 2 Lars Bjorndal 2010-06-19 15:43:33 EDT
Thank you very much for your useful information!

The spec file found at <http://lamasti.net/filer/liblouis.spec> is now
updated and should reflect the issues you mentioned. The release is
increased, and accordingly the sources rpm is at
<http://lamasti.net/filer/liblouis-1.8.0-2.fc12.src.rpm>.

While running rpmlint on the rpms, I get several warnings like this:

  liblouis-tools.i686: W: unstripped-binary-or-object
  /usr/bin/lou_debug

However, building under mock, I don't get this problem. Why is it so?

Lars
Comment 3 Martin Gieseking 2010-06-20 03:05:45 EDT
(In reply to comment #2)
> While running rpmlint on the rpms, I get several warnings like this:
> 
>   liblouis-tools.i686: W: unstripped-binary-or-object
>   /usr/bin/lou_debug

I don't get these warnings. The debuginfo is only stripped if the executable flags of the binary files are set. Maybe you use an uncommon umask that disables the flags. On my system it's 0002.


Here are some more things you should address:

- rpmlint complains about the mixed use of tabs and spaces. Use either spaces or tabs for indentation, don't mix them.

- replace rm -f $RPM_BUILD_ROOT/usr/lib/liblouis.la
  with rm -f $RPM_BUILD_ROOT/%{_libdir}/liblouis.la
  otherwise your package doesn't build on 64bit archs because libraries go to /usr/lib64 there

- I suggest to also use a macro in the preceding line:
  rm -f $RPM_BUILD_ROOT/%{_infodir}/dir

- add AUTHORS, NEWS and ChangeLog as %doc to the base package

- documentation files should go to %{_defaultdocdir}/%{name}-%{version}/ but the .html and .txt file are placed in %{_defaultdocdir}/%{name}/.
I suggest to remove the doc directory in %install and manually add the files as %doc to the base package: %doc doc/liblouis.txt doc/liblouis.html

- add file COPYING as %doc to the -tools package

- adapt the %changelog headers according to
  https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
  + add your name
  + fix the release number of the latest entry
  + drop the dist tag .f12
  Example:
  * Thu Jun 17 2010 Lars Bjorndal <lars.bjorndal@broadpark.no> 1.8.0-2

- add an empty line between the changlog entry sets to make them easier to recognize
Comment 4 Chen Lei 2010-06-20 22:54:24 EDT
(In reply to comment #2)
> Thank you very much for your useful information!
> 
> The spec file found at <http://lamasti.net/filer/liblouis.spec> is now
> updated and should reflect the issues you mentioned. The release is
> increased, and accordingly the sources rpm is at
> <http://lamasti.net/filer/liblouis-1.8.0-2.fc12.src.rpm>.
> 
> While running rpmlint on the rpms, I get several warnings like this:
> 
>   liblouis-tools.i686: W: unstripped-binary-or-object
>   /usr/bin/lou_debug
> 
> However, building under mock, I don't get this problem. Why is it so?
> 
> Lars    

Please install redhat-rpm-config/fedora-packager first.
Comment 5 Lars Bjorndal 2010-06-21 15:29:23 EDT
In reply to Martin Gieseking <martin.gieseking@uos.de>

Please find an updated version of the spec file here:
<http://lamasti.net/filer/liblouis.spec>. The new src.rpm is at:
<http://lamasti.net/filer/liblouis-1.8.0-3.fc12.src.rpm>

You wrote:
[...]
> - adapt the %changelog headers according to
>   https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs + add
>   your name + fix the release number of the latest entry + drop the
>   dist tag .f12 Example: * Thu Jun 17 2010 Lars Bjorndal
>   <lars.bjorndal@broadpark.no> 1.8.0-2

Actually my surname is not Bjorndal, but Bjørndal. Is it ok to insert
the letter ø (&oslash;) into this field? Does it require Unicode, or
is it ok to use iso8859-1?

Lars
Comment 6 Martin Gieseking 2010-06-22 01:34:28 EDT
(In reply to comment #5)
> Actually my surname is not Bjorndal, but Bjørndal. Is it ok to insert
> the letter ø (&oslash;) into this field? Does it require Unicode, or
> is it ok to use iso8859-1?

Yes, it's of course OK to insert your name with correct spelling. In this case you should use UTF-8 encoding.
Comment 7 Martin Gieseking 2010-06-22 02:00:46 EDT
Your spec file looks much better now. As suggested in comment #3, you can completely remove the installed documentation, e.g. with
  rm -rf $RPM_BUILD_ROOT/%{_defaultdocdir}/%{name}/

The files added with %doc doc/liblouis.* are directly taken from the doc directory of the uncompressed tarball. 

Since I'm not a sponsor, I can't do the formal review of this package. In order to get sponsored, you usually need to provide some more packages and comment on several review requests of other packagers to show that you're familiar with the packaging guidelines.
Comment 8 Till Maas 2010-06-30 10:04:27 EDT
(In reply to comment #7)

> Since I'm not a sponsor, I can't do the formal review of this package. In order
> to get sponsored, you usually need to provide some more packages and comment on
> several review requests of other packagers to show that you're familiar with
> the packaging guidelines.    

I am a sponsor and I agree here. The additional packages are not as important as so called unofficial reviews where you comment on other package reviews.
Comment 9 Lars Bjorndal 2010-06-30 15:12:13 EDT
In reply to comment #7 from Martin Gieseking:

> Your spec file looks much better now. As suggested in comment #3,
> you can completely remove the installed documentation, e.g. with rm
> -rf $RPM_BUILD_ROOT/%{_defaultdocdir}/%{name}/

Thank you. Now, I understand.. The spec file is at:
<http://lamasti.net/filer/liblouis.spec>. The src.rpm is at:
<http://lamasti.net/filer/liblouis-1.9.0-1.fc12.src.rpm>.

To day, there was a new version up too, and I used that, of course.

> Since I'm not a sponsor, I can't do the formal review of this
> package. In order to get sponsored, you usually need to provide some
> more packages and comment on several review requests of other
> packagers to show that you're familiar with the packaging
> guidelines.

Thank you for your helpful tips. I'll try to follow your suggestion.
Comment 10 Martin Gieseking 2010-07-05 03:28:20 EDT
Hi Lars,

%doc doc/liblouis.* also adds the unwanted files liblouis.info and liblouis.texi. To avoid this, explicitly list the intended files, e.g. with
%doc doc/liblouis.{html,txt}

Since the tarball contains a texinfo file, you could create a pdf file from it and add it as a further documentation variant. To do so, simply add "BuildRequires: texinfo-tex", call "make -C doc liblouis.pdf" in %build, and append "pdf" to the above %doc suffix list.
Comment 11 Lars Bjorndal 2010-07-06 05:29:53 EDT
Thank you, Martin! I've followd your suggestions. The spec file is at:
<http://lamasti.net/filer/liblouis.spec>, and the src.rpm at:
<http://lamasti.net/filer/liblouis-1.9.0-2.fc12.src.rpm>.

I will soon follow up with a new package, liblouisxml, for another
review request.

Again, thank you!

Lars
Comment 12 Martin Gieseking 2010-07-31 01:55:00 EDT
Hi Lars,

I've recently been promoted to sponsor status, and if you're still interested in becoming a member of the packager group, I'm willing to sponsor you.

After you've been sponsored you're allowed to formally review and approve new packages submitted by other packagers. Hence, it's important that you're familiar with the packaging guidelines. To acquaint yourself with the review process and to show your (basic) understanding of the guidelines, you should do some informal reviews in the next couple of days. I'll contact you by email for further details.

Since you've already submitted liblouisxml, it's not necessary to me to provide another package. However, if you'd like to make another submission, I would do the review.
Comment 13 Martin Gieseking 2011-02-16 05:33:06 EST
Closing this bug in agreement with Lars.

*** This bug has been marked as a duplicate of bug 677943 ***