varnish-cache icon indicating copy to clipboard operation
varnish-cache copied to clipboard

Expose almost all of libvarnish as libvarnishapi

Open neufeind opened this issue 2 years ago • 10 comments

Expected Behavior

Build the slash-vmod mentioned in your news should work using varnish-devel package (on Rocky 9).

Current Behavior

varnish-devel is missing libvarnish.la. That is created during compilation but not added to varnish-devel during packaging.

Possible Solution

For now it's possible to build the slash-vmod by hand-compiling the whole varnish. But the docs for building the module on Debian suggest that installing varnish-devel (along with other build-tools) should be sufficient. However the rpm (package used here on Rocky 9) does not contain libvarnish.la.

Steps to Reproduce (for bugs)

  1. Install varnish and varnish-devel (7.3.0) from packagecloud-repos on Rocky 9.
  2. Try to build slash-vmod (7.3 branch).

Context

Trying to build slash-vmod.

Varnish Cache version

varnishd (varnish-7.3.0 revision 84d79120b6d17b11819a663a93160743f293e63f)

Operating system

Rocky Linux 9

Source of binary packages used (if any)

https://packagecloud.io/varnishcache/varnish73/el/9/$basearch

neufeind avatar Jun 25 '23 21:06 neufeind

Hi @neufeind, I am the author of SLASH/, which is an extension not part of core varnish-cache code. Thus, a better place for this issue would have been https://gitlab.com/uplex/varnish/slash/-/issues

Regarding your suggestion:

  • At this point, libvarnish is not planned for inclusion in varnish-devel because it is only intended to be used by varnishd and bundled tools. (edit: for completeness, libvarnishapi does link statically to libvarnish, because it itself uses libvarnish code, but does not export its symbols).
  • vmod_slash itself would not need to statically link to libvarnish.
  • but SLASH/ brings a along unit tests and an additional utility (fellow_log_dbg), which need functions from libvarnish

I understand that having to compile varnish-cache may be an inconvenience, but INSTALL.rst clearly mentions this requirement:

Build required components from varnish-cache: [...]

Note that besides libvarnish, SLASH/ also needs libvsc from varnish-cache.

nigoroll avatar Jun 26 '23 17:06 nigoroll

In my eyes it was an "issue" or something that could be fixed in the varnish-devel packages. The readme imho suggests that it might compile on Debian with just the devel-packages (if I read right, can't test on Debian). So I thought it was something that could be improved by varnish-devel. The files are built when building varnish - they are just not packaged in the devel, or am I mistaken? If you say they aren't public API then that's possibly something that varnish and slack should negotiate how to address (if I may suggest). It would be great to have the slash-VMOD. Even greater if there were prebuilt packages :-) but at least being able to build them without rebuilding the varnish itself as well would help testing. A great extension/vmod by the way :-)

neufeind avatar Jul 03 '23 16:07 neufeind

May I suggest adding --with[out]-tests options to the configure script, defaulting to --without-tests and adding --with-tests to the bootstrap script?

This way package maintainers aren't bothered by the optional dependency, only slash developers.

dridi avatar Jul 03 '23 16:07 dridi

@Dridi I would want package maintainers to bundle fellow_log_dbg because it might provide invaluable debug information in case anyone finds a log inconsistency, so as much as I appreciate your suggestion, it would require to give up on this.

As a follow up to this ticket, I had brief discussion with @bsdphk on IRC if the libvarnish/libvarnishapi separation was still justified:

If we only had libvarnishapi, our installed binaries could shrink (IIUC by ~10%) because we could stop statically linking to libvarnish multiple times, of which most symbols are also already exposed by libvarnishapi (and all of libvarnish is linked into libvarnishapi as well).

Time and time again, we extended the API, and I wonder how much we gain by not just having all libvarnish symbols in the public API - given that we were also not particularly strict about API stability.

So, to be discussed: Should we

  • expose all of libvarnish with libvarnishapi
  • dynamically link to libvarnishapi wherever we link to libvarnish today?

nigoroll avatar Jul 09 '23 12:07 nigoroll

I'm not opposed to the idea of pruning libvarnish.a but that would result in:

  • varnishd linking to libvarnishapi.so
  • new symbols and more frequent soname bumps

We probably also need to clean up certain things like VFIL, but I can probably dig up an old pull request of mine adding the missing bits for VFIL path to be usable outside of the VCC context.

dridi avatar Jul 10 '23 06:07 dridi

bugwash: we will inventory libvarnish APIs here and decide which ones go in libvarnishapi and which ones remain confined in libvarnish. We may create pull requests similar to #3956 for APIs that may require adjustments to become usable.

dridi avatar Jul 17 '23 13:07 dridi

@neufeind to give some context, quite surprisingly to me we actually ended up deciding that we will expose almost all of libvarnish as libvarnishapi. So your suggestion will, in a slightly different manner, become a reality eventually.

nigoroll avatar Jul 17 '23 13:07 nigoroll

Bugwash: Identify parts of libvarnish needed now

nigoroll avatar Aug 28 '23 13:08 nigoroll

Edit: Only VSHA is missing, a previous version of this comment was generated with wrong linker flags.

nigoroll avatar Aug 28 '23 13:08 nigoroll

bugwash: @Dridi volunteered to write up a plan

nigoroll avatar Sep 18 '23 13:09 nigoroll