Bug 964161 - Review Request: libpng15 - backwards compatibility for libpng
Review Request: libpng15 - backwards compatibility for libpng
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kalev Lember
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-17 07:44 EDT by Petr Hracek
Modified: 2013-06-26 07:49 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-06-26 07:49:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kalevlember: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Petr Hracek 2013-05-17 07:44:06 EDT
Spec URL: http://phracek.fedorapeople.org/libpng15/libpng15.spec
SRPM URL: http://phracek.fedorapeople.org/libpng15/libpng15-1.5.13-2.fc18.src.rpm
Description: Description: Proposed backwards-compatibility package for libpng 1.5

Because LSB wants libpng 1.5 to be available, we can't just drop it after all
other packages are recompiled to use libpng 1.6.  It needs to be a standalone
package, and here is that package.

One thing that's possibly questionable is whether to bother with a -devel subpackage at all; but if we're going to offer the library it should probably be possible to compile against it.

Fedora Account System Username: phracek

== Review ==

$> rpmlint /home/phracek/rpmbuild/SRPMS/libpng15-1.5.13-2.fc18.src.rpm
libpng15.src: W: spelling-error %description -l en_US libpng -> sibling
libpng15.src: W: spelling-error %description -l en_US Libpng -> Sibling
libpng15.src: W: invalid-url Source0: ftp://ftp.simplesystems.org/pub/png/src/libpng-1.5.13.tar.bz2 <urlopen error ftp error: [Errno ftp error] 550 libpng-1.5.13.tar.bz2: No such file or directory>
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
$>
Comment 1 Kalev Lember 2013-05-17 09:28:36 EDT
(In reply to comment #0)
> One thing that's possibly questionable is whether to bother with a -devel
> subpackage at all; but if we're going to offer the library it should
> probably be possible to compile against it.

Up to you if you want to support building against the older libpng 1.5 or not. I think there are two main use cases:

 1) Packages within Fedora package collection. Since libpng 1.5 and 1.6 APIs are almost equivalent, I think it would make sense if all packages within Fedora migrate to libpng 1.6 during the F20 release cycle, without exceptions. To enforce that transition, it might make sense to just not ship the libpng15-devel.

 2) Packages outside of Fedora. There are people who build packages for 3rd party distribution and want to make them work on a wide range of Fedora releases. I assume they'd find the libpng15-devel useful. (e.g. the request at https://bugzilla.redhat.com/show_bug.cgi?id=845110#c9 )
Comment 2 Kalev Lember 2013-05-17 09:50:04 EDT
Taking for review.

Some issues about packaging, in no particular order:

 1) Don't set Epoch in new packages, the field should be left out (and delete all references to %{epoch} below)

 2) The description and summary should reflect that this is a compatibility package only, similar to what http://pkgs.fedoraproject.org/cgit/libpng12.git/tree/libpng12.spec has

 3) I would kill off the static .a libraries and the -static subpackage for a compat package like this

 4) It doesn't build:

> $ rpmbuild -ba libpng15.spec 
> [...]
> + cd libpng15-1.5.13
> /var/tmp/rpm-tmp.WgkPQ3: line 35: cd: libpng15-1.5.13: No such file or directory

Use '%setup -q -n libpng-%{version}' to fix this.

 5) Requires: pkgconfig%{?_isa} is superfluous, the pkgconfig dep is already automatically generated by rpmbuild. (If you fix it here, also do it in the libpng 1.6 package, please)

 6) -devel subpackage, if you decide to ship it, should have explicit Conflicts: libpng-devel, because the file names overlap.

 7) The man pages in the main package are going to conflict between libpng15 and libpng, which need to be parallel installable. Move them to -devel if you decide to ship it, or just remove if not. I'd recommend only shipping the LICENSE file and the libs in the main package:

%files
%doc LICENSE
%{_libdir}/libpng15.so.*

 8) example.c and libpng-manual.txt are developer oriented and should go to -devel package %doc instead (both in libpng15 and libpng 1.6 packages)

 9) Add a %changelog entry saying that this is a renamed package
Comment 3 Petr Hracek 2013-05-27 10:18:15 EDT
Spec URL: http://phracek.fedorapeople.org/libpng15/libpng15.spec
SRPM URL: http://phracek.fedorapeople.org/libpng15/libpng15-1.5.13-3.fc18.src.rpm

I have just uploaded new version
devel and static subpackages will not be delivered.
Comment 4 Kalev Lember 2013-05-27 18:30:05 EDT
The %files section is including too many files:

%files
%doc LICENSE
%{_libdir}/libpng15.*
%{_libdir}/pkgconfig/libpng15.pc

$ rpm -qlp libpng15-1.5.13-3.fc20.x86_64.rpm
/usr/lib64/libpng15.a
/usr/lib64/libpng15.so
/usr/lib64/libpng15.so.15
/usr/lib64/libpng15.so.15.13.0
/usr/lib64/pkgconfig/libpng15.pc
/usr/share/doc/libpng15-1.5.13
/usr/share/doc/libpng15-1.5.13/LICENSE

It shouldn't have the .a file and the .so symlink and the .pc file, these are all development files.

I'd use the following for the %files section:

%files
%doc LICENSE
%{_libdir}/libpng15.so.*

... and %configure --disable-static to remove the .a file, plus some more rm commands to get rid of the .so symlink and the .pc file.
Comment 5 Kalev Lember 2013-05-27 18:35:34 EDT
Some more minor nitpicking:

> Patch0: libpng-multilib.patch
> ...
> %patch0 -p1

The patch is for /usr/bin/libpng-config which you aren't shipping in the package, so should be able to drop it if you want.


> %description
> ...
> This version should be used only if you are unable to use the current
> version of libpng

Should have a full stop . at the end of the sentence.


> * Mon May 27 2013 Fedora Release Engineering <rel-eng@lists.fedoraproject.org> - 2:1.5.13-3
> - this is only renamed package

Add your own name here, instead of Fedora Release Engineering and remove the epoch 2:
Comment 6 Petr Hracek 2013-05-28 08:28:53 EDT
Well, hopefully this is final version.
Spec and SRPM URLs are the same.

$> rpmlint /home/phracek/rpmbuild/SRPMS/libpng15-1.5.13-3.fc18.src.rpm
libpng15.src: W: spelling-error Summary(en_US) libpng -> sibling
libpng15.src: W: spelling-error %description -l en_US libpng -> sibling
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
$> rpm -qpl /home/phracek/rpmbuild/RPMS/x86_64/libpng15-1.5.13-3.fc18.x86_64.rpm
/usr/lib64/libpng15.so.15
/usr/lib64/libpng15.so.15.13.0
/usr/share/doc/libpng15-1.5.13
/usr/share/doc/libpng15-1.5.13/LICENSE
$>
Comment 7 Kalev Lember 2013-05-29 02:46:21 EDT
Fedora review libpng15-1.5.13-3.fc18.src.rpm 2013-05-29

$ rpmlint libpng15-1.5.13-3.fc20.src.rpm \
          libpng15-1.5.13-3.fc20.x86_64.rpm \
          libpng15-debuginfo-1.5.13-3.fc20.x86_64.rpm

libpng15.src: W: spelling-error Summary(en_US) libpng -> sibling
libpng15.src: W: spelling-error %description -l en_US libpng -> sibling
libpng15.src: E: specfile-error warning: bogus date in %changelog: Thu Jul 28 2000 Preston Brown <pbrown@redhat.com>
libpng15.x86_64: W: spelling-error Summary(en_US) libpng -> sibling
libpng15.x86_64: W: spelling-error %description -l en_US libpng -> sibling
libpng15.x86_64: W: incoherent-version-in-changelog 2:1.5.13-3 ['1.5.13-3.fc20', '1.5.13-3']
3 packages and 0 specfiles checked; 1 errors, 5 warnings.


+ OK
! needs attention

+ The package is named according to Fedora packaging guidelines
+ The spec file name matches the base package name.
+ The package meets the Packaging Guidelines
+ The package is licensed with a Fedora approved license and meets the
  Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ The package contains the license file (LICENSE)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match sources in the srpm. md5sum:
  186b3098d1ef844f76681bc69968efe2  libpng-1.5.13.tar.bz2
  186b3098d1ef844f76681bc69968efe2  Download/libpng-1.5.13.tar.bz2
+ The package builds in koji (F20)
n/a ExcludeArch bugs filed
+ BuildRequires look sane
n/a Proper locale handling
+ ldconfig in %post and %postun
+ Package does not bundle copies of system libraries
n/a Package isn't relocatable
+ Package owns all the directories it creates
+ No duplicate files in %files
+ Permissions are properly set
+ Consistent use of macros
+ The package must contain code or permissible content
n/a Large documentation files should go in -doc subpackage
+ Files marked %doc should not affect package
n/a Header files should be in -devel
n/a Static libraries should be in -static
n/a Library files that end in .so must go in a -devel package
n/a -devel must require the fully versioned base
+ Packages should not contain libtool .la files
n/a .desktop file handling
+ Doesn't own files or directories already owned by other packages
+ Filenames are valid UTF-8

Remaining issues:

 o rpmlint warnings: incoherent-version-in-changelog and bogus date in %changelog -- should be easy to fix up to make rpmlint happier
 o source URL: I had to locally fix it up to verify the sources, would be great if you could do it in the package file as well

- Source0: ftp://ftp.simplesystems.org/pub/png/src/libpng-%{version}.tar.bz2
+ Source0: ftp://ftp.simplesystems.org/pub/png/src/history/libpng15/libpng-%{version}.tar.bz2

Otherwise looks good. No need to post new spec files here, just fix these two minor issues up on your own before importing the package.


APPROVED
Comment 8 Petr Hracek 2013-05-29 04:10:32 EDT
New Package SCM Request
=======================
Package Name: libpng15
Short Description: Old version of libpng, needed to run old binaries
Owners: phracek
Branches:
Comment 9 Gwyn Ciesla 2013-05-29 07:41:20 EDT
Git done (by process-git-requests).
Comment 10 Petr Hracek 2013-05-29 10:27:40 EDT
scm-commit (http://lists.fedoraproject.org/pipermail/scm-commits/Week-of-Mon-20130527/1030132.html) -> MODIFIED

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