This repository was archived by the owner on Feb 16, 2026. It is now read-only.
support for externally built vtest with libvtc_varnish.so and libvtc_builtwith.so#4406
Open
nigoroll wants to merge 2 commits intovarnishcache:masterfrom
Open
support for externally built vtest with libvtc_varnish.so and libvtc_builtwith.so#4406nigoroll wants to merge 2 commits intovarnishcache:masterfrom
nigoroll wants to merge 2 commits intovarnishcache:masterfrom
Conversation
d361ecd to
95f62b3
Compare
…with" As we no longer compile vtest with every build, we can no longer use code in vtest core to inform tests about compile time settings. We move these to a new vtest extension which we do build. Ref varnishcache#4398
95f62b3 to
b155963
Compare
Member
Author
|
I noticed that I was making the life of reviewers unnecessarily hard by not providing at least a draft of a version without the submodule. So here it is: https://github.com/nigoroll/varnish-cache/tree/vtest_ext_varnish_nosubmodule |
Member
Author
|
bugwash feedback: avoid the extra |
Member
Author
|
It seems that my explanation of this PR was not good enough, or too long, or whatever, but apparently it is not clear what this PR does and doesn't do:
The edit: I wrongly wrote "fork/exec" wrapper. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the option to use an externally built vtest binary as agreed in #4398.
Demo
Install VTest2 with extension support
extbranch from Add extension support vtest/VTest2#7 such that vtest is found in$PATH(the example here is intended to show a "canonical" way as with external packaging, I would not normally install unter/usr):quick check:
Build vinyl-cache in out-of-tree vtest mode"
This can be later be done by not cloning the submodule (because the TODO items are not complete, see below), but for now we simulate a missing submodule by deleting the file which we look for to determine if the submodule is present:
slink@haggis21:~/Devel/varnish-git/varnish-cache (vtest_ext_varnish)$ rm bin/varnishtest/vtest2/src/vtc_main.cNow build as always, e.g.
All tests should succeed
Implementation / Explanation
vtest/VTest2#7 adds to VTest the option to load a shared object at runtime, which can then extend vtest functionality by registering additional commands.
Moving vinyl test functionality to
libvtc_varnish.soThe first commit changes the build to create such a shared object with the vinyl-specific test mechanics in
vtc_varnish.c,vtc_vsm.candvtc_logexp.c.With this, calling
vtest -E libvtc_varnish.sogives us a the same functionality we had before with the static build.To keep the rest of the build simple, a wrapper program called ... (you guessed it) ...
varnishtestis added which simply callsvtestwith the extension argument. This is used in favor of the simple shell script 2-liner which could also easily fulfill the task in order to be able to get thelibtoolwrapper script for uninstalled libraries. This in turn requires linking to an empty dummy library.Moving compile-time flag checks / macros to
libvtc_builtwithvarnishtestalready works for the most part with just the above, but we have just broken much of thefeaturefunctionality and thepkg_versionandpkg_branchmacros: These can only work if the respective code is built together with the code base it is supposed to reflect on - otherwise we get behavior based on howevervtestwas compiled, but not how vinyl-cache was compiled.So we apply the same idea again and move these checks to a vtest extension with we do compile each time. The outcome is a working
varnishtestas demonstrated.Alternatives considered
We could do without the extra
libvtc_builtwithand just move the respective code intolibvtc_varnish, but I think this functionality might also be of general interest to other projects and I would add this as an example to VTest2 itself if we decide that we want to go this route.The
builtwithchecks could also be replaced with compile time configuration of the tests to execute. I actually have working code which moves all the-spersistentchecks to a subdirectory oftestand includes these tests only if--with-persistent-storageis given toconfigure.But in particular with negated tests I think this quickly becomes really tedious, and the information which tests to run is no longer nicely contained in the tests itself (unless we add something like magic comments which we grep for...). All in all, the "feature test" way is much clearer and easier to maintain.
In general, another alternative would be to invert the sense of what is an executable and what is a library: Create a
libvtest.soin theVTest2project and then an executable in vinyl which only has minimal code to call the library and extend it. In my mind this simply seemed like the wrong way around and I found the extension model very clear, but opinions might vary and at this point, without having written the actual code, I would think that the alternative should also work.TODOs
A main TODO item is that that switch to the new model is not complete, and deliberately so:
For one, I think that keeping vtest bundled as a submodule will make the life for most of the developers easier. The problem to solve is to enable a build from git with an external vtest, and I see no strong reason to destroy the existing workflow.
Secondly, the vinyl specific code still needs to be checked into the vinyl-cache repo again and removed from Vtest2.
Thirdly, the
featuretests which have now been added tobuiltwithneed to be removed from the former.But all of this only makes sense after the changes are accepted.
Notes
make diststill requires the submodule, and rightly so: Again, this addition is to support builds from git only, not to destroy previous workflows.