ostreedev / ostree

Operating system and container binary deployment and upgrades
https://ostreedev.github.io/ostree/
Other
1.26k stars 291 forks source link

OstreeRepoListRefsExtFlags GIR changed #1243

Open dbnicholson opened 6 years ago

dbnicholson commented 6 years ago

The GIR for OstreeRepoListRefsExtFlags changed from enumeration to bitfield. Here's what I have from a 2016.15 system:

    <enumeration name="RepoListRefsExtFlags"
                 c:type="OstreeRepoListRefsExtFlags">
      <member name="repo_list_refs_ext_none"
              value="0"
              c:identifier="OSTREE_REPO_LIST_REFS_EXT_NONE">
        <doc xml:space="preserve">No flags.</doc>
      </member>
    </enumeration>

And from a 2017.11 system:

    <bitfield name="RepoListRefsExtFlags" c:type="OstreeRepoListRefsExtFlags">
      <member name="none"
              value="0"
              c:identifier="OSTREE_REPO_LIST_REFS_EXT_NONE">
        <doc xml:space="preserve">No flags.</doc>
      </member>
      <member name="aliases"
              value="1"
              c:identifier="OSTREE_REPO_LIST_REFS_EXT_ALIASES">
        <doc xml:space="preserve">Only list aliases.  Since: 2017.10</doc>
      </member>
      <member name="exclude_remotes"
              value="2"
              c:identifier="OSTREE_REPO_LIST_REFS_EXT_EXCLUDE_REMOTES">
        <doc xml:space="preserve">Exclude remote refs.  Since: 2017.11</doc>
      </member>
    </bitfield>

These were built on different systems, so perhaps this is a change in g-ir-scanner. However, maybe this change was due to 7ed881b.

cgwalters commented 6 years ago

Hm yes...fun. It was indeed that commit. The values are ABI compatible from a C perspective, but not IABI obviously. The value was named Flags but was missing one of the magic bits like /* flags */ or just using left shifts to convince gtk-doc/g-ir-scanner that it was flags.

cgwalters commented 6 years ago

Did you have a static analysis tool that noticed this IABI change, or did a script actually break?

dbnicholson commented 6 years ago

I was just testing one of our python scripts that uses OSTree.RepoListRefsExtFlags.REPO_LIST_REFS_EXT_NONE and it broke when I ran it against a newer ostree.

cgwalters commented 6 years ago

OK, ugh. So this was out in 2017.11, which is now two releases ago. Still though, I think my tentative vote is to revert the API change here and do a new release where it's an enum. Then we should probably add a new API?

dbnicholson commented 6 years ago

I don't mind working around it, but I have no idea if this is affecting anyone else.

cgwalters commented 6 years ago

If it's an easy change for you to work around (and I assume it is)...eh, maybe we just live with it and learn our lesson? I could be convinced either way.

dbnicholson commented 6 years ago

Sure, it's not a big deal. I just copied to a local enum for now:

class RepoListRefsExtFlags(IntEnum):
    """Compatibility enumeration for OSTree.RepoListRefsExtFlags

    The type changed incompatibly in 2017.11, so we provide our own to
    be safe across versions.
    """
    # No flags
    NONE = 0
    # Only list aliases.  Since: 2017.10
    ALIASES = (1 << 0)
    # Exclude remote refs.  Since: 2017.11
    EXCLUDE_REMOTES = (1 << 1)

I'll switch back to the real thing after we've got 2017.11 running everywhere. I tried to do it dynamically (check whether it derives from GEnum or GFlags), but I couldn't quite get pygobject to give me access to those types to do comparisons with. I figured it would be easy enough to keep this in sync for now since I'm only using NONE right now, anyways.