Bug 346121 - Review Request: malaga - Programming language for modelling of language-dependent grammatical information
Review Request: malaga - Programming language for modelling of language-depen...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 349191 350781
  Show dependency treegraph
 
Reported: 2007-10-22 18:20 EDT by Ville-Pekka Vainio
Modified: 2012-09-18 10:11 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-29 14:50:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ville-Pekka Vainio 2007-10-22 18:20:27 EDT
Spec URL: http://vpv.fedorapeople.org/packages/malaga.spec
SRPM URL: http://vpv.fedorapeople.org/packages/malaga-7.11-0.1.fc7.src.rpm

Description:
A software package for the development and application of
grammars that are used for the analysis of words and sentences of natural
languages. It is a language-independent system that offers a programming
language for the modelling of the language-dependent grammatical
information. This language is also called Malaga.

Malaga is based on the grammatical theory of the "Left Associative Grammar"
(LAG), developed by Roland Hausser, professor for Computational Linguistics at
University of Erlangen, Germany.


I'm not personally using this package as such, but the Finnish F/OSS spellchecker Voikko depends on Suomi-Malaga which naturally depends on Malaga, so in order to get Voikko packaged for Fedora I need to package the whole dependency chain.

I'm a new packager with only one package in Fedora before these, so I may have made some errors here, I appreciate your feedback.

There are some rpmlint issues with the packages:

malaga.src and malaga - E: summary-too-long. Hard to fix, the summary is directly from the README and I can't come up with a better one.

malaga: E: postin-without-install-info /usr/share/info/malaga.info.gz. This is probably caused by the fact that the upstream Makefile installs the info file, so I don't call install-info in the spec file. Should I?

malaga: W: empty-%postun. Is this caused by the issue above?

malaga-devel and libmalaga: W: no-documentation. All the documentation is in the main malaga package, is that OK?

Have I split the packages correctly, so that there is malaga which has all the executables, docs, man/info files etc, libmalaga which has the library files and malaga-devel? Or should the lib package be named malaga-libs? Or should the files currently in malaga and libmalaga even be in the same package?
Comment 1 Mamoru TASAKA 2007-10-24 14:44:03 EDT
Some random comments (I just glanced at your spec file,
I have not even tried to rebuild this)

- Requires between subpackages from one srpm must usually be
  version-release specific. i.e.
  Requires: lib%{name} = %{version}-%{release}, for example
- debuginfo must not be empty.
- For -devel package, -devel package requires %{name} and
  %{name} requires lib%{name}. So "Requires: lib%{name}" is not
  needed for -devel package.
- install-info must be called on %post.
  * Makefile is used only at build time and has no relation with
    the rebuilt binary rpms
- Please don't write empty scriptlet entry (why is %postun line
  needed?)
- Please check if "INSTALL.txt" is really needed. This type of files
  are usually needed for people who want to rebuild/install packages
  by themselves and are not needed for people who will install packages
  using rpm system.
- main package depends on lib%{name} package. So if
  only lib%{name} package is installed and main package is not installed,
  no documents are installed.
  i.e. all documents should be moved to lib%{name} package.
- The directory %{_datadir}/%{name} is not owned by any package.
- Please remove static archive.

(In reply to comment #0)
> malaga: E: postin-without-install-info /usr/share/info/malaga.info.gz. This is
probably caused by the fact that the upstream Makefile installs the info file,
so I don't call install-info in the spec file. Should I?
> 
> malaga: W: empty-%postun. Is this caused by the issue above?

Please see above
> 
> malaga-devel and libmalaga: W: no-documentation. All the documentation is in
the main malaga package, is that OK?
  For %{name}-devel package, no documentation warning is okay,
  however as said above, all listed documents should be moved
  into lib%{name} package.
Comment 2 Ville-Pekka Vainio 2007-10-24 16:49:58 EDT
Thank you for your feedback, I've fixed some of the issues you pointed out.

New spec: http://vpv.fedorapeople.org/packages/malaga.spec
New SRPM: http://vpv.fedorapeople.org/packages/malaga-7.11-0.2.fc7.src.rpm

(In reply to comment #1)
> - Requires between subpackages from one srpm must usually be
>   version-release specific.

Fixed.

> - debuginfo must not be empty.

NOT fixed. Upstream Makefile calls install -s, how do I remove it? Do I need to
make a patch for it, or rather Makefile.in? Sorry to use bugzilla as a
learning/discussion forum, but I couldn't get an answer from #fedora-devel either.

> - For -devel package, -devel package requires %{name} and
>   %{name} requires lib%{name}. So "Requires: lib%{name}" is not
>   needed for -devel package.

Fixed.

> - install-info must be called on %post.

Fixed.

> - Please don't write empty scriptlet entry (why is %postun line
>   needed?)

It wasn't, my error, I removed it.

> - Please check if "INSTALL.txt" is really needed.

It wasn't, so removed.


>   i.e. all documents should be moved to lib%{name} package.

Done.

> - The directory %{_datadir}/%{name} is not owned by any package.

Fixed.

> - Please remove static archive.

If you mean %{_libdir}/libmalaga.a in -devel, if I remove that, rpmbuild
complains and won't build the packages. How do I go around it then?
Comment 3 Mamoru TASAKA 2007-10-25 04:23:32 EDT
For 7.11-0.2:

(In reply to comment #2) 
> > - debuginfo must not be empty.
> 
> NOT fixed. Upstream Makefile calls install -s, how do I remove it? Do I need to
> make a patch for it, or rather Makefile.in? 
  - You have to apply a patch against Makefile.in (before configure).
    Alternative way is
-----------------------------------------------------------------
$ sed -i.strip -e 's| -s | |' Makefile.in
-----------------------------------------------------------------
    at the end of %prep.

> > - Please remove static archive.
> 
> If you mean %{_libdir}/libmalaga.a in -devel, if I remove that, rpmbuild
> complains and won't build the packages. How do I go around it then?
  - Actually remove this static archive before %install finishes
   (by "rm -f", for example) and remove this entry from %files.

This time I tried to rebuild this packge, then several
more problems are found.
* build log more verbosely
  - The output like
----------------------------------------------------------------
Compiling avl_trees.c
----------------------------------------------------------------
    is not useful.
    - For example, we want to check if fedora specific compilation
      flags are correctly honored from build.log
    Please make build procedure more verbose. To do so, please remove
    @ (at-mark)s which suppress build process output from Makefile.in.
    For example:
----------------------------------------------------------------
sed -i.debug -e 's|^\([ \t][ \t]*\)@|\1|' Makefile.in
----------------------------------------------------------------

  - Also, it is preferred that you make libtool output more verbose like:
----------------------------------------------------------------
sed -i.silent -e 's|--silent||' Makefile.in
----------------------------------------------------------------
    to check if -fPIC is passed correctly, for example.

* linkage against libmalaga.so
  - The installed binaries in malaga don't use libmalaga.so (
    is not linked against libmalaga.so).

    For example:
----------------------------------------------------------------
$ ldd -r ./malsym
        linux-gate.so.1 =>  (0x00110000)
        libglib-2.0.so.0 => /lib/libglib-2.0.so.0 (0x00ad2000)
        libc.so.6 => /lib/libc.so.6 (0x007d6000)
        /lib/ld-linux.so.2 (0x007b7000)
----------------------------------------------------------------
    build log says:
----------------------------------------------------------------
Linking malsym
gcc  avl_trees.o basic.o files.o hangul.o malaga_files.o malsym.o pools.o
scanner.o sym_compiler.o symbols.o tries.o values.o -lglib-2.0   -o malsym
----------------------------------------------------------------
    where
----------------------------------------------------------------
gcc -shared  .libs/analysis.o .libs/avl_trees.o .libs/basic.o .libs/cache.o
.libs/commands.o .libs/display.o .libs/files.o .libs/hangul.o .libs/input.o
.libs/lexicon.o .libs/malaga_files.o .libs/malaga_lib.o .libs/options.o
.libs/patterns.o .libs/pools.o .libs/processes.o .libs/rules.o .libs/scanner.o
.libs/symbols.o .libs/transmit.o .libs/tries.o .libs/value_parser.o
.libs/values.o .libs/libmalaga.o  -lglib-2.0 -lm  -Wl,-soname -Wl,libmalaga.so.7
-o .libs/libmalaga.so.7.0.0
----------------------------------------------------------------
    so almost all objects used in malsym are in libmalaga.so.

Extras issues:
- Redundant Requires:
  "Requires: gtk2" is not needed. rpmbuild checks dependencies for
   libraries and automatically adds those dependency to binary rpms.

- Timestamps
  - Add 
----------------------------------------------------------------
INSTALL="install -p"
----------------------------------------------------------------
    option to "make install" to keep timestamps on installed files.
    This method usually works for recent Makefiles.

* Permission
  - Permission of libmalaga.so must be 0755, not 0644
  ! Don't use %attr. Change the permission of file before %install
    section ends so that this libraries can be stripped by
    rpmbuild.
Comment 4 Ville-Pekka Vainio 2007-10-26 18:53:18 EDT
I've tried to fix most issues you pointed out.

New spec: http://vpv.fedorapeople.org/packages/malaga.spec
New SRPM: http://vpv.fedorapeople.org/packages/malaga-7.11-0.3.fc7.src.rpm

(In reply to comment #3)

> $ sed -i.strip -e 's| -s | |' Makefile.in

Done.
 
>   - Actually remove this static archive before %install finishes
>    (by "rm -f", for example) and remove this entry from %files.

Done.


>     Please make build procedure more verbose. To do so, please remove
>     @ (at-mark)s which suppress build process output from Makefile.in.

Done.
 
>   - Also, it is preferred that you make libtool output more verbose

Done.

> * linkage against libmalaga.so
>   - The installed binaries in malaga don't use libmalaga.so (
>     is not linked against libmalaga.so).

NOT done yet, mostly because of lack of time in the recent couple of days and
also lack of knowledge. I assume I'd have to patch Makefile.in heavily so that
the binaries would actually use libmalaga.so? Also, is this really necessary,
the binaries would then differ from those that are built from pristine upstream
sources? If this is needed, I'll try to study about it during the next few days.

>   "Requires: gtk2" is not needed.

Fixed.


> INSTALL="install -p"

Added.

>   - Permission of libmalaga.so must be 0755, not 0644
>   ! Don't use %attr. Change the permission of file before %install
>     section ends so that this libraries can be stripped by
>     rpmbuild.

Currently I've done this so that all libmalaga.so* files have permissions 0755.
Though it seems to me that the library (only one, the rest are actually symbolic
links) does get stripped by rpmbuild even if it is 0644.
Comment 5 Mamoru TASAKA 2007-10-28 04:01:40 EDT
(In reply to comment #4)

> > * linkage against libmalaga.so
> >   - The installed binaries in malaga don't use libmalaga.so (
> >     is not linked against libmalaga.so).
> 
> NOT done yet, mostly because of lack of time in the recent couple of days and
> also lack of knowledge. I assume I'd have to patch Makefile.in heavily so that
> the binaries would actually use libmalaga.so? Also, is this really necessary,
> the binaries would then differ from those that are built from pristine upstream
> sources? If this is needed, I'll try to study about it during the next few days.

- For now I can admit as it is, however please contact with upstream so
  that binaries in malaga package actually uses libmalaga.

Rest issues:
For 7.11-0.3:

* readline dependency
  - Please check if the following build.log result is
    correct.
------------------------------------------------------
   185  appending configuration tag "F77" to libtool
   186  checking for Win32... no
   187  checking for readline in -lreadline... no
   188  checking readline/readline.h usability... no
   189  checking readline/readline.h presence... no
   190  checking for readline/readline.h... no
   191  checking for GTK+ 2.0... yes
   192  checking for GLib... yes
   193  configure: creating ./config.status
------------------------------------------------------

* Dependency for -devel package
  - Well, after I reread your spec file, perhaps -devel
    package does not depend on malaga package, only depends
    on libmalaga package.
    For -devel package,
------------------------------------------------------
Requires:       %{name} = %{version}-%{release}
------------------------------------------------------
    should be changed to
------------------------------------------------------
Requires:       libmalaga = %{version}-%{release}
------------------------------------------------------
Comment 6 Ville-Pekka Vainio 2007-10-28 12:57:16 EDT
New spec: http://vpv.fedorapeople.org/packages/malaga.spec
New SRPM: http://vpv.fedorapeople.org/packages/malaga-7.11-0.4.fc7.src.rpm

(In reply to comment #5)
> - For now I can admit as it is, however please contact with upstream so
>   that binaries in malaga package actually uses libmalaga.

I'll do this. I might even try to make a patch myself, but the Fedora packaging
process will be a lot quicker if we won't stay waiting for it.

> Rest issues:
> For 7.11-0.3:
> 
> * readline dependency

CHANGES.txt mentions:
"The configure option "--with-readline" enables fancy command line editing,
provided that the GNU readline library is installed."
But also:
"When using "libmalaga", the readline library is no longer needed"

I'm not sure what to make of that, but I added the --with-readline option to
configure and also added BuildRequires readline-devel. I guess it won't hurt to
have the readline support in there.

> * Dependency for -devel package
>   - Well, after I reread your spec file, perhaps -devel
>     package does not depend on malaga package, only depends
>     on libmalaga package.
>     For -devel package,
> ------------------------------------------------------
> Requires:       %{name} = %{version}-%{release}
> ------------------------------------------------------
>     should be changed to
> ------------------------------------------------------
> Requires:       libmalaga = %{version}-%{release}
> ------------------------------------------------------

I tried this, but rpmlint said it was an error to not have Requires malaga in
-devel, so I left it as it was.
Comment 7 Mamoru TASAKA 2007-10-28 13:03:48 EDT
(In reply to comment #6)
> > * Dependency for -devel package
> >   - Well, after I reread your spec file, perhaps -devel
> >     package does not depend on malaga package, only depends
> >     on libmalaga package.
> >     For -devel package,
> > ------------------------------------------------------
> > Requires:       %{name} = %{version}-%{release}
> > ------------------------------------------------------
> >     should be changed to
> > ------------------------------------------------------
> > Requires:       libmalaga = %{version}-%{release}
> > ------------------------------------------------------
> 
> I tried this, but rpmlint said it was an error to not have Requires malaga in
> -devel, so I left it as it was.

Really? I guess it is "warning", not "error". If it is really
"error", it is a bug in rpmlint.
Comment 8 Ville-Pekka Vainio 2007-10-28 13:29:01 EDT
New spec: http://vpv.fedorapeople.org/packages/malaga.spec
New SRPM: http://vpv.fedorapeople.org/packages/malaga-7.11-0.5.fc7.src.rpm

(In reply to comment #7)
> Really? I guess it is "warning", not "error". If it is really
> "error", it is a bug in rpmlint.

You're correct, it was just a warning, so I did as you advised, -devel requires
only libmalaga now.

Comment 9 Mamoru TASAKA 2007-10-28 13:56:59 EDT
Okay.

-----------------------------------------------------------
  This package (malaga) is APPROVED by me
-----------------------------------------------------------

Note:
As written in
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure :
now F-8 branch is also valid.
Comment 10 Ville-Pekka Vainio 2007-10-28 18:33:25 EDT
New Package CVS Request
=======================
Package Name: malaga
Short Description: A programming language for automatic language analysis
Owners: vpv
Branches: F-7 F-8
Cvsextras Commits: yes
Comment 11 Ville-Pekka Vainio 2007-10-29 14:50:23 EDT
The package was built successfully. Mamoru Tasaka, thank you for your review and
help. I'm closing this bug now as NEXTRELEASE.
Comment 12 Fedora Update System 2007-11-06 11:03:07 EST
malaga-7.11-1.fc8 has been pushed to the Fedora 8 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update malaga'
Comment 13 Fedora Update System 2007-11-09 18:43:31 EST
malaga-7.11-1.fc7 has been pushed to the Fedora 7 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update malaga'
Comment 14 Fedora Update System 2007-11-15 19:35:39 EST
malaga-7.11-1.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 15 Fedora Update System 2007-11-15 19:40:36 EST
malaga-7.11-1.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 16 Ville-Pekka Vainio 2008-02-24 13:17:29 EST
I have finally patched the Makefile.in of this package so that the executables
are linked against the library, this is something Mamoru Tasaka pointed out in
the review. The patch is in malaga-7.11-3.fc9, I'll send it upstream if it
doesn't seem to cause any bugs.

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