dconf: Check for changes properly despite style of quotes used by user

[jikamens]

SUMMARY

GVariant strings can use either single or double quotes, and therefore the user can use either single or double quotes when specifying a string value in their task, and what they choose to use may not match what is output by the dconf command, so for accurate comparison we need to strip the quotes and just compare what's inside them.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

dconf

[jikamens]

I am not sure whom to ask to review this. Only @felixfontein and @bcoca have changelog entries in dconf.py, and their changes don't look functional?

[ansibullbot]

cc @azaghal
click here for bot help

[jikamens]

Hmm.

My goal here was to make a minimal change to improve the behavior without risking breaking anything else. However, when I made that calculation I forgot that this would handle a single string but not an array or tuple of strings. That somewhat changes the calculus, and I am wondering if we want to make this a larger change which solves the problem "correctly.".

The way to do that would be to use gi.repository.GLib.Variant, something like this:

from gi.repository.GLib import Variant
from gi.repository.GLib import GError

def variants_are_equal(canonical_value, user_value):
    """Compare two string GVariant representations for equality.

    Assumes `canonical_value` is "canonical" in the sense that the type of the
    variant is specified explicitly if it cannot be inferred; this is true for
    textual representations of variants generated by the `dconf` command. The
    type of `canonical_value` is used to parse `user_value`, so the latter does
    not need to be explicitly typed.

    Returns True if the two values are equal.
    """
    try:
        variant1 = Variant.parse(None, canonical_value)
        variant2 = Variant.parse(variant1.get_type(), user_value)
        return variant1 == variant2
    except GError:
        return canonical_value == user_value

The risk here is that this introduces a new dependency on the gi.Repository.GLib Python library. I don't know how risky this is because I don't know how essential that library is to a system that has dconf installed. At least on Ubuntu if I try to uninstall the package containing this library apt threatens to uninstall 50 packages, so it's not much of a risk, but the package containing dconf isn't one of them, so theoretically at least a system could have dconf installed without the Python library being available.

Perhaps the compromise answer would be to write the code so that gi.repository.GLib is used for full parsing of GVariants if it's available for import and the old string-comparison behavior is used otherwise? Something like:

import sys

try:
    import NoneSuch
    from gi.repository.GLib import Variant
    from gi.repository.GLib import GError
except ImportError:
    print('WARNING: gi.repository.GLib not available, using string '
          'comparison to check value equality ', file=sys.stderr)
    Variant = None
    GError = AttributeError

def variants_are_equal(canonical_value, user_value):
    """Compare two string GVariant representations for equality.

    Assumes `canonical_value` is "canonical" in the sense that the type of the
    variant is specified explicitly if it cannot be inferred; this is true for
    textual representations of variants generated by the `dconf` command. The
    type of `canonical_value` is used to parse `user_value`, so the latter does
    not need to be explicitly typed.

    Returns True if the two values are equal.
    """
    try:
        variant1 = Variant.parse(None, canonical_value)
        variant2 = Variant.parse(variant1.get_type(), user_value)
        return variant1 == variant2
    except GError:
        return canonical_value == user_value

What do you tihink?

And yeah, I should probably write some unit tests. :-/

(It's a good thing I am between jobs right now because otherwise I would not have time to fix this properly. ;-) )

[jikamens]

FYI: https://blog.kamens.us/2023/02/25/the-joy-of-contributing-to-a-well-run-open-source-project/

[jikamens]

OK, I've implemented the proposal above, let me know what you think.

The one concern I have, and I'm not sure this is something I need to worry about or what to do about it if I do, is that the unit tests require the gi.repository Python library to be present.

[jikamens]

Well apparently I do need to worry about that given that a bunch of the checks are failing. I'm not sure how to proceed here. Any suggestions you have would be appreciated. Thanks!

[github-actions]

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

[jikamens]

Since this unit test doesn't really test anything that isn't part of gi.repository.GLib itself, it's probably best to simply remove it.

Rather than removing the unit test, I would prefer to leave it in after adjusting the code so that the tests past even when gi.repository isn't present, so that anyone working on the code in the future can run the unit tests locally with gi.repository present on their machine to test that everything is functioning as intended before their changes are merged. Are you OK with that?

[jikamens]

@felixfontein thanks for your most excellent code review. I think I've responded to everything. Please take a look and let me know what you think. I will squash once you're OK with the changes so we don't send a bunch of cluttery commits to main.

[jikamens]

OK apparently I have to include something in the version field of the module.deprecate call but I'm not sure what the convention is here or what the right thing to do is. Please advise.

[felixfontein]

Looks good to me! I think there should be two follow-up PRs:

1. Use respawning to restart the module with the correct Python interpreter if the module happens to be installed for another Python interpreter, similar to https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/apt.py#L1233-L1286 (except that it shouldn't try to install that module). Respawn functionality has been added in ansible-core 2.11, which happens to be the minimum version supported by this collection. This is useful since it seems likely that gi.repository.GLib is only installed for the system Python interpreter, which might not be the one used by users, and users cannot install gi.repository.GLib with pip.
2. Create a PR that actually deprecates not having that library (by emitting a deprecation warning, and announcing removal for community.general 9.0.0) - I would only merge that to main and not backport it, so it will first appear in community.general 7.0.0.

Are you interested in working on this?

[jikamens]

Are you interested in working on this?

@felixfontein sure why not. Let's get this PR merged and backported and then I will start working on the other issues.

[patchback]

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/627371e2d8cfdd9be43a00e697a0d631036bcb10/pr-6049

Backported as #6145

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@jikamens thanks a lot for working on this! :)