Bug 1142407 - Review Request: drpm - deltarpm manipulation library
Summary: Review Request: drpm - deltarpm manipulation library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tomas Mlcoch
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-09-16 17:30 UTC by Pavel Tobiáš
Modified: 2015-02-15 03:21 UTC (History)
8 users (show)

Fixed In Version: drpm-0.1.2-1.fc21
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-02-15 03:21:25 UTC
Type: ---
tmlcoch: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
build.log (6.00 KB, text/plain)
2014-11-20 13:00 UTC, Tomas Mlcoch
no flags Details

Description Pavel Tobiáš 2014-09-16 17:30:29 UTC
Spec URL: http://www.stud.fit.vutbr.cz/~xtobia01/drpm.spec
SRPM URL: http://www.stud.fit.vutbr.cz/~xtobia01/drpm-0.1.0-1.fc20.src.rpm

Description: 
Currently, the drpm package provides a small library allowing one to fetch various info from deltarpm packages. The long-term aim of the project is to replace the deltarpm suite completely by providing a similar, backwards-compatible deltarpm-making and -applying functionality as a part of the API, as well as in form of stand-alone utilities.

Fedora Account System Username: ptobias

Comment 1 Florian "der-flo" Lehner 2014-09-16 18:27:18 UTC
hi!

At the moment there are some Issues:

[ ] Your package isn't building.
    drpm_compstrm.c:29:18: fatal error: lzma.h: No such file or directory

[ ] %changelog is missing in the spec-file

[ ] Please preserve timestamps while using cp

Cheers,
 Florian

Comment 2 Florian "der-flo" Lehner 2014-09-16 18:34:25 UTC
For information about the error while building take a look at: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=7594011

Comment 3 Christopher Meng 2014-09-17 04:30:38 UTC
1. DO NOT use cp.

Please, use install with option -m to set the perms as well, it will save your time and will avoid the weird perms by accident.

Anyway, why can't this be done by make install?

2. BuildRequires:  rpm-libs, rpm-devel, zlib, zlib-devel, lzma-sdk, lzma-sdk-devel, bzip2-libs, bzip2-devel

Ok, please take a look at -devel packages first, Fedora packages have requires for libs if they are linked correctly.

bzip2-devel:

= Requires =
bzip2-libs = 1.0.6-14.fc22
libbz2.so.1

zlib-devel:

= Requires =
libz.so.1
zlib = 1.2.8-7.fc22

And so on...

So the final BR list should be:

rpm-devel
zlib-devel
lzma-sdk-devel
bzip2-devel

And I hope you can put dependencies by line instead of putting them together with comma separated in one line, poor readability.

3. Is this a public library? If so you should use soname on it.

4. make %{?_smp_mflags}

Please honor the %{optflags} and %{__global_ld_flags} macros.

5. %description
Currently, the drpm package provides a small library allowing one to fetch
various info from deltarpm packages. The long-term aim of the project is to
replace the deltarpm suite completely by providing a similar,
backwards-compatible deltarpm-making and -applying functionality as a part
of the API, as well as in form of stand-alone utilities.

We don't need any explanation about the past, just tell us the main usage:

%description
This project is to replace the deltarpm suite completely by providing a similar,
backwards-compatible deltarpm-making and -applying functionality as a part
of the API, as well as in form of stand-alone utilities.

6. @Flo I just reset the review flag because he needs a sponsor.

Comment 5 Matěj Chalk 2014-11-19 12:45:58 UTC
Hi, I've taken over the project from Pavel Tobias. Here are the edited files:

SPEC: http://www.stud.fit.vutbr.cz/~xchalk00/drpm.spec
SRPM: http://www.stud.fit.vutbr.cz/~xchalk00/drpm-0.1.1-1.fc20.src.rpm

They should now be in accordance with your specifications, except for the description (5.). You see, replacing the deltarpm suite is the long term aim of the project, but currently it does not offer the full range, so I think it would be misleading to phrase the description as if it did. So I have shortened the original description, but I've left out the stuff about the long term aim instead, so it now reads simply:

%description
Currently, the drpm package provides a small library allowing one to fetch
various info from deltarpm packages.

Which is also in accordance with the (unchanged) summary:

Summary:        A small library for fetching information from deltarpm packages



Fedora Account System Username: mchalk

Comment 6 Tomas Mlcoch 2014-11-20 12:55:36 UTC
Hi Matej,
few things:

1) Build

I've got an error:
drpm_compstrm.c:29:18: fatal error: lzma.h: No such file or directory
 #include <lzma.h>

2) BuildRequire lzma-sdk-devel

This looks weird to me:
BuildRequires:  lzma-sdk-devel
do you realy need lzma-sdk-deve? Wouldn't be the "lzma-devel" the appropriate build dep (this could also fix the build error)?

3) Unversioned .so

The unversioned .so (libdrpm.so) should go into devel sub-package.
See: https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages

4) Package version in chagelogs

Please include pkg version and release in changelogs.
Also please keep the individual changelogs separated by one newline for better readability. (Also, it is a good convention to include your whole name before the email address)
See: https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

5) Devel package should require the base package

So please add the line to devel package:
Requires: %{name}%{?_isa} = %{version}-%{release}
See: https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


Suggestion:

a) It would be nice to have a drpm.pc (pkgconfig file) for pkg-config tool in the devel sub-package as well.

b) Add address of upstream git repo to the project homepage. (https://fedorahosted.org/drpm/). I've noticed that my drpm checkout from the very beginning of drpm development is not up to date with source code in the srpm you have pasted here (at least the Makefile is different) and git pull doesn't pull any changes.

c) Makefile is fine for such small project, but if you plan to extend it, consider use of more sophisticated build system like the CMake.

d) Consider of removing word "Current" from description. I think that the present is implicit and doesn't have to be specified explicitly. Mostly, we care about the package we have right now, we are not interested about its possible features in distant future. :)

Tomas

Comment 7 Tomas Mlcoch 2014-11-20 13:00:27 UTC
Created attachment 959310 [details]
build.log

See the attached build.log with the build error.

Protip: Next time, before submitting a srpm to review, try to build it in koji first by:
$ koji build rawhide --scratch drpm-0.1.1-1.fc20.src.rpm
and paste the link to the build task together with updated srpms and spec file.
E.g. http://koji.fedoraproject.org/koji/taskinfo?taskID=8193243

Comment 8 Matěj Chalk 2014-12-03 14:51:04 UTC
Hi,

Thanks for your review. Here are the updated links:

SPEC: http://www.stud.fit.vutbr.cz/~xchalk00/drpm.spec
SRPM: http://www.stud.fit.vutbr.cz/~xchalk00/drpm-0.1.2-1.fc20.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=8285782

Switching to CMake is planned.


Matej

Comment 9 Tomas Mlcoch 2014-12-03 15:50:12 UTC
Hi Matej,
good work, it's almost ready, but there is still one issue:

The line with requirement for base package "Requires: %{name}%{?_isa} = %{version}-%{release}" should go to devel package's section:

%package devel
Summary:        C interface for the drpm library
Requires:       %{name}%{?_isa} = %{version}-%{release}

Tomas

Comment 11 Michael Schwendt 2014-12-03 17:01:19 UTC
> rm -f %{buildroot}%{_libdir}/libdrpm.so.0
> ln -sf libdrpm.so.0.0.0 %{buildroot}%{_libdir}/libdrpm.so.0

Provided that the SONAME for this library is set correctly, you should be able to run

  /sbin/ldconfig -N -n %{buildroot}%{_libdir}

instead of trying to do it manually with "ln".


> %post
> /sbin/ldconfig
> 
> %postun
> /sbin/ldconfig

For plain library packages, prefer

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

which will execute ldconfig _directly_ instead of running it via /bin/sh. It would also make the package depend on /sbin/ldconfig directly instead of /bin/sh.


> License:        LGPLv3
> %doc COPYING

http://www.gnu.org/licenses/gpl-faq.html#v3HowToUpgrade

| If you're using LGPLv3 in your project, be sure to include copies of both
| GPLv3 and LGPLv3, since LGPLv3 is now written as a set of additional
| permissions on top of GPLv3.


> %{_datadir}/pkgconfig/drpm.pc

Wrong dir. Should be %{_libdir}.

Plus, the file contents are broken. Test them, please! The "Requires" field in .pc files is for pkg-config inter-dependencies, _not_ for RPM dependencies. Also check carefully which dependencies you really need when linking with -ldrpm. You don't want to relink with libs libdrpm is linked with already, for example.

Comment 12 Michael Schwendt 2014-12-04 16:36:23 UTC
For example:

$ pkg-config --cflags --libs drpm
Package rpm-devel was not found in the pkg-config search path.
Perhaps you should add the directory containing `rpm-devel.pc'
to the PKG_CONFIG_PATH environment variable
Package 'rpm-devel', required by 'drpm', not found


> http://koji.fedoraproject.org/koji/taskinfo?taskID=8286489
> https://kojipkgs.fedoraproject.org//work/tasks/6491/8286491/build.log

cc -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -std=c99 -pedantic -Wall -Wextra -fPIC -g -O0   -c -o drpm.o drpm.c

#warning _FORTIFY_SOURCE requires compiling with optimization (-O) [-Wcpp]
 #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)

https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

Comment 13 Matěj Chalk 2014-12-11 12:17:59 UTC
SPEC: http://www.stud.fit.vutbr.cz/~xchalk00/drpm.spec
SRPM: http://www.stud.fit.vutbr.cz/~xchalk00/drpm-0.1.2-1.fc20.src.rpm
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=8347846

/sbin/ldconfig -N -n %{buildroot}%{_libdir} didn't fix symlink to buildroot error.

Matej

Comment 14 Michael Schwendt 2014-12-11 13:15:31 UTC
> /sbin/ldconfig -N -n %{buildroot}%{_libdir} didn't fix symlink to buildroot
> error.

What build target you refer to? That command is not supposed to create a "symlink to buildroot":

  $ ls
  libdrpm.so.0.0.0  pkgconfig
  $ ldconfig -N -n $(pwd)
  $ ls
  libdrpm.so.0  libdrpm.so.0.0.0	pkgconfig
  $ file libdrpm.so.0
  libdrpm.so.0: symbolic link to `libdrpm.so.0.0.0'

Anyway, since  make install  doesn't install the libdrpm.so symlink either, the command would only help with half of the show (note that adjusting SONAME symlinks manually inside the spec file has gone wrong for packages before, so beware).


> http://koji.fedoraproject.org/koji/taskinfo?taskID=8347848

The build still overrides Fedora's compiler flags. See comment 12.


> License:        LGPLv3

The source files contradict: LGPLv3+

Comment 16 Tomas Mlcoch 2014-12-11 15:31:04 UTC
Seems good to me! What do you think Michael?

Comment 17 Michael Schwendt 2014-12-11 17:02:48 UTC
Just the .so symlink is missing now in the -devel package. One cannot link with -ldrpm without this symlink. 

The %changelog tells:

  | - Removed unversioned .so from package

That only tells what has been done, not _why_ it has been done. ;-) The better comments in changelogs and in source code give the rationale and answer the "why?".

And yes, the upstream drpm Makefile ought to install this symlink to begin with.


> %install
> make install DESTDIR=%{buildroot} ...

The guidelines recommend %make_install these days (see "rpm -E %make_install" for what it does).

  %make_install libdir=%{_libdir} includedir=%{_includedir}

Comment 19 Vladimir Stackov 2014-12-16 08:42:07 UTC
Greetings,

a few comments on your spec.

1. Could you enable hardened build (https://fedoraproject.org/wiki/Packaging:Guidelines#PIE)? It seems that you package is building successfully with PIE enabled;
2. Use %license for license information insted of %doc.

Thanks!

Please note that this is informal review.

Comment 21 Matěj Chalk 2015-01-29 11:29:07 UTC
New Package SCM Request
=======================
Package Name: drpm
Short Description: A small library for fetching information from deltarpm packages
Upstream URL: http://fedorahosted.org/drpm
Owners: mchalk
Branches: f21 f22
InitialCC: am1g0 cicku jzeleny mschwendt paragn tmlcoch

Comment 22 Gwyn Ciesla 2015-01-29 14:35:45 UTC
Git done (by process-git-requests).

Comment 23 Fedora Update System 2015-01-29 15:40:54 UTC
drpm-0.1.2-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/drpm-0.1.2-1.fc21

Comment 24 Fedora Update System 2015-01-30 04:29:11 UTC
drpm-0.1.2-1.fc21 has been pushed to the Fedora 21 testing repository.

Comment 25 Fedora Update System 2015-02-15 03:21:25 UTC
drpm-0.1.2-1.fc21 has been pushed to the Fedora 21 stable repository.


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