On Wed, Feb 9, 2022 at 11:15 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote: > > 2022-02-09 09:53 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > > On Wed, Feb 9, 2022 at 4:37 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote: > >> > >> 2022-02-08 16:39 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > >>> On Tue, Feb 8, 2022 at 4:07 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote: > >>>> > >>>> Since the notion of versions was introduced for bpftool, it has been > >>>> following the version number of the kernel (using the version number > >>>> corresponding to the tree in which bpftool's sources are located). The > >>>> rationale was that bpftool's features are loosely tied to BPF features > >>>> in the kernel, and that we could defer versioning to the kernel > >>>> repository itself. > >>>> > >>>> But this versioning scheme is confusing today, because a bpftool binary > >>>> should be able to work with both older and newer kernels, even if some > >>>> of its recent features won't be available on older systems. Furthermore, > >>>> if bpftool is ported to other systems in the future, keeping a > >>>> Linux-based version number is not a good option. > >>>> > >>>> Looking at other options, we could either have a totally independent > >>>> scheme for bpftool, or we could align it on libbpf's version number > >>>> (with an offset on the major version number, to avoid going backwards). > >>>> The latter comes with a few drawbacks: > >>>> > >>>> - We may want bpftool releases in-between two libbpf versions. We can > >>>> always append pre-release numbers to distinguish versions, although > >>>> those won't look as "official" as something with a proper release > >>>> number. But at the same time, having bpftool with version numbers that > >>>> look "official" hasn't really been an issue so far. > >>>> > >>>> - If no new feature lands in bpftool for some time, we may move from > >>>> e.g. 6.7.0 to 6.8.0 when libbpf levels up and have two different > >>>> versions which are in fact the same. > >>>> > >>>> - Following libbpf's versioning scheme sounds better than kernel's, but > >>>> ultimately it doesn't make too much sense either, because even though > >>>> bpftool uses the lib a lot, its behaviour is not that much conditioned > >>>> by the internal evolution of the library (or by new APIs that it may > >>>> not use). > >>>> > >>>> Having an independent versioning scheme solves the above, but at the > >>>> cost of heavier maintenance. Developers will likely forget to increase > >>>> the numbers when adding features or bug fixes, and we would take the > >>>> risk of having to send occasional "catch-up" patches just to update the > >>>> version number. > >>>> > >>>> Based on these considerations, this patch aligns bpftool's version > >>>> number on libbpf's. This is not a perfect solution, but 1) it's > >>>> certainly an improvement over the current scheme, 2) the issues raised > >>>> above are all minor at the moment, and 3) we can still move to an > >>>> independent scheme in the future if we realise we need it. > >>>> > >>>> Given that libbpf is currently at version 0.7.0, and bpftool, before > >>>> this patch, was at 5.16, we use an offset of 6 for the major version, > >>>> bumping bpftool to 6.7.0. > >>>> > >>>> It remains possible to manually override the version number by setting > >>>> BPFTOOL_VERSION when calling make. > >>>> > >>>> Signed-off-by: Quentin Monnet <quentin@xxxxxxxxxxxxx> > >>>> --- > >>>> Contrarily to the previous discussion and to what the first patch of the > >>>> set does, I chose not to use the libbpf_version_string() API from libbpf > >>>> to compute the version for bpftool. There were three reasons for that: > >>>> > >>>> - I don't feel comfortable having bpftool's version number computed at > >>>> runtime. Somehow it really feels like we should now it when we compile > >>> > >>> Fair, but why not use LIBBPF_MAJOR_VERSION and LIBBPF_MINOR_VERSION to > >>> define BPFTOOL_VERSION (unless it's overridden). > >> > >> I forgot the macros were exposed, which is silly, because I was the one > >> to help expose them in the first place. Anyway. > >> > >>> Which all seems to be > >>> doable at compilation time in C code, not in Makefile. This will work > >>> with Github version of libbpf just as well with no extra Makefile > >>> changes (and in general, the less stuff is done in Makefile the > >>> better, IMO). > >>> > >>> Version string can also be "composed" at compile time with a bit of > >>> helper macro, see libbpf_version_string() implementation in libbpf. > >> > >> Sounds good, I can do that. > > ... Except that you can only compose so much. The preprocessor won't > allow me to sum libbpf's major version with the offset (6) before > turning it into a string. I need to think about this a bit more. Yeah, it sucks. Well, we can either go back to `make version` or you'll have to do snprintf() to get string representation. 6 + LIBBPF_MAJOR_VERSION should work in #if condition, it just doesn't stringifies to 6, but rather "6 + 0", unfortunately. > > >> > >> This won't give me the patch number, though, only major and minor > >> version. We could add an additional LIBBPF_PATCH_VERSION to libbpf. > >> Although thinking about it, I'm not sure we need a patch version for > >> bpftool at the moment, because changes in libbpf's patch number is > >> unlikely to reflect any change in bpftool, so it makes little sense to > >> copy it. So I'm considering just leaving it at 0 in bpftool, and having > >> updates on major/minor numbers only when libbpf releases a major/minor > >> version. If we do want bugfix releases, it will still be possible to > >> overwrite the version number with BPFTOOL_VERSION anyway. What do you think? > > > > So the patch version is not currently reflected in libbpf.map file. I > > do patch version bumps only when we detect some small issue after > > official release. It happened 2 or 3 times so far. I'm hesitant to > > expose that as LIBBPF_PATCH_VERSION, because I'll need to remember to > > bump it manually (and coordinating this between kernel sources and > > Github is a slow nightmare). Let's not rely on patch version, too much > > burden. > > Agreed, thanks. > Quentin
