-
Notifications
You must be signed in to change notification settings - Fork 381
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
Add VMOD version information to Panic output #4077
base: master
Are you sure you want to change the base?
Conversation
Does it make sense to take it one step further and introduce a way for the vmod author add information to the panic? Btw, this change seems to break a couple of CCI jobs: /bin/python3.6 ../../../lib/libvcc/vmodtool.py --boilerplate -o vcc_debug_if ../../../vmod/vmod_debug.vcc
Traceback (most recent call last):
File "../../../lib/libvcc/vmodtool.py", line 1315, in <module>
runmain(i_vcc, opts.rstdir, opts.output)
File "../../../lib/libvcc/vmodtool.py", line 1280, in runmain
v.mkcfile()
File "../../../lib/libvcc/vmodtool.py", line 1263, in mkcfile
self.vmod_data(fo)
File "../../../lib/libvcc/vmodtool.py", line 1215, in vmod_data
fo.write('\t.version =\t%s,\n' % json.dumps(self.version()))
File "../../../lib/libvcc/vmodtool.py", line 1188, in version
for pkgstr in open(os.path.join(srcdir, "Makefile")):
FileNotFoundError: [Errno 2] No such file or directory: '../../../vmod/Makefile' |
lib/libvcc/vmodtool.py
Outdated
# from varnish-cache include/generate.py | ||
def version(self): | ||
srcdir = os.path.dirname(self.inputfile) | ||
|
||
for pkgstr in open(os.path.join(srcdir, "Makefile")): | ||
if pkgstr[:14] == "PACKAGE_STRING": | ||
break | ||
pkgstr = pkgstr.split("=")[1].strip() | ||
|
||
gitver = subprocess.check_output([ | ||
"git -C %s rev-parse HEAD 2>/dev/null || echo NOGIT" % | ||
srcdir], shell=True, universal_newlines=True).strip() | ||
|
||
return pkgstr + " " + gitver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't ship that to third-party VMOD authors, this is very specific to this repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um - how is it specific?
Using autotools is very common, as is git. The git
command received a slight modification to be more agnostic to the directory structure (-C
instead of --git-dir=
). It is important that anything like this works out of the box for most cases, and I think we achieve this goal.
Where I do absolutely agree is that the code should be tolerant and not fail. I got this wrong in the first iteration.
It breaks all jobs running If anything, we should probably have a Projects using autoconf can rename their VCC files to And autoconf or not, we let VMOD authors figure what they want to put in the version information, and how. |
Yes, again, this was an oversight, sorry.
We sure can.
Please, no. Let's make this just work. Yes, we can have an override, but let's not push the burden downstream. Dridi, you did the project a great service improving the m4 macros and with vcdk, which simplify vmod infrastructure, and we should absolutely follow along that route. As someone diagnosing vmod issues, I do not want to depend on vmod authors to add infrastructure just to get this basic information. I want to know the vmod version and git rev if I can get it. If I can not get it for some cases, ok, we can handle that. As a VMOD author, I do not want more maintenance burdon. If we can provide a simple solution for most cases, please let's do this. And then provide a custom option for anything else. |
bugwash feedback summary:
|
FTR: This comment put this back on top of my table because oob communication clarified that users of pre-built containers do not know which vmod versions were used, exactly. |
b1270b3
to
ae59be9
Compare
I have made changes according to the bugwash feedback and force-pushed an update:
This is now done via the
Done
Done via an ELF section such that
This was already contained. I have also changed the |
This commit adds a facility which I had wanted for a very long time, and should have added much earlier: Analyzing bug reports with VMODs involved used to be complicated by the fact that they typically did not contain reliable version information. We add to vmodtool.py the generic code to create version information from generate.py with minor modifications. It adds the version set for autotools and, if possible, the git revision to the vmodtool-generated interface code. We copy that to struct vrt and output it for panics and the debug.vmod CLI command. For builds from distributions, save the git revision to a file. Being at it, we polish the panic output slightly for readability. There are two elements to the version information: - "version", which is intended to be user-friendly and - "vcs" (for version control system) which is intended to represent a commit id from the scm. The VCC file Stanza $Version overrides "version". As an example, the output from a panic now might look like this: vmods = { debug = {p=0x7f5b19c2c600, abi=\"Varnish trunk 172e7b43a49bdf43d82cc6b10ab08362939fc8a5\", vrt=19.1, vcs=\"172e7b43a49bdf43d82cc6b10ab08362939fc8a5\", version=\"Varnish trunk\"}, }, The debug.vmod CLI outout is, for example: 1 vtc (path="/home/slink/Devel/varnish-git/varnish-cache/vmod/.libs/libvmod_vtc.so", version="Varnish trunk", vcs="172e7b43a49bdf43d82cc6b10ab08362939fc8a5") And, finally, a readelf example to extract the vcs string: $ readelf -p.vmod_vcs ./vmod/.libs/libvmod_std.so String dump of section '.vmod_vcs': [ 0] 172e7b43a49bdf43d82cc6b10ab08362939fc8a5
ae59be9
to
47c1e3e
Compare
Once again, I ran into a situation where the exact version of a vmod was unclear, so I would really appreciate if we made some progress here. I also rebased the PR. |
This commit adds a facility which I had wanted for a very long time, and should have added much earlier: Analyzing bug reports with VMODs involved used to be complicated by the fact that they typically did not contain reliable version information.
We add to vmodtool.py the generic code to create version information from generate.py with minor modifications. It adds the version set for autotools and, if possible, the git revision to the vmodtool-generated interface code. We copy that to struct vrt and output it for panics.
Being at it, we polish the panic output slightly for readability.
As an example, the output from a panic now might look like this: