GNOME Bugzilla – Bug 747979
tools: Add a g-ir-diff tool
Last modified: 2018-02-08 12:34:39 UTC
Here’s a series of patches to add a new g-ir-diff tool. This is intended to be used to verify that there are no API breaks between one version of a GIR file and the next, for some definition of ‘API break’ which is decided by the library author. While tools exist for checking the stability of a C API between releases (such as the ABI Compliance Checker (http://ispras.linuxbase.org/index.php/ABI_compliance_checker), no such tool exists for GIR APIs. The best I've seen is a `diff -u` of the two GIR files, which is subject to a lot of false positives, and requires the developer to manually determine whether each change is an API break. The ultimate goal is for this to be integrated into the release process for any library which installs a stable GIR file, giving a reliable (in the sense that it is allowed to cause `make distcheck` to fail) indication that a project has broken its introspected API. However, more tooling is needed before that can be a reality — build system integration and a solid set of guidelines for recommended ways of storing old versions of GIR files, for example. One thing which should be considered before the interface for this tool is finalised is how it would be best to fit it into CI systems and `make check`. I think any approach has to have an archive of historical GIR files to compare against — the question is how to best store this. The idea I'm toying with at the moment is to store it in the version control system (i.e. git; I don't care about anything else). If there were some way of extracting the GIR files from each tagged release, it would be easy to compare the current ones in the build tree against those from the most recent release. In the easiest case, the GIR files are stored in git directly (for example, if they are hand-written). In most projects, they are generated (by g-ir-scanner). In this case, I suggest using `git notes` to add the generated GIR files to each release tag in a special notes namespace. They could then be retrieved as trivially as in the easiest case. Of course, this would mean that g-ir-diff needs to be able to read input files from stdin or somesuch other magic, which it doesn’t currently do. --- The attached patches are basically complete as a first draft. More documentation and unit tests could be added, and there are a few minor TODO comments. I’m attaching the patches here to get feedback on the overall concept, and the suggested command line interface. Once I have some feedback, I can polish up the patches ready for actual review.
Created attachment 301704 [details] [review] giscanner: Store parent link in AST for TypeContainer instances It is useful for navigating the AST to have a link from each TypeContainer instance (e.g. each Parameter and Return) to the parent AST node, such as a Function. This will be used in the g-ir-diff tool.
Created attachment 301705 [details] [review] giscanner: Store direction in TypeContainer instance Instead of storing a direction property on both Parameter and Return separately, hoist it up to TypeContainer so it’s inherited. This neatens things up a bit, but doesn’t really change anything in practice.
Created attachment 301706 [details] [review] giscanner: Add a Parameter.name property This is an alias of Parameter.argname, which makes it easier to duck-type debugging of AST nodes by printing out their name property.
Created attachment 301707 [details] [review] tools: Add a g-ir-diff tool This allows two different versions of the same GIR file to be compared, and any API breaks to be highlighted. This can be used to verify that a library has not changed its introspected API between two releases, for example; or that changes to the g-ir-scanner have not caused regressions in publicly introspected APIs. The tool supports three ‘levels’ of output: info, forwards-incompatibilities and backwards-incompatibilities. They can all be disabled or enabled individually. Backwards incompatibilities are where code written against the old version of the API might not work against the new API. Forwards incompatibilities are the converse: code written against the new API may not work against the old. Informational output is more akin to a conventional `diff` call against the files, highlighting differences which should not affect API compatibility, but are interesting anyway. Each possible error has a unique code which can be used to disable it, so projects can choose the exact definition of API compatibility they compare about. The tool is designed to be used as part of a CI or build system, rather than manually, so aims for a zero-false-positive rate, so that it can be used to abort `make distcheck` if an API break is detected, for example.
Review of attachment 301704 [details] [review]: OK.
Review of attachment 301705 [details] [review]: OK.
Review of attachment 301706 [details] [review]: Sold.
It might be interesting to store in git only the *differences* between the defaults. In other words if you add (out) to an int *, that gets recorded. Then to use your diff tool, we generate the gir and verify it has those properties? We could do some of this in GNOME Continuous as well as a layer on top. We are actually today storing the history of GIR files for many projects in the ostree history, although I reserve the right to delete that repo at any time.
(In reply to Colin Walters from comment #8) > It might be interesting to store in git only the *differences* between the > defaults. In other words if you add (out) to an int *, that gets recorded. > > Then to use your diff tool, we generate the gir and verify it has those > properties? What happens if the defaults change at some point? This is not unprecedented for GIR (bug #729660, for example). If the defaults change from $defaults_old to $defaults_new, the GIR generated against $defaults_new cannot be compared against a diff which was taken against $defaults_old. I’m not sure I understand the benefits of storing the diff from the defaults. > We could do some of this in GNOME Continuous as well as a layer on top. We > are actually today storing the history of GIR files for many projects in the > ostree history, although I reserve the right to delete that repo at any time. Maybe we could copy the GIR files from the ostree history into git-notes for all the repos before you next delete the ostree history? That would be a good kickstart. How do you envisage this being used in gnome-continuous? I was planning to integrate it into `make distcheck` (just like I’ve done with dbus-deviation [https://people.collabora.com/~pwith/dbus-deviation/] for D-Bus APIs).
(In reply to Philip Withnall from comment #9) > (In reply to Colin Walters from comment #8) > > It might be interesting to store in git only the *differences* between the > > defaults. In other words if you add (out) to an int *, that gets recorded. > > > > Then to use your diff tool, we generate the gir and verify it has those > > properties? > > What happens if the defaults change at some point? This is not unprecedented > for GIR (bug #729660, for example). If the defaults change from > $defaults_old to $defaults_new, the GIR generated against $defaults_new > cannot be compared against a diff which was taken against $defaults_old. True, though "unlikely" again I would say. > I’m not sure I understand the benefits of storing the diff from the > defaults. Mainly to reduce visual noise in review - if I'm looking at a commit which adds new public API, then I'd also be looking at the XML diff. To a lesser degree, object storage cost. Anyways...I'm not strongly objecting to just storing the literal GIR, but we should at least consider alternatives. > How do you envisage this being used in gnome-continuous? I was planning to > integrate it into `make distcheck` (just like I’ve done with dbus-deviation > [https://people.collabora.com/~pwith/dbus-deviation/] for D-Bus APIs). Continuous will never run distcheck, but we could figure something out there. There are a number of options. None of this blocks landing a tool for generating the diff.
(In reply to Colin Walters from comment #10) > > I’m not sure I understand the benefits of storing the diff from the > > defaults. > > Mainly to reduce visual noise in review - if I'm looking at a commit which > adds new public API, then I'd also be looking at the XML diff. To a lesser > degree, object storage cost. I think git only shows notes in diffs and log output if you pass --show-notes? But that’s a reasonable argument. I think object storage cost is a minor argument, although looking in $datadir/gir-1.0, I’m surprised to see .gir files as big as 2MB. Git should compress that fairly well though. > Anyways...I'm not strongly objecting to just storing the literal GIR, but we > should at least consider alternatives. Overall, I think it’s less risky to just store the entire GIR file. I quite like the fact that means it can be used by other tools without needing g-ir-diff to re-generate the .gir file. > > How do you envisage this being used in gnome-continuous? I was planning to > > integrate it into `make distcheck` (just like I’ve done with dbus-deviation > > [https://people.collabora.com/~pwith/dbus-deviation/] for D-Bus APIs). > > Continuous will never run distcheck, but we could figure something out > there. There are a number of options. > > None of this blocks landing a tool for generating the diff. OK. Do you have any thoughts about the current command line interface for g-ir-diff in attachment #301707 [details], or shall I just do another iteration with general polish? It might take me a few days to find time for this.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gobject-introspection/issues/130.