On Thu, Feb 03, 2022 at 05:52:10PM -0500, John Snow wrote: > On Thu, Feb 3, 2022 at 5:40 AM Daniel P. Berrangé <berra...@redhat.com> wrote: > > > > On Wed, Jan 26, 2022 at 01:58:19PM -0500, John Snow wrote: > > > > As you mention though, bear in mind that a command returning > > nothing today, might return something tomorrow. IOW, we'd be > > going from a empty dict to a non-empty dict. If you use "None" > > then it'd be gonig from None to a non-empty dict. > > > > I think you can argue both of these approaches are backwards > > compatible. The python app is not likely to be looking at > > the return type at all initially - unlike C, errors get > > raised as exceptions, so you don't need to look at return > > type to distinguish succes from failure. > > > > So I'd consider it merely a documentation problem to say > > that a "None" return type might later change to a dict. > > Dunno how you represent that in python type annotations, > > but I presume there's a way. > > I don't think type hints offer a temporal dimension to them yet :) > > I started writing a much lengthier response, but the subject of > compatibility is really complex and I am not prepared to give a > comprehensive response to some of the issues you raise ... so instead > I will say "Wow, good points!" and I will get back to you on some of > it. A lot of things will only make sense if we are talking about a > very specific type of project, with very specific goals that are > clearly established. I don't really have that ready, here; I am just > experimenting to learn where some of the pain points are, still. > > So... I'll get back to you on this. > > > We do allow fields to be deleted, which is a *non-compatible* > > evolution, but they MUST have been marked as deprecated for > > 2 cycles first. > > Good point. > > > I'd say sorting required vs optional arguments is doomed as > > a strategy. Stuff that is mandatory today can be made optional > > tomorrow and I think that's reasonable to want todo as we > > evolve an API design. > > Also a good point. Python requires all mandatory arguments precede all > optional ones, so you're probably right that in order to maximize > cross-version compatibility, keyword-only arguments for *all* > arguments both mandatory and optional is the only real way to fly. > > I think this might cause problems for Marc-Andre in rust/dbus land, > but it's tractable in Python. I am unclear on the ramifications for > golang. (Victor, Marc-Andre, any input here?)
Well as a general point, if we consider usage outside of qemu.git, I'm far from convinced that generating code from the schema as it exists today is going to be broadly usable enough to make it worthwhile. The problem is precisely that the code that is generated is only ensured to work with the specific version of QEMU that it was generated from. The key problem here is the removal of features after deprecation. That removal is fine if you only ever need an API to talk to the current QEMU, or only need to be able to introspect the current QEMU. Management apps are likely to want to write code that works with more than 1 version of QEMU, and be able to decide whether they provide the params needed by the old command or the new command. The introspection data lets them make the decision about which command needs to be invoked, but it can't be used to generate the code needed for the old command. I don't know how exactly you're dealing with the Python mapping. If you're literally statically generating Python code you'll face this same problem. If on the other hand you've just got a stub object that does dynamic dispatch then it can dynamically adapt at runtime to work with whatever version of the schema it queried from the QEMU it is talking to. I'm hoping you're doing the latter for this reason. One strategy for compiled languages is to generate multiple copies of the APIs, one for each QEMU version. This could be made to work but I feel it is pretty horrific as an approach. Libvirt has one set of client APIs that we've extended over time so they can be used to call both old and new variants of commands - we just need to use the introspected schema to decide which to use. To make the latter viable for generating compiled code though, we need to change how we deal with removals, by essentially never removing things at all. Instead we would need some way to express that "field A" or "type T" is not present after some point in time / release. Overall I don't think we're really in a position to use compiled auto generated bindings, except for QMP clients that are released in lockstep with specific QEMU versions, and I don't think lockstep releases are a viable strategy in general. > > It sounds like you need a wrapper type. This StrOrNull scenario > > is QMP's "alternate" type IIUC, but you're trying to avoid > > expressing the existance fo the "alternate" type in the API > > Yes. This is a very clean way to type it, but it is a little more > laborious for the user to have to remember to wrap certain strings in > a special constructor first. Still, this is a good trick that I hadn't > considered. I'll keep it in mind for future experiments. Bear in mind that this situation is pretty rare, so I don't think the user is especially burdened by needing an extra level of indirection for using 'alternate' types $ git grep "'alternate'" qapi qapi/block-core.json:{ 'alternate': 'BlockDirtyBitmapMergeSource', qapi/block-core.json:{ 'alternate': 'Qcow2OverlapChecks', qapi/block-core.json:{ 'alternate': 'BlockdevRef', qapi/block-core.json:{ 'alternate': 'BlockdevRefOrNull', qapi/common.json:{ 'alternate': 'StrOrNull', $ git grep "'StrOrNull'" qapi qapi/block-core.json: 'iothread': 'StrOrNull', qapi/common.json:{ 'alternate': 'StrOrNull', qapi/migration.json: '*tls-creds': 'StrOrNull', qapi/migration.json: '*tls-hostname': 'StrOrNull', qapi/migration.json: '*tls-authz': 'StrOrNull', Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|