Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose almost all of libvarnish as libvarnishapi #3946

Open
neufeind opened this issue Jun 25, 2023 · 10 comments
Open

Expose almost all of libvarnish as libvarnishapi #3946

neufeind opened this issue Jun 25, 2023 · 10 comments
Assignees

Comments

@neufeind
Copy link

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 84d7912)

Operating system

Rocky Linux 9

Source of binary packages used (if any)

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

@nigoroll nigoroll self-assigned this Jun 26, 2023
@nigoroll
Copy link
Member

nigoroll commented Jun 26, 2023

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.

@neufeind
Copy link
Author

neufeind commented Jul 3, 2023

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 :-)

@dridi
Copy link
Member

dridi commented Jul 3, 2023

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.

@nigoroll
Copy link
Member

nigoroll commented Jul 9, 2023

@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?

@dridi
Copy link
Member

dridi commented Jul 10, 2023

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
Copy link
Member

dridi commented Jul 17, 2023

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.

@nigoroll nigoroll changed the title Add libvarnish.la to varnish-devel packages Expose almost all of libvarnish as libvarnishapi Jul 17, 2023
@nigoroll
Copy link
Member

@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
Copy link
Member

Bugwash: Identify parts of libvarnish needed now

@nigoroll
Copy link
Member

nigoroll commented Aug 28, 2023

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

nigoroll added a commit that referenced this issue Aug 28, 2023
@nigoroll
Copy link
Member

bugwash: @dridi volunteered to write up a plan

@nigoroll nigoroll assigned dridi and unassigned nigoroll Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants