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
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
For information about the error while building take a look at: http://koji.fedoraproject.org/koji/taskinfo?taskID=7594011
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.
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
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
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
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
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
Hi, Requirement line moved to devel package: 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=8286489 Matej
> 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.
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
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
> /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+
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=8348842
Seems good to me! What do you think Michael?
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}
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=8385665
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.
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=8436525
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
Git done (by process-git-requests).
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
drpm-0.1.2-1.fc21 has been pushed to the Fedora 21 testing repository.
drpm-0.1.2-1.fc21 has been pushed to the Fedora 21 stable repository.