Re: [libvirt PATCH] wireshark: Don't include config.h

On Fri, 2020-09-04 at 11:50 +0200, Michal Prívozník wrote: > On 9/4/20 10:33 AM, Andrea Bolognani wrote: >> I should have pushed the branch to GitLab first: >> >> https://gitlab.com/abologna/libvirt/-/pipelines/185421025 >> >> We might be able to adopt a more nuanced approach, where we use >> ws_version.h where available and fall back to config.h where it's the >> only option, but I'm not quite sure how to implement that in Meson >> and can't spare the time to learn right now. >> >> Are either you or Michal interested in giving it a try? > > I had this patched marked for review but did not get to it yesterday, sorry. No need to apologise :) > I understand your reasoning and agree that the current code looks bad. > But this is just the way it used to be when you wanted to built plugins > outside of wireshark. I had a long discussion with wireshark devels when > our plugin was being written. Part of them did not want other projects > to build plugins from outside at all (which didn't really work for us > because our RPC was changing a lot back then and we would have to wait > full release cycle of wireshark to dissect new procedures), and part was > at least willing to merge my patches that made our code less horrible. > > And truth to be told, I did not really watch the discussion on wireshark > end since and with your patch it looks they moved towards making it > easier for other to build plugins out of wireshark's source. > > End of history class. > > The patch looks good. ws_version.h was introduced with 2.9.0 release > which is 1.5 years old. Given that the dissector is aimed mostly on us, > developers to help us debug RPC issues, I think we can safely bump the > minimal wireshark version required (currently 2.4.0 which is 3 years old). That sounds reasonable in theory, but if you look at https://gitlab.com/abologna/libvirt/-/pipelines/185421025 you'll see that even platforms that ship pretty recent Wireshark[1] don't include ws_version.h among the headers. Not building the dissector on those non-obsolete platforms seems excessively harsh, so I think an approach similar to the one I described above is still necessary. And at that point, you might as well not bump the minimum required version and keep building the dissector on the current list of platforms... [1] Debian sid has 3.2.6 and Ubuntu 20.04 has 3.2.3