Add version query method (`xkb_get_version`)

[Caellian]

There's currently no way to use multiple versions of libxkbcommon with standard dynamic linking. You're forced to use dlopen if you're going to support both Arch (on version 1.13.0 atm) and Ubuntu (on version 1.7.0).

This is very inconvenient because now you're forced to manually load all the symbols and check whether they exist to figure out which version you're on. There's versions that don't differ in exposed symbols though, so you're actually supposed to query the system package database (?) to deal with breaking changes. Or just assume you don't have access to functionality newer than 1.7.0.

In my case (Rust bindings) I want to bind xkb_keymap_new_from_names2, but return a VersionNotSupported error if the specified format is too new and fall back on xkb_keymap_new_from_names.

Solution

Provide a global xkb_get_version function that returns a pointer to a static version triplet (e.g. [uint8; 3]) which would allow the libraries to query the installed version of the library on the system and change some logic based on that.

[wismill]

This is very inconvenient because now you're forced to manually load all the symbols and check whether they exist to figure out which version you're on. There's versions that don't differ in exposed symbols though, so you're actually supposed to query the system package database (?) to deal with breaking changes. Or just assume you don't have access to functionality newer than 1.7.0.

AFAIK, querying the symbols is the most robust method. What if we deleted a symbols after some deprecation period and your bindings are not synced?

Provide a global xkb_get_version function that returns a pointer to a static version triplet (e.g. [uint8; 3]) which would allow the libraries to query the installed version of the library on the system and change some logic based on that.

I would rather make the private API xkb_keymap_is_supported_format() public.

xkb_get_version() is not a good idea, because you get the burden to ensure to be in sync with the latest version you support. What if we deprecate then delete a keymap version?

[Caellian]

What if we deleted a symbols after some deprecation period and your bindings are not synced?

The way they're currently written (xkbcommon-rs) is without any guards anyway and simply assume 1.7.0 ABI of your library works.

Adding version information simply gives safe access to newer functions (if one isn't running Ubuntu).

I can add backwards compatibility for older versions in run environment, adding forwards compatibility is not possible. If I'm right in assuming libxkbcommon shouldn't be statically linked, I can't handle build env & run env differences at compile time beyond checking for version.

I would rather make the private API xkb_keymap_is_supported_format() public.

But I couldn't use that on Ubuntu (on v1.7.0) anyway. I can only use symbols to figure out xkb_keymap_new_from_names2 isn't available and that I should call now-deprecated xkb_keymap_new_from_names and return detailed errors for TEXT_V2.

Currently I'm doing this:

pub static VERSION: std::sync::LazyLock<[u8; 3]> = std::sync::LazyLock::new(|| {
    #[allow(unused_assignments)]
    let mut result = super::BUILD_VERSION;

    #[cfg(feature = "dynamic")]
    {
        result = [1, 7, 0];
        unsafe {
            let library = libloading::Library::new("libxkbcommon.so").ok()
                .or_else(|| libloading::Library::new("xkbcommon").ok())
                .expect("unable to load libxkbcommon");

            const FUNCTIONS: &[(&[u8], [u8; 3])] = &[
                (b"xkb_keysym_to_upper\0", [1,8,1]),
                (b"xkb_components_names_from_rules\0", [1,9,2]),
                (b"xkb_keymap_mod_get_mask\0", [1,10,0]),
                (b"xkb_rmlvo_builder_unref\0", [1,11,0]),
                (b"xkb_keymap_get_as_string2\0", [1,12,3]),
            ];

            // Yes, this will break if any of the above functions are removed
            for (name, version) in FUNCTIONS {
                if library.get::<*const core::ffi::c_void>(*name).is_ok() {
                    result = *version;
                } else {
                    break;
                }
            }

            if result[0] == 1 && result[1] == 12 && result[2] >= 3 && try_keysym_from_name("XF86MediaPlayPause", KeysymFlags::empty()).ok().flatten().is_some(){
                result = [1, 13, 0];
            }
        }
    }

    result
});

In order to be able to do this:

    pub fn new_from_names(
        context: &Context,
        rule_names: &RuleNames,
        format: KeymapFormat,
        flags: KeymapCompileFlags,
    ) -> Result<Keymap, CompileError> {
        // SAFTY: RuleNames is borrowed and can't be moved before xkb_keymap_new_from_names is done
        let rule_names = unsafe { rule_names.as_raw() };

        if !format.supported().unwrap_or(true) {
            return Err(CompileError::FormatNotSupported(format));
        }

        #[cfg(not(LIBXKBCOMMON_1_11))]
        let ptr = unsafe { xkb_keymap_new_from_names(context.ptr, &rule_names, flags.bits()) };
        #[cfg(LIBXKBCOMMON_1_11)]
        let ptr = if runtime_version!(< 1 . 11) {
            unsafe { xkb_keymap_new_from_names(context.ptr, &rule_names, flags.bits()) }
        } else {
            unsafe { xkb_keymap_new_from_names2(context.ptr, &rule_names, format.0, flags.bits()) }
        };

        if ptr.is_null() {
            Err(CompileError::Compile)
        } else {
            Ok(Keymap { ptr })
        }
    }

It's nasty, but it's also the only way to ensure bindings will work when either built in Ubuntu CI, and when built on Arch but executed on Ubuntu.

It would be nice if I could use some function for later versions instead of checking each symbol individually. Or worse yet, checking symbols inside every function that varies across versions .

[mahkoh]

If you want to claim semver, then deleting a function requires bumping the major. Otherwise application that link at compile time will stop compiling. Similarly, dropping support for any existing keymap format is not possible.

If you wanted to require application to load symbols dynamically at runtime and to query keymap format support at runtime, then that would be possible. That is what vulkan does. But this library currently doesn't do either of these things so they must be supported until the major version is bumped.

[Caellian]

You're right, instead of versions I should be checking for capabilities and release a major when something is removed as well.

I don't personally care about xkb_keymap_is_supported_format, but someone downstream might. It's up to your judgement whether it'd be a sensible addition - I think it could be useful and doesn't hurt to expose it.

In my code, I had to write

pub fn supported(&self) -> Option<bool> {
        match *self {
            Self::TEXT_V1 => Some(true),
            #[cfg(LIBXKBCOMMON_1_11)]
            Self::TEXT_V2 => Some(runtime_version!(>= 1 . 11)),
            Self::USE_ORIGINAL => Some(true),
            _ => None,
        }
    }

because newer/unknown versions of the format can't be verified - so result must be Option<bool>. Adding xkb_keymap_is_supported_format would mean that function can return bool, so it does make the result more precise, even though TEXT_V2 has to be checked via xkb_keymap_new_from_names2 symbol for 1.11.0-1.14.0.

[mahkoh]

You're right, instead of versions I should be checking for capabilities and release a major when something is removed as well.

I was talking about libxkbcommon not your wrapper.

[Caellian]

I was talking about libxkbcommon not your wrapper.

I know, but what you've said applies to the wrapper as well - I could polyfill removed functions for later versions, but if they were removed from libxkbcommon then it doesn't make sense for the bindings to reimplement them either.

I check for version with pkg-config during build, so I can simply assert version < 2.0.0

[mahkoh]

A new major version of libxkbcommon will have to be distributed as a separate library so that check should be ineffective. A version check as you're proposing is only useful for checking the minor version.

[wismill]

If you want to claim semver, then deleting a function requires bumping the major. Otherwise application that link at compile time will stop compiling. Similarly, dropping support for any existing keymap format is not possible.

Thank you master @mahkoh, but I do know what semver means. I merely pointed that using xkb_get_version() is weaker than checking for the symbols and requires to keep the wrapper synced.

[wismill]

Tip

There is a simple cross-version way to check if the keymap format is supported: compile a dummy minimal keymap with xkb_keymap_new_from_buffer() and check the result.

Again, it’s not like we are going to publish a new keymap format at every release. I have no plan to introduce a new format after v2. The current plan is rather to facilitate the migration to v2.

If we ever add a v3 format, I wish that it will be totally different and easier to parse, together with a Wayland format version negotiation.

[wismill]

A new major version of libxkbcommon will have to be distributed as a separate library so that check should be ineffective. A version check as you're proposing is only useful for checking the minor version.

@mahkoh This is a distribution decision. It is not mandatory if the apps compile (optionally patched).

To be clear: there is currently no plan to remove functions nor alter the enums in a non-backward compatible way, it is merely a thought experiment to discuss the utility of xkb_get_version().

[mahkoh]

Thank you master @mahkoh, but I do know what semver means.

That is good.

I merely pointed that using xkb_get_version() is weaker than checking for the symbols

Using detection mechanisms that are not based on symbol availability is required to check for the availability of new flags. For example, if a new flag were added to xkb_context_new, checking the version would be the only way to see if the flag is supported. In particular, xkb_context_new does not return an error if an unsupported flag is used.

Looking at the version is all around simpler and it is the obvious choice since applications that link against xkbcommon at compile time also change their behavior/used symbols based on the pkg-config version.

and requires to keep the wrapper synced.

This is no different from a wrapper based on compile-time linking that uses the pkg-config version. Wrapping new functionality made available in new versions requires changing the wrapper.

[wismill]

xkb_context_new does not return an error if an unsupported flag is used.

Maybe this is rather a bug to fix. See #936.

Looking at the version is all around simpler

@mahkoh This is debatable: if the all the functions reject unsupported flags, then one just have to check for failure instead of maintaining a compatibility table.