Bug 953514 (varnish-vmod-querystring) - Review Request: varnish-vmod-querystring - QueryString VMOD for Varnish
Summary: Review Request: varnish-vmod-querystring - QueryString VMOD for Varnish
Keywords:
Status: CLOSED WONTFIX
Alias: varnish-vmod-querystring
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL: https://www.varnish-cache.org/vmod/qu...
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2013-04-18 10:58 UTC by Dridi Boukelmoune
Modified: 2016-09-27 07:35 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-09-25 18:47:42 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Dridi Boukelmoune 2013-04-18 10:58:31 UTC
Spec URL: http://dl.bintray.com/v1/content/dridi/sources/vmod-querystring/0.2/#vmod-querystring.spec
SRPM URL: http://dl.bintray.com/v1/content/dridi/sources/vmod-querystring/0.2/#vmod-querystring-0.2-0.gitb3868eb.v3.0.3.fc18.src.rpm
Fedora Account System Username: dridi
Description:

Hello,

I've written a module for Varnish, and I've tried to package it following Fedora's
guidelines but it doesn't seem to fit. I would appreciate a review of my current
work before submitting the real "0.2" release package (which will include your
review's modifications).

The module is a shared library, but there is no need for ldconfig in %pre and
%post scriptlets because it is loaded "manually" by varnish (and it lands on
%_libdir/varnish/vmods).

The module needs to be built along with Varnish, and Varnish won't load a module
that was not built with the same exact version, which means that the module
requires more than just varnish (Requires: varnish = 3.0.x). So I had to make
some sort of conditional build, but it seems like --with[out] options in rpmbuild
are only boolean switches.

The RPM is currently built with a %VARNISHVER macro equal to the varnish version:
- rpmbuild -ba --define 'VARNISHVER 3.0.3' vmod-querystring.spec
- rpmbuild --rebuild --define 'VARNISHVER 3.0.3' vmod-querystring-0.2-0.gitb3868eb.v3.0.3.fc18.src.rpm
- mock --define 'VARNISHVER 3.0.3' vmod-querystring-0.2-0.gitb3868eb.v3.0.3.fc18.src.rpm

Unlike a --with[out] option, the %VARNISHVER macro isn't serialized in the source
RPM so it is needed again when building the binary RPM.

To be able to build the same module against different versions of Varnish, I've
added a ".v3.0.x" string in the release tag, the version on the other hand is the
same as the upstream project. One final word on the package name. The upstream
project is named libvmod-querystring, and I've stripped the lib prefix because
of some work on Varnish's side on the matter:
https://github.com/varnish/libvmod-example/blob/ab13e3ab6ff4093823f5c7775d9c20b21f680dd4/vmod-example.spec

The vmod-example is a hello world module you can fork to make your own (it
contains a working build tree as a starting point).

For the vmod build, I use autogen.sh because I don't want to make a source
distribution with "make dist" since it needs to be %configured first with a
hard-coded path to the varnish build tree.

As for my own testing, I can build it on an x86 Fedora VM, except for varnish
3.0.3 (you'll find Fedora's patches for varnish 3.0.3 in the source RPM to make
it work). I can only build it for varnish 3.0.3 on my x64 Fedora Laptop. With
mock, the build fails on the rpmbuild command, but it works when I chroot to mocks
build root and do it myself as mockbuild.

This is my first package for Fedora Extras, and it had to break the guidelines,
so please don't be too harsh. I hope I answered all your potential questions.
I also don't know whether I need a sponsor.

Best Regards,
Dridi

Comment 1 Kevin Fenzi 2013-05-09 18:41:47 UTC
So, some general comments. ;) 

- Usually when we package extensions or plugins for a package they have the base package name in the name of the extension or plugin. So, you might consider renaming this: 
varnish-extension-querystring or varnish-vmod-querystring?

- You cannot do conditional builds like that in the Fedora buildsystem. I'd suggest doing a 'BuildRequires: varnish-exact-version' and set it to that version. Then, when varnish is updated, you update that to the new version and rebuild. 

- This needs patches to varnish? Have you opened a bug to get the Fedora varnish maintainer(s) feedback on applying those?

Comment 2 Dridi Boukelmoune 2013-05-10 04:16:51 UTC
(In reply to comment #1)
> So, some general comments. ;) 

Thank you for your answer, I'll provide new spec and source rpm asap. But first I can answer your questions.

> - Usually when we package extensions or plugins for a package they have the
> base package name in the name of the extension or plugin. So, you might
> consider renaming this: 
> varnish-extension-querystring or varnish-vmod-querystring?

I see the point, and varnish-vmod-querystring sounds better (and more community-ish) to me, I'll use that.

> - You cannot do conditional builds like that in the Fedora buildsystem. I'd
> suggest doing a 'BuildRequires: varnish-exact-version' and set it to that
> version. Then, when varnish is updated, you update that to the new version
> and rebuild. 

Ok then it would be my responsibility to follow varnish's releases and make the vmod stay in sync ? Also, the varnish package is not useful at build time, unless there's a devel package I've missed with all the bits needed by vmods. This is the reason why I also build varnish from the source.

> - This needs patches to varnish? Have you opened a bug to get the Fedora
> varnish maintainer(s) feedback on applying those?

No I have not, I've discovered a segfault issue when I started building the vmod against different versions of varnish on different archs. Since at the time I opened this issue only varnish 3.0.3 was available in the repo, I took the patches from the source RPM for Fedora 18.

Comment 3 Kevin Fenzi 2013-05-10 17:02:03 UTC
Is varnish-libs-devel what you need? Or something more? If it's something more, you should consult with the varnish maintainer(s) and ask for them to expose the functions you need. Bundling varnish is not possible in a Fedora package. 

Yes, you would then need to rebuild this package anytime varnish updated.

Comment 4 Dridi Boukelmoune 2013-05-11 01:03:53 UTC
I hope this will answer your questions. I still have to provide a new spec and source rpm for review.

(In reply to comment #3)
> Is varnish-libs-devel what you need?

It seems that varnish-libs-devel only contains what's usually needed to use a shared object (specifically libvarnishapi for this package).

> Or something more?

It is definitely something more, as of today, if you want to build any vmod that I've heard of, you need a varnish build:
- the source tree contains vmod build scripts
- tests need varnishtest and varnishd (built from the source)

> If it's something more, you should consult with the varnish maintainer(s)
> and ask for them to expose the functions you need.

It would require a change in the upstream (varnish) build system in order to install and then package what's needed (this can be solved with a patch of course). It would also require changes in the vmod build.

I'll wait for your answer before asking the varnish maintainer(s).

>Bundling varnish is not possible in a Fedora package. 

I am not sure I understand this one. I don't package (if that's what you mean with bundling) varnish with the module, it is just an additional upstream required at buildtime.

On my laptop, the resulting package contains:
$ rpm -ql vmod-querystring
/usr/lib64/varnish/vmods/libvmod_querystring.so
/usr/share/doc/vmod-querystring-0.2
/usr/share/doc/vmod-querystring-0.2/CONTRIBUTORS
/usr/share/doc/vmod-querystring-0.2/LICENSE
/usr/share/man/man3/vmod_querystring.3.gz

Comment 5 Dridi Boukelmoune 2013-05-11 16:55:28 UTC
As requested, the build is no more conditional, and I've renamed the package to varnish-vmod-querystring (and incidentally removed varnish's version in the release tag).

Spec and source RPM: http://dl.bintray.com/dridi/sources/varnish-vmod-querystring/0.2.5c5dc14/

I've also added missing build dependencies (namely autoconf, automake and libtool) and now it builds with mock :)

Comment 6 Kevin Fenzi 2013-06-01 17:15:21 UTC
Adding the Fedora varnish maintainer for comment. If he's ok with this I'm happy to help move it forward. ;)

Comment 7 Dridi Boukelmoune 2013-06-15 10:37:34 UTC
With Varnish 3.0.4 coming soon, it might be simpler to wait until it's packaged on Fedora. Then, if this package is ok in its current form, I'll re-submit another which targets Varnish 3.0.4.

Comment 8 Dridi Boukelmoune 2013-07-27 09:21:07 UTC
Hi,

During the last months, the version 0.2 of the module, Fedora 19, and Varnish 3.0.4 have been released :)

Since Varnish 3.0.4 hasn't been packaged yet, I've made new packages for Varnish 3.0.3 on Fedora 19:
https://bitbucket.org/dridi/fedora_packages/downloads/varnish-vmod-querystring.spec
https://bitbucket.org/dridi/fedora_packages/downloads/varnish-vmod-querystring-0.2-1.fc19.src.rpm

Comment 9 Ingvar Hagelund 2013-07-28 20:44:16 UTC
I'll have a look at this, and varnish 3.0.4 after my summer's vacation. Too little time to do packaging this summer.

Ingvar

Comment 10 Ingvar Hagelund 2013-08-12 12:07:28 UTC
I haven't looked in detail on this yet, but if I understand you correctly, you have to get the source tree of varnish (like the .src.rpm, for example) to be able to build the vmod. Is this correct?

There are several ways to accomplish this. One is to create a package varnish-devel, containing the complete source tree, including fedora patches. That is the easiest way out, I guess.

Mark that the vmod at least during the varnish-3.x series, has to have a hard dependency on the varnish version it is built against. If there is different versions of varnish available, for example while an update is rushed out, the vmod might break unless it's updated at the same time. This is impractical, but I can't see any other solution. Coordination could happen via the varnish-dist email list, I guess.

Ingvar

Comment 11 Dridi Boukelmoune 2013-08-13 09:12:49 UTC
(In reply to Ingvar Hagelund from comment #10)
> I haven't looked in detail on this yet, but if I understand you correctly,
> you have to get the source tree of varnish (like the .src.rpm, for example)
> to be able to build the vmod. Is this correct?

Almost correct, this is currently just another "upstream":
# VMODs need a varnish build from the source, this is by design
Source0: http://github.com/Dridi/%{VMODNAME}/archive/v%{version}.tar.gz
Source1: http://repo.varnish-cache.org/source/varnish-%{VARNISHVER}.tar.gz

> There are several ways to accomplish this. One is to create a package
> varnish-devel, containing the complete source tree, including fedora
> patches. That is the easiest way out, I guess.

This would probably help, but where would it land in the filesystem ? Something like "/usr/src/varnish" ?

My concern is that it would be a read-only source tree, which would not be enough. If you don't compile varnish and just use C headers, and python scripts for vmod-related code generation, then you have to significantly patch the vmod build to make it use binaries in /usr instead of those built in the source tree.

Being both the upstream and the packager, this isn't really a problem. My plan was to submit more useful vmods, and I don't know which solution would be simpler.

> Mark that the vmod at least during the varnish-3.x series, has to have a
> hard dependency on the varnish version it is built against.

Yes and still true for Varnish 4, in the spec this is enforced with:
Requires:      varnish = %{VARNISHVER}

> If there is
> different versions of varnish available, for example while an update is
> rushed out, the vmod might break unless it's updated at the same time. This
> is impractical, but I can't see any other solution. Coordination could
> happen via the varnish-dist email list, I guess.

The vmod *will* break even if it builds correctly, because the ABI check is done at runtime. So yes we would probably need to coordinate our releases. The current version of the module works with both varnish 3.0.4 and varnish 4. The varnish 4 support might break until it's actually released, but I'm regularly checking the master branch.

I hope this answer your questions,
Dridi

Comment 12 Ingvar Hagelund 2013-08-14 06:54:38 UTC
I think you should post to the varnish-dist list. But I think the main devs' answer will be something along the lines I got when I asked for this on IRC:

"Don't package vmods until 4.0".

:-/

Ingvar

Comment 13 Dridi Boukelmoune 2013-11-14 16:42:27 UTC
Hi Ingvar,

It's been quite a while, and I'd like to resume this review because lately I've had to install a couple modules (the ones I'd like to package).

Now that I understand the process better, I'd like to try your proposal of a source tree in a varnish-devel package. I believe it would not be too hard to use binaries in /usr instead of those built in the source tree. I may not even need to build the varnish source tree at all (but I haven't tested that yet).

Would you have time to work on this ?

Dridi

Comment 14 Ingvar Hagelund 2013-11-20 23:16:02 UTC
As this is not supported by upstream for varnish 3, and I have to find extra time to work on varnish packaging, I would prefer to wait for varnish 4.

Ingvar

Comment 15 Dridi Boukelmoune 2013-11-25 13:28:23 UTC
If it's a matter of time, I'll gladly help you with the varnish part. I have a simple patch for the master branch of varnish's package that adds a devel package with the source tree in /usr/src/varnish.

I could either open a bug or ask for commit rights to save your time. Pushing a varnish-devel package in rawhide would at least help me fix the vmod spec and move this review forward.

Varnish 4 will be a big release with a lot of breaking changes, it won't be there for f20 (hopefully f21). With such a devel package, I could start packaging other modules for f19 and f20, modules I could port to varnish 4 if upstreams are not ready when the time comes. I've done that for the querystring module which builds against varnish 4 devel branch (last time I checked it was still good).

Would you be OK with me helping on the varnish package ? I could also co-maintain it for varnish >= 3.

Dridi

Comment 16 Ingvar Hagelund 2016-01-12 07:15:29 UTC
For what it's worth, I have started some general vmod packaging for fedora and epel: https://copr.fedoraproject.org/coprs/ingvar/varnish41/packages/

vmod-querystring is not ported to varnish-4.1 yet, so this bug may sleep until that is done.

I may start to submit my copr packages to fedora when the vmod maintainers have standardized on version numbering. For now, many of the version numbers in my copr packages are pure guesswork. Upstream is working on this.

Ingvar

Comment 17 Dridi Boukelmoune 2016-09-25 18:47:42 UTC
Closing in favor of #1379174


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