Bug 822329 - Review Request: libzen - Shared library for libmediainfo and medianfo
Review Request: libzen - Shared library for libmediainfo and medianfo
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Christopher Meng
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 822327 822328
  Show dependency treegraph
 
Reported: 2012-05-17 01:22 EDT by Vasiliy Glazov
Modified: 2013-08-31 15:19 EDT (History)
7 users (show)

See Also:
Fixed In Version: libzen-0.4.29-2.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-20 20:04:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
i: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Vasiliy Glazov 2012-05-17 01:22:32 EDT
Spec URL: https://github.com/RussianFedora/libzen/blob/master/libzen.spec
SRPM URL: http://koji.russianfedora.ru/packages/libzen/0.4.26/1.fc17.R/src/libzen-0.4.26-1.fc17.R.src.rpm
Description: Files shared library for libmediainfo and medianfo

It needed for https://bugzilla.redhat.com/show_bug.cgi?id=822327 and https://bugzilla.redhat.com/show_bug.cgi?id=822328

I need a sponsor for include this package in Fedora.
Comment 1 Ralf Corsepius 2012-05-17 01:44:11 EDT
FYI: You may want to compare your packages against those I ship through packman:
http://ftp.gwdg.de/pub/linux/misc/packman/fedora/
Comment 2 Vasiliy Glazov 2012-05-17 02:01:34 EDT
Ralf Corsepius, I don't see sufficient difference.
But why you choose License:        LGPLv3
I think this wrong. http://sourceforge.net/p/zenlib/code/397/tree/ZenLib/trunk/License.txt
Comment 3 Ralf Corsepius 2012-05-17 02:24:18 EDT
Then you will want to look again - There are plenty of differences, primarily related to building the package, because upstream's configuration is a dysfunctional mess ;)

Some remarks for now:

* Do not ship static libs (*.a)

* Do not ship libtool *.la's

* Last time I checked, the upstream's libzen-config was dysfunctional
I added a patch to my package to work around this.

* Wrt. the license: The package contains an LGPL'ed file:
# grep License Source/ZenLib/*
Source/ZenLib/PreComp.h:// under the terms of the GNU Lesser General Public License as published by
Source/ZenLib/PreComp.h:// the Free Software Foundation, either version 3 of the License, or
Source/ZenLib/PreComp.h:// GNU Lesser General Public License for more details.
Source/ZenLib/PreComp.h:// You should have received a copy of the GNU Lesser General Public License

=> This file's license outrules the license from License.txt and renders this package as a whole LGPL'ed.
Comment 5 Ralf Corsepius 2012-05-18 00:44:42 EDT
(In reply to comment #4)
> I don't see libzen-config in package from mediainfo at all 
> http://mediainfo.sourceforge.net/en/Download/Fedora
> http://downloads.sourceforge.net/zenlib/libzen0-0.4.26-1.i386.Fedora_16.rpm

As I said before, upstream's configuration is pretty messy ... and so is their packaging.

They are shipping libzen-config inside of their sources, but miss to install it in their rpms:
# tar tjvf libzen_0.4.26.tar.bz2 | grep config
-rwxrwxrwx 0/0            9714 2012-04-08 14:50 ZenLib/Project/GNU/Library/configure.ac
-rwxrwxrwx 0/0             340 2011-10-13 09:50 ZenLib/Project/GNU/Library/libzen-config.in
Comment 6 Vasiliy Glazov 2012-05-18 03:08:27 EDT
Corrected SPEC: https://github.com/RussianFedora/libzen/blob/master/libzen.spec
Corrected SRPM:
http://koji.russianfedora.ru/packages/libzen/0.4.26/3.fc17.R/src/libzen-0.4.26-3.fc17.R.src.rpm

I add you patch and libzen-config to devel.
Now all OK?
Comment 7 Jérôme Martinez 2012-05-18 11:32:21 EDT
(In reply to comment #3)
> Then you will want to look again - There are plenty of differences, primarily
> related to building the package, because upstream's configuration is a
> dysfunctional mess ;)

As the original developer of the spec file, I agree ;-).
It was made quickly for my own needs (different configs, different distros), without a lot of knowledge about RPM, and it is definitely ugly.

> * Wrt. the license: The package contains an LGPL'ed file
> => This file's license outrules the license from License.txt and renders this
> package as a whole LGPL'ed.

Corrected in the SVN:
https://sourceforge.net/p/zenlib/code/398/
The intended license is definitely zlib license for this package.

(In reply to comment #5)
> They are shipping libzen-config inside of their sources, but miss to install it

Error from me. It is expected to be installed for people not using pkg-config.
I corrected it in the SVN:
https://sourceforge.net/p/zenlib/code/399/

Jérôme, developer of mediainfo/libmediainfo and the tiny ugly libzen library used by mediainfo/libmediainfo.
Comment 9 Nikita Klimov 2013-04-06 14:58:48 EDT
Hi Vasiliy

- libzen should be released with zlib/libpng license. Why are you using LGPLv3 in spec file? 

- Please add short informative comments in %{build} and %{install} section

- " The package must be consistent. For any given path, within the same spec, use either a hard-coded path or a macro, not a combination of the two. "
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#macros

A good solution will use %{name} macro where you can.
 
- In my opinion it's not necessary in spec file:

 42 # Fix up Makefile.am
 43 cat << EOF >> Project/GNU/Library/Makefile.am
 44 
 45 bin_SCRIPTS = libzen-config
 46 
 47 pkgconfigdir = \$(libdir)/pkgconfig
 48 pkgconfig_DATA = libzen.pc
 49 
 50 EOF

Upstream file also included this lines, and you are simply doubled it.

- Please remove Russian Fedora tags in %{changelog} section.
Comment 10 Vasiliy Glazov 2013-04-08 02:38:50 EDT
Spec URL: https://raw.github.com/RussianFedora/libzen/master/libzen.spec
SRPM URL: http://koji.russianfedora.ru/koji/getfile?taskID=70536&name=libzen-0.4.28-2.fc19.R.src.rpm

Almost all corrected.
I correct %{changelog} when package will be included in Fedora.
Comment 11 Nikita Klimov 2013-04-08 17:49:53 EDT
- "Macro forms of system executables SHOULD NOT be used except when there is a need to allow the location of those executables to be configurable. For example, rm should be used in preference to %{__rm}, but %{__python} is acceptable. "
https://fedoraproject.org/wiki/Packaging:Guidelines#Macros

- Use %{buildroot} and %{optflags} or $RPM_BUILD_ROOT and $RPM_OPT_FLAGS but not %{buildroot} and $RPM_OPT_FLAGS or $RPM_BUILD_ROOT and %{optflags}
https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

In my opinion %{buildroot} and %{optflags} a little more beautiful but ypu can use $RPM_BUILD_ROOT and $RPM_OPT_FLAGS if you prefer it.

- Take a look at line 63 in spec file:

63     %__chmod +x autogen

There is no need, since executable bit also set.

- Take a look at line 64:

64     ./autogen

There is also no need, simply use autoconf.
Comment 12 Vasiliy Glazov 2013-04-09 04:55:20 EDT
I'll remove %__chmod +x autogen. But simple autoconf not work.
I think ./autogen is needed.
Comment 13 Nikita Klimov 2013-04-09 05:23:02 EDT
(In reply to comment #12)
> I'll remove %__chmod +x autogen. But simple autoconf not work.
> I think ./autogen is needed.

Oh, sorry. You are right :) Please correct other remarks and update links to spec file and source RPM. I'll do a detailed review tomorrow.
Comment 15 Nikita Klimov 2013-04-10 14:12:57 EDT
Unfortunately, there are a number of comments:

- In License: section replace 'zlib/libpng License' to 'zlib/libpng'

- dos2unix     *.txt Source/Doc/*.html -> dos2unix     *.txt Source/Doc/Documentation.html (wildcard in my opinion not need)

- Please don't ignore any remark:
"Macro forms of system executables SHOULD NOT be used except when there is a need to allow the location of those executables to be configurable. For example, rm should be used in preference to %{__rm}, but %{__python} is acceptable. "
https://fedoraproject.org/wiki/Packaging:Guidelines#Macros

For example:
%__chmod -> chmod

If you disagree with any remark, explain your point of view, but don't ignore it.

- Doxygen's tags 'DETAILS_AT_TOP' and 'HTML_ALIGN_MEMBERS' has become obsolete, please contact upstream. Temporary you can use:

doxygen -u

before generate documentation.

- Do more informative comments in %{changelog} section. For example 'Clean spec' and 'Spec prepared for review' not informative.
"This is important not only to have an idea about the history of a package, but also to enable users, fellow packages, and QA people to easily spot the changes that you make."
http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

- Please add short informative comments to %{prep}, %{build} and %{install} section before any 'nonstandart' tasks, that explains why this task necessary.

- I know that you are also maintainer in RFRemix project, but in this stage it doesn't matter. We have to prepare the package for inclusion in Fedora, not RFRemix. And so the package must comply with Fedora rules. Please remove RFRemix tags in %{changelog} section. 

- Do you read this https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Convincing_someone_to_sponsor_you ? Please try to doing informal reviews to other packagers in order to find and convincing a willing sponsor. I can help fix errors and omissions in package, but can't be a sponsor, and so can't do formal review, because review and approval for the first package for new packagers must be done by registered sponsors.
Comment 16 Vasiliy Glazov 2013-04-11 06:29:30 EDT
Spec URL: https://raw.github.com/RussianFedora/libzen/master/libzen.spec
SRPM URL: http://koji.russianfedora.ru/koji/getfile?taskID=71133&name=libzen-0.4.28-4.fc19.R.src.rpm

1. License corrected.
2. Wildcard removed.
3. Done for all except %configure, because it not just ./configure. Right?
4. You mean that I should inform upstream about obsoleted tags 'DETAILS_AT_TOP' and 'HTML_ALIGN_MEMBERS'?
Added command
doxygen -u Doxyfile
5. I am not sure about nonstandard commands, because I copied it from original spec http://sourceforge.net/p/zenlib/code/HEAD/tree/ZenLib/trunk/Project/GNU/libzen.spec
6. Removed RFRemix tags from %changelog.
7. Yes I read it, and I almost get sponsor here https://bugzilla.redhat.com/show_bug.cgi?id=821404 but he lost :(
I already made some review.
Comment 17 Jérôme Martinez 2013-04-11 06:47:41 EDT
> 4. You mean that I should inform upstream about obsoleted tags 'DETAILS_AT_TOP' and 'HTML_ALIGN_MEMBERS'?

I will update my doxygen file soon with an up to date version.

> 5. I am not sure about nonstandard commands, because I copied it from original spec 

Actually, I, the upstream developper, am not expert of Makefile/spec files, any help would be appreciated to to make it "standard" (e.g. in Makefile rather than in spec file)
Comment 18 Vasiliy Glazov 2013-04-11 06:58:03 EDT
Jérôme, very glad for your respond.

Please do next:
1. Correct file encoding and remove executable bits from documentation in source package:
dos2unix *.txt Source/Doc/Documentation.html
chmod 644 *.txt Source/Doc/Documentation.html
2. Make install headers and development files, than i remove this from spec:
# Zenlib headers and ZenLib-config
install -dm 755 %{buildroot}%{_includedir}/ZenLib
install -m 644 Source/ZenLib/*.h \
    %{buildroot}%{_includedir}/ZenLib
for i in HTTP_Client Format/Html Format/Http; do
    install -dm 755 %{buildroot}%{_includedir}/ZenLib/$i
    install -m 644 Source/ZenLib/$i/*.h \
        %{buildroot}%{_includedir}/ZenLib/$i
done

sed -i -e 's|Version: |Version: %{version}|g' \
    Project/GNU/Library/%{name}.pc
install -dm 755 %{buildroot}%{_libdir}/pkgconfig
install -m 644 Project/GNU/Library/%{name}.pc \
    %{buildroot}%{_libdir}/pkgconfig
3. I think you can remove *.la and *.a files automatically if shared compile enabled.

Normal spec - if sections %prep %build %install contain 1-2 commands.
Comment 19 Jérôme Martinez 2013-04-11 07:12:51 EDT
> 1. Correct file encoding and remove executable bits from documentation in source package:

Source package is created from a Windows machine so executable bit is set by default everywhere during tar.bz2 creation, and it is intended for different platforms, so documentation has \r\n.
I don't plan to force my source files to Linux line ending.
But it is maybe possible to put dos2unix and chmod in the Makefile if there is a way to detect the platform ("if not Windows or Mac, run dos2unix")

> 2. Make install headers and development files, than i remove this from spec:

I am a total newbee in Makefile, I am aware it should be in Makefile, but I currently don't know how to do it. Sorry, but I currently lack of time to learn this, any patch for Makefile would be appreciated.
Comment 20 Vasiliy Glazov 2013-04-11 07:15:55 EDT
I think now we can leave it as is. Because all installed and work very good.
Comment 21 Nikita Klimov 2013-04-11 15:26:19 EDT
Hi Vasiliy,
- we must correct my mistake with license. Valid license short name for zlib/libpng license is 'zlib' https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Good_Licenses . Please correct it.

- If you want to be maintainer this package in Fedora, you must be extremely familiar with its contents. "Software components included in Fedora needs to be maintained actively and bugs, especially security issues needs to be fixed in a timely manner. As a Fedora package maintainer, it is your primary responsibility to ensure this." 
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Understand_your_responsibilities
Please do all steps in spec file by hand, not using RPM system, and comment all tasks.

- Rebuild SRC RPM with new spec file and give direct link to it. 

Almost all other, in my opinion is fine, except comments in %{changelog} section, but it no so necessary in this stage.
Comment 22 Jérôme Martinez 2013-04-11 15:35:16 EDT
> - If you want to be maintainer this package in Fedora, you must be extremely familiar with its contents.

FYI, the maintainer of Fedora package will have full bugs and security support from upstream, MediaInfo (the tool which needs this package) is maintained for 10 years  and security is very important for the company behind it, security issues will be fixed in a timely manner directly from upstream, the maintainer will only have to update the package.
Comment 23 Vasiliy Glazov 2013-04-12 03:21:51 EDT
Spec URL: https://raw.github.com/RussianFedora/libzen/master/libzen.spec
SRPM URL: http://koji.russianfedora.ru/koji/getfile?taskID=71294&name=libzen-0.4.28-5.fc19.R.src.rpm

All done. Old %changelog entries will be removed later.
All links direct. Or you want link without "?" and "&"?
Comment 24 Nikita Klimov 2013-04-14 16:18:53 EDT
Hi Vasiliy,
This is my informal review:

Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that are
  listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: gcc-c++
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
- Large documentation must go in a -doc subpackage.
  Note: Documentation size is 4177920 bytes in 290 files.
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[-]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[!]: Useful -debuginfo package or justification otherwise.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[!]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[!]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Arch-ed rpms have a total of 4259840 bytes in /usr/share 4239360
     libzen-devel-0.4.28-5.fc18.x86_64.rpm 20480
     libzen-0.4.28-5.fc18.x86_64.rpm
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).


Rpmlint
-------
Checking: libzen-0.4.28-5.fc18.x86_64.rpm
          libzen-devel-0.4.28-5.fc18.x86_64.rpm
libzen.x86_64: W: spelling-error Summary(en_US) libmediainfo -> infomercial
libzen.x86_64: W: spelling-error Summary(en_US) medianfo -> median
libzen.x86_64: W: spelling-error Summary(ru) libmediainfo 
libzen.x86_64: W: spelling-error Summary(ru) and 
libzen.x86_64: W: spelling-error Summary(ru) medianfo 
libzen.x86_64: W: spelling-error %description -l en_US libmediainfo -> infomercial
libzen.x86_64: W: spelling-error %description -l en_US medianfo -> median
libzen.x86_64: W: spelling-error %description -l ru libmediainfo 
libzen.x86_64: W: spelling-error %description -l ru and 
libzen.x86_64: W: spelling-error %description -l ru medianfo 
libzen-devel.x86_64: E: script-without-shebang /usr/bin/libzen-config
libzen-devel.x86_64: W: no-manual-page-for-binary libzen-config
2 packages and 0 specfiles checked; 1 errors, 11 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint libzen-devel libzen
libzen-devel.x86_64: I: enchant-dictionary-not-found ru
libzen-devel.x86_64: W: invalid-license zlib/libpng
libzen-devel.x86_64: E: script-without-shebang /usr/bin/libzen-config
libzen-devel.x86_64: W: no-manual-page-for-binary libzen-config
libzen.x86_64: W: spelling-error Summary(en_US) libmediainfo -> infomercial
libzen.x86_64: W: spelling-error Summary(en_US) medianfo -> median
libzen.x86_64: W: spelling-error %description -l en_US libmediainfo -> infomercial
libzen.x86_64: W: spelling-error %description -l en_US medianfo -> median
libzen.x86_64: W: invalid-license zlib/libpng
2 packages and 0 specfiles checked; 1 errors, 7 warnings.
# echo 'rpmlint-done:'



Requires
--------
libzen-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    libzen(x86-64)
    libzen.so.0()(64bit)

libzen (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_4.0.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    rtld(GNU_HASH)



Provides
--------
libzen-devel:
    libzen-devel
    libzen-devel(x86-64)
    pkgconfig(libzen)

libzen:
    libzen
    libzen(x86-64)
    libzen.so.0()(64bit)



MD5-sum check
-------------
http://downloads.sourceforge.net/zenlib/libzen_0.4.28.tar.bz2 :
  CHECKSUM(SHA256) this package     : 0649c10e1657f5a0c5447301c0c3c367343069159b3bb5452ae7ef344dc63805
  CHECKSUM(SHA256) upstream package : 0649c10e1657f5a0c5447301c0c3c367343069159b3bb5452ae7ef344dc63805


Please read http://fedoraproject.org/wiki/Packaging:Debuginfo about debuginfo packaging.
Comment 25 Vasiliy Glazov 2013-04-15 01:24:46 EDT
Spec URL: https://raw.github.com/RussianFedora/libzen/master/libzen.spec
SRPM URL: http://koji.russianfedora.ru/koji/getfile?taskID=71522&name=libzen-0.4.28-6.fc19.R.src.rpm

Created doc subpackage.
Disabled debuginfo subpackage generation.
Removed gcc-c++ from BR.

Jérôme, can you add shebang to /usr/bin/libzen-config? 
It mean add 
#!/bin/bash 
to first line.
Comment 26 Ralf Corsepius 2013-04-15 01:34:33 EDT
(In reply to comment #25)

> Disabled debuginfo subpackage generation.
Why? I can't imagine any valid reason, why debuginfos should be disabled.
Comment 27 Vasiliy Glazov 2013-04-15 01:35:47 EDT
Because when it created it empty.
Comment 28 Eduardo Echeverria 2013-04-15 01:44:59 EDT
Hi Vasily, please read http://fedoraproject.org/wiki/Packaging:Debuginfo#Useless_or_incomplete_debuginfo_packages_due_to_packaging_issues

Btw, you thought use autoreconf instead of . /autogen?
Comment 29 Eduardo Echeverria 2013-04-15 01:57:39 EDT
other comments:
- Why use dosunix, if you can do the same with thirst?.
- Please do not use wildcards to install the files that are in directories, it is implied that you are installing the entire directory and not only part of the files that are contained in the same
Comment 30 Eduardo Echeverria 2013-04-15 01:59:16 EDT
I'm sorry, I meant "sed", not thirst
Comment 31 Ralf Corsepius 2013-04-15 02:14:15 EDT
(In reply to comment #27)
> Because when it created it empty.

Then fix it. This is an arched package, so debuginfo files are mandatory.
In other words something is defective and you (the prospective packager) are supposed to fix it.

In case wasn't clear enough: This is a MUSTFIX.
Comment 32 Vasiliy Glazov 2013-04-15 02:26:14 EDT
OK. I change make install-strip to make install. And create debuginfo subpackage. Fixed.

Some wildcards in %files section removed.

dos2unix removed. Instead it used sed.

./autogen - is upstream authors script
$ cat rpmbuild/BUILD/ZenLib/Project/GNU/Library/autogen 

#libtoolize 
if test "$(uname)" = "Darwin" ; then
  #Darwin based Systems like Mac OS X: libtoolize is called glibtoolize.
  glibtoolize --automake
else
  libtoolize --automake
fi

#aclocal
if test -e /usr/bin/aclocal-1.11 ; then
  #OpenSolaris: no aclocal
  aclocal-1.11
elif test -e /usr/bin/aclocal-1.10 ; then
  aclocal-1.10
else
  aclocal
fi

#automake
if test -e /usr/bin/automake-1.11 ; then
  #OpenSolaris: no automake
  automake-1.11 -a
elif test -e /usr/bin/automake-1.10 ; then
  automake-1.10 -a
else
  automake -a
fi

autoconf
Comment 33 Ralf Corsepius 2013-04-15 02:27:24 EDT
(In reply to comment #31)
> (In reply to comment #27)
> > Because when it created it empty.
> 
> Then fix it. This is an arched package, so debuginfo files are mandatory.
> In other words something is defective and you (the prospective packager) are
> supposed to fix it.

This is the cause:
%install
pushd Project/GNU/Library
    make install-strip DESTDIR=%{buildroot}
popd
Invoke "make install" instead of "make install-strip".
(Background: executables must not be stripped for debuginfo generateion.
Rpm automatically strips them later.

Finally there are more questionable items in your spec:
* autogen is supposed to be run in %prep (not in %build)

* you are passing --disable-static to %configure
=> find %{buildroot} -name '*.a' -exec rm -f {} ';'
is superfluous.

* Many source files carry bogus permissions. chmod -x them in %
A visible symptom is rpmlint complaining about files in *debuginfo:
... 
libzen-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ZenLib/Source/ZenLib/Format/Http/Http_Cookies.cpp
libzen-debuginfo.x86_64: E: wrong-script-end-of-line-encoding /usr/src/debug/ZenLib/Source/ZenLib/Format/Http/Http_Cookies.cpp
libzen-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ZenLib/Source/ZenLib/ZtringListListF.h
...

* Another MUSTFIX: 
libzen-devel.x86_64: E: script-without-shebang /usr/bin/libzen-config
Comment 34 Ralf Corsepius 2013-04-15 02:28:53 EDT
(In reply to comment #32)
> OK. I change make install-strip to make install. And create debuginfo
> subpackage. Fixed.
> 
> Some wildcards in %files section removed.
> 
> dos2unix removed. Instead it used sed.
> 
> ./autogen - is upstream authors script
Forget about it. It's crap. 

Use autoreconf or switch to using patches
Comment 35 Vasiliy Glazov 2013-04-15 03:23:57 EDT
Corrected SPEC: https://raw.github.com/RussianFedora/libzen/master/libzen.spec
Corrected SRPM:
http://koji.russianfedora.ru/koji/getfile?taskID=71532&name=libzen-0.4.28-6.fc19.R.src.rpm

Removed find %{buildroot} -name '*.a' -exec rm -f {} ';
And corrected others. Added shebang. Moved ./autogen to %prep. All spurious-executable-perm and wrong-script-end-of-line-encoding corrected.

But I am not sure what i must write instead of ./autogen, because if I use autoreconf it causes errors and can't compile. Why we can't use ./autogen?
Comment 37 Jérôme Martinez 2013-04-15 03:43:11 EDT
(In reply to comment #35)
> But I am not sure what i must write instead of ./autogen, because if I use
> autoreconf it causes errors and can't compile.

I think it is OK with:
autoreconf -i
Comment 39 Nikita Klimov 2013-04-16 07:48:22 EDT
Hi Vasiliy,
- "Every time you make changes, that is, whenever you increment the E-V-R of a package, add a changelog entry. This is important not only to have an idea about the history of a package, but also to enable users, fellow packages, and QA people to easily spot the changes that you make."
http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

Please don't forgot about it.
Comment 40 Nikita Klimov 2013-04-20 16:22:39 EDT
Hi Vasiliy,
Are you still interested in adding mediainfo and related packages in fedora? If so, I suggest to work closely on correcting existing errors. After the packages are ready, I think you will find a sponsor. Please, will increase the version of the libzen package and add a comment to %{changelog} section. Also, please fix errors that were identified in this review, in packages libmedianfo and mediainfo.
Comment 41 Vasiliy Glazov 2013-04-22 01:45:51 EDT
OK, it will be done soon.
Comment 43 Christopher Meng 2013-08-01 21:02:18 EDT
Seems this package is messy.

Issues:

1. Please fix changelog contains ".R" stuffs of RussianFedora, I don't want to see again of it.

2. Please preserve the timestamps by install -p.

3. Ralf is not correct, you should use find to find the la files and -delete them, or just rm, do NOT ship any libtool files.

4. Please notify upstream about pkgconfig sed fix.

Once fixed, this will get approved.
Comment 44 Ralf Corsepius 2013-08-02 02:38:13 EDT
(In reply to Christopher Meng from comment #43)
> you should use find to find the la files
Says who? The only thing that matters is that they are removed.
There are many ways to achieve this.
Comment 45 Vasiliy Glazov 2013-08-02 04:05:30 EDT
Spec URL: https://raw.github.com/RussianFedora/libzen/master/libzen.spec
SRPM URL: http://koji.russianfedora.ru/koji/getfile?taskID=82976&name=libzen-0.4.29-2.fc20.R.src.rpm

0. Updated to 2.4.29.
1. Suffix .R removed.
2. install -p added.
3. .la files removed.
4. pkgconfig sed already fixed in 2.4.29
Comment 46 Christopher Meng 2013-08-02 04:12:17 EDT
Ralf, do you still have questions against this package here?

If not, I will change flag.

Thanks.
Comment 47 Michael Schwendt 2013-08-02 04:43:46 EDT
From comment 3:

> * Do not ship static libs (*.a)
> 
> * Do not ship libtool *.la's

That has been very clear, IMO. Except that the packaging guidelines ought to comment on libtool archives in a separate section and not in the static libs section.
Comment 48 Christopher Meng 2013-08-08 21:00:28 EDT
APPROVED.


Please understand workflow of fedpkg asap, I see many requests are not imported int SCM.

Thanks.
Comment 49 Vasiliy Glazov 2013-08-09 02:17:42 EDT
New Package SCM Request
=======================
Package Name: libzen
Short Description: Shared library for libmediainfo and medianfo
Owners: vascom
Branches: f18 f19 el6
InitialCC:
Comment 50 Vasiliy Glazov 2013-08-09 02:20:23 EDT
New Package SCM Request
=======================
Package Name: libzen
Short Description: Shared library for libmediainfo and mediainfo
Owners: vascom
Branches: f18 f19 el6
InitialCC:
Comment 51 Jon Ciesla 2013-08-09 08:35:49 EDT
Git done (by process-git-requests).
Comment 52 Vasiliy Glazov 2013-08-09 08:38:07 EDT
Christopher, which requests are you referring to?
Comment 53 Fedora Update System 2013-08-12 03:16:32 EDT
libzen-0.4.29-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/libzen-0.4.29-2.el6
Comment 54 Fedora Update System 2013-08-12 03:18:21 EDT
libzen-0.4.29-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/libzen-0.4.29-2.fc18
Comment 55 Fedora Update System 2013-08-12 03:19:10 EDT
libzen-0.4.29-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/libzen-0.4.29-2.fc19
Comment 56 Fedora Update System 2013-08-12 13:58:24 EDT
libzen-0.4.29-2.fc18 has been pushed to the Fedora 18 testing repository.
Comment 57 Fedora Update System 2013-08-20 20:04:43 EDT
libzen-0.4.29-2.fc18 has been pushed to the Fedora 18 stable repository.
Comment 58 Fedora Update System 2013-08-20 20:13:51 EDT
libzen-0.4.29-2.fc19 has been pushed to the Fedora 19 stable repository.
Comment 59 Fedora Update System 2013-08-31 15:19:28 EDT
libzen-0.4.29-2.el6 has been pushed to the Fedora EPEL 6 stable repository.

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