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.

[mahkoh]

These functions only return a null pointer in the error case which means that, if the call returns null, you have to try up to 2^N calls to check if the failure is caused by flags or if something else is wrong. Checking first without any flags is not viable since flags might change the behavior in a way that causes function call fails even if the flag is valid.

[wismill]

These functions only return a null pointer in the error case

Yeah, we do not have more details except in the log. I am trying to change that in the new functions. Ideally we should provide some structured error. WIP

which means that, if the call returns null, you have to try up to 2^N calls to check if the failure is caused by flags or if something else is wrong. Checking first without any flags is not viable since flags might change the behavior in a way that causes function call fails even if the flag is valid.

While this is true in theory, in practice this is simply not the case: the only enum[^1] that changed since 1.7 is the keymap format. Any new enum can be used only by new functions too, so if the symbol is not available then the flags are not available either. So first check the symbols, then the flags.

In the case of the keymap format, I already pointed above a way to test it reliably. It would only error if the format is not supported or on an allocation error.

[^1]: Checked on mobile, correct me if I miss some.

[Caellian]

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.

Yeah, but in practice this means instead of testing if version > 1.20.0, I have to construct Context or (worst case) Context > Keymap > State a dozen times to query which flags are available.

Also, Rust bindings at the moment produce opaque errors because xkb_context_set_log_fn requires compiling a trampoline function in C to avoid varargs. I can't tell what the error was even by parsing strings.

[wismill]

Yeah, but in practice this means instead of testing if version > 1.20.0, I have to construct Context or (worst case) Context > Keymap > State a dozen times to query which flags are available.

No drama please: there is only the keymap format issue at the moment and no flag has been removed (nor is it planned).

Also, Rust bindings at the moment produce opaque errors because xkb_context_set_log_fn requires compiling a trampoline function in C to avoid varargs. I can't tell what the error was even by parsing strings.

Do you have a concrete example?

[Caellian]

No drama please: there is only the keymap format issue at the moment and no flag has been removed (nor is it planned).

I'm not being dramatic. I'm saying access to versions allows checking which flags are supported. Most flag enums are empty at the moment, but they were added to allow room for growth in future. However, checking which are supported is not easy without constructing (in worst case) State which requires both Context and Keymap to be constructed as well - this implies calling a parser and a lot of other code to just get a boolean value which is known statically internally.

State only has functions with xkb_state_component, which are unlikely to change much anyway. It's just an example though. It's not clear to my why one would need to compile a dummy Keymap file, to check xkb_keymap_compile_flags.

For this sort of thing, there could exist functions for checking all the flags and arguments, but that would probably add to much bloat.

Do you have a concrete example?

See #937.

[wismill]

There is only one value in one enum that is problematic ATM, and there is already a backward compatible workaround. I get your point about the difficulty for the checking (in general) the first time, no need to repeat it.

What I mean is that ATM all of this is very speculative (no value relevant to test apart the format keymap) and that we can design a proper solution hopefully soon.

[Caellian]

There is only one value in one enum that is problematic ATM [... and] all of this is very speculative

just [...] check for failure instead of maintaining a compatibility table

isn't a good long term solution.

For instance, there's 3+ points of failure in implementation for xkb_keymap_new_from_names2 at the moment:

• N flags can be unsupported (currently 0 for keymap)
• keymap source could be malformed
  • unsupported version (TEXT_V2)
  • actually bad source
• out of memory (e.g. calloc of keymap struct fails)

All of these return NULL, and today there is no programmatic way to distinguish between them.

This is already a problem for bindings - and it becomes worse as additional flags or formats appear. You're suggesting a wrapper or a GUI toolkit to re-invoke the same function multiple times to “probe” the cause:

• try without flags
• try with fewer flags
• try fallback format
• try any other combination that makes sense
• eventually realize the real cause was OOM and abort

This is brittle and requires downstream to manually handle errors, which can't be exhaustive, because as API expands more things will be able to go wrong.

This is why I’m pushing for two things that complement each other:

• A way to determine feature support and anticipate errors (your #935, or xkb_keymap_is_supported_format) - prevents unnecessary probing calls
• A structured error mechanism (#937) - allows downstream consumers to react cleanly without heuristics or string-parsing.

Even today, these two mechanisms would make it a lot easier to write robust code. And they become only more important as new formats/flags are added later.

[wismill]

I GET YOUR POINT.

It seems you still do not get mine:

1. There is already a workaround (not great, but there)
2. Thus there is no rush.
3. Let's design a sustainable solution.

My current direction is some int xkb_is_supported_enum_value(enum xkb_enum e, int value), which would enable to test:

• an enum existence
• an enum's value (or values for flags) support

Return value could be:

• -1 for unsupported enums
• 0 if both enum and value are supported
• 1 if a non-flag value is not supported, or the unsupported flags mask

So the function succeeds if it returns 0 and is a single entry point for all enums.

[Caellian]

I GET YOUR POINT.

Sorry. I thought you didn't.

If you're gonna go with xkb_is_supported_enum_value, then I suggest you generate it to reduce the risk of someone accidentally forgetting to add variants to it.

I used ChatGPT to generate awk (more limited) and python scripts that dump:

xkb_rmlvo_builder_flags XKB_RMLVO_BUILDER_NO_FLAGS
xkb_keysym_flags XKB_KEYSYM_NO_FLAGS XKB_KEYSYM_CASE_INSENSITIVE
xkb_context_flags XKB_CONTEXT_NO_FLAGS XKB_CONTEXT_NO_DEFAULT_INCLUDES XKB_CONTEXT_NO_ENVIRONMENT_NAMES XKB_CONTEXT_NO_SECURE_GETENV
...

when you give them xkbcommon.h file. Tested them with malicious examples of nested comments in code and removed some other issues, so they should work (awk one handles less though). This is a large chunk of automatic xkb_is_supported_enum_value generation done, so hopefully it helps.

I didn't go with a proper parser (e.g. clang, pyparsing, etc.) because that makes the build process a lot more complicated, slower or doesn't work (pyparsing fails for stdlib). So assuming standard C syntax (no weird #defines after enum kw), it will work.

[wismill]

@Caellian I wrote an implementation of xkb_is_supported_enum_value in #942 that uses libclang for proper parsing. The difference with your proposal is that it not part of the build process, rather update have to be done manually and the CI checks it is up to date. That way we avoid a new build dependency.