| Message ID | 20230109225419.22621-1-philmd@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | qom: Introduce object_class_property_deprecate() | expand |
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? Kevin
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?
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. With regards, Daniel
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.
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. With regards, Daniel
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.
On 11/1/23 12:29, Markus Armbruster wrote: > 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. I understand breaking the build system is not a welcomed idea and that the deprecation grace period can be flexible. I don't understand why storing version information isn't helpful, as I'd like to run some ./qapi-dump-deprecated-stuff script at least once between releases, and being able to display this info directly seems simpler than going to match in deprecated.rst. But overall the discussion was enlightening, so thanks! Phil.