Spec URL: https://psutter.fedorapeople.org/libkeepalive.spec SRPM URL: https://psutter.fedorapeople.org/libkeepalive-0.3-1.fc25.src.rpm Description: libkeepalive is a library that enables tcp keepalive features in glibc based binary dynamic executables, without any change in the original program. Fedora Account System Username: psutter Originally I got a request for inclusion into RHEL (see private bug 1356103 for details) but I think including this into Fedora first and then cloning from there is the preferred method. The library is actually quite simple: It enables TCP keepalives in binaries by wrapping the socket() syscall and setting SO_KEEPALIVE sockopt before returning the new socket. It is activated by setting LD_PRELOAD to it's path before calling the binary to alter, so by default does not have any effect on other packages.
* CFLAGS/LDFLAGS are not used export CFLAGS="%{optflags}" export LDFLAGS="%{__global_ldflags}" but it still requires some patching of Makefile `=` -> `?=` * Missing BuildRequires: gcc * You should try to version library (though I'm not sure how much it matters here, probably doesn't at all) * echo is not needed in %build * instead of make you should use %make_build (e.g. %make_build -C src) * there are some tests, so try to build / run them * instead of doing mkdir, you can pass `-D` to install Apart from that, looks good ;)
Hi, (In reply to Igor Gnatenko from comment #1) > * CFLAGS/LDFLAGS are not used > export CFLAGS="%{optflags}" > export LDFLAGS="%{__global_ldflags}" > but it still requires some patching of Makefile `=` -> `?=` > * Missing BuildRequires: gcc > * You should try to version library (though I'm not sure how much it matters > here, probably doesn't at all) > * echo is not needed in %build > * instead of make you should use %make_build (e.g. %make_build -C src) > * there are some tests, so try to build / run them > * instead of doing mkdir, you can pass `-D` to install > > Apart from that, looks good ;) Thanks for the elaborate review! I'll try to get things sorted ASAP then upload fresh SRPM and spec file. Cheers, Phil
Hi again, (In reply to Phil Sutter from comment #2) > (In reply to Igor Gnatenko from comment #1) > > * CFLAGS/LDFLAGS are not used > > export CFLAGS="%{optflags}" > > export LDFLAGS="%{__global_ldflags}" > > but it still requires some patching of Makefile `=` -> `?=` > > * Missing BuildRequires: gcc > > * You should try to version library (though I'm not sure how much it matters > > here, probably doesn't at all) I didn't do this. To my knowledge, versioning shared objects makes sense only if programs are expected to link against them so it's possible to distinguish between minor and major upgrades. In this case, the library is meant to be injected into the program using LD_PRELOAD, so no versioning is required. > > * echo is not needed in %build > > * instead of make you should use %make_build (e.g. %make_build -C src) > > * there are some tests, so try to build / run them > > * instead of doing mkdir, you can pass `-D` to install > > > > Apart from that, looks good ;) > > Thanks for the elaborate review! I'll try to get things sorted ASAP then > upload fresh SRPM and spec file. All fixed apart from the missing versioning, updated spec file and SRPM available here: Spec URL: https://psutter.fedorapeople.org/libkeepalive.spec SRPM URL: https://psutter.fedorapeople.org/libkeepalive-0.3-2.fc25.src.rpm Thanks, Phil
* BuildRequires: git -> BuildRequires: git-core git itself requires a lot of perl which you don't really need to apply patches. * %autosetup -S git probably you just want %autosetup -p1 to not pull git into BuildRequires * %doc LICENSE README LICENSE should be %license, READNE should be %doc * Also put comment where you sent patches to upstream (or why you didn't sent)... Apart from that, looks good.
Hi Igor, (In reply to Igor Gnatenko from comment #4) > * BuildRequires: git -> BuildRequires: git-core > git itself requires a lot of perl which you don't really need to apply > patches. > * %autosetup -S git > probably you just want %autosetup -p1 to not pull git into BuildRequires Ah yes, that is much easier. When playing around with autosetup options, I got errors during patch applying, but probably that was just due to missing '-p1' option. > * %doc LICENSE README > LICENSE should be %license, READNE should be %doc > * Also put comment where you sent patches to upstream (or why you didn't > sent)... OK, DONE! Here are updated URLs for review: Spec URL: https://psutter.fedorapeople.org/libkeepalive.spec SRPM URL: https://psutter.fedorapeople.org/libkeepalive-0.3-3.fc25.src.rpm
any plans to request package in PkgDB? ;)
(In reply to Igor Gnatenko from comment #6) > any plans to request package in PkgDB? ;) Doh! I just caught up on that, thanks for the heads-up! Cheers, Phil
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/libkeepalive