Re: [RFC PATCH 0/4] qom: Introduce object_class_property_deprecate()

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Jan 11, 2023 at 11:08:05AM +0100, Philippe Mathieu-Daudé wrote:
>> On 11/1/23 10:59, Daniel P. Berrangé wrote:
>> > On Wed, Jan 11, 2023 at 10:55:47AM +0100, Philippe Mathieu-Daudé wrote:
>> > > On 10/1/23 14:02, Kevin Wolf wrote:
>> > > > Am 09.01.2023 um 23:54 hat Philippe Mathieu-Daudé geschrieben:
>> > > > > Hi,
>> > > > > 
>> > > > > There will always be a need to deprecate things. Here I'm
>> > > > > tackling the QOM (class) properties, since they can be set
>> > > > > from some CLI options (-object -device -global ...).
>> > > > > 
>> > > > > As an experiment, we add object_class_property_deprecate()
>> > > > > to register a class property as deprecated (since some version),
>> > > > > then we deprecate the TYPE_PFLASH_CFI02 'width' property, and
>> > > > > finally as a bonus we emit a warning when the deprecation period
>> > > > > is over, as a reminder. (For that we introduce few 'versions'
>> > > > > helpers).
>> > > > 
>> > > > The last part means that increasing the version number (i.e. the commit
>> > > > that opens the development tree for the next release) can change the
>> > > > output, and this is turn can break test cases.
>> > > > 
>> > > > If we are happy to introduce breakage with a version number change that
>> > > > will require future commits to open the development tree less trivial
>> > > > than they are today because they need to fix the breakage, too, why not
>> > > > make it a build error instead of a different warning message at 
>> > > > runtime?
>> > > 
>> > > To avoid build breakages, maybe it is clever is to store the deprecation
>> > > version in ObjectPropertyInfo and let QAPI inspection scripts enumerate
>> > > / report deprecated features?
>> > 
>> > I don't think we want the version information in the code nor
>> > introspectable at all.
>> > 
>> > We want applications to only apply logic based off features that are
>> > actually available, not predicted future versions where something may
>> > or may not be removed. This is why we exposed only a plain 'deprecated'
>> > boolean field in QAPI schema for other deprecations.  This is just a
>> > warning to be ready for something to change in future. If an application
>> > has not been updated they are fine to carry on using the deprecated
>> > feature. If an application has been updated, they should probe for
>> > existance of the new feature and use that if available, in preference
>> > to the deprecated feature. There's no reason for an application to
>> > consider version numbers.
>> 
>> Right, but "applications" can also be developer scripts right? Not
>> only user / sysadmin.
>> 
>> In particular, some HMP commands are only useful for developers, and
>> they are implemented over QMP -> QAPI. So we already expose extra
>> developer information via QAPI.
>
> Sure, but I still don't think we should expose any version info there.
> A deprecated feature isn't gone until it is gone. In the deprecations
> doc we only mention the release where it is first deprecated, don't
> explicitly state when it will be removed. The 2 cycle timeframe is
> a minimum, not an exact removal date, so it would be misleading to
> claim we'll remove things in exactly 2 cycles.

I agree with Daniel.

I understand the motivation for making developers aware of expired grace
periods.

A warning is one way to make aware.  It creates another problem, though:
since the grace period is flexible, we need a way to extend the period,
and we need to decide right at the beginning of the development cycle.

I think the existing process for getting rid of deprecated stuff in a
timely manner is good enough: document all deprecations in
docs/about/deprecated.rst, and check the file periodically.

I'd recommend to follow QAPI's lead and add a "deprecated" flag to QOM.

We may want to follow QAPI some more and add an "unstable" flag, too.
See commit a3c45b3e62 'qapi: New special feature flag "unstable"' for
rationale.