Bug 346121
Summary: | Review Request: malaga - Programming language for modelling of language-dependent grammatical information | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ville-Pekka Vainio <vpvainio> |
Component: | Package Review | Assignee: | Mamoru TASAKA <mtasaka> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, mfabian, mtasaka, notting |
Target Milestone: | --- | Flags: | mtasaka:
fedora-review+
wtogami: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-10-29 18:50:23 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 349191, 350781 |
Description
Ville-Pekka Vainio
2007-10-22 22:20:27 UTC
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. 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? 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. 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. (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} ------------------------------------------------------ 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. (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. 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. 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. 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 The package was built successfully. Mamoru Tasaka, thank you for your review and help. I'm closing this bug now as NEXTRELEASE. 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' 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' 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. 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. 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. |