Daniel P. Berrangé <berra...@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.
