inconsistency in evolution guidelines?

[Mukesh Agrawal]

Hi API Council,

In looking over the Fuchsia API evolution guidelines, some of the examples on the page seem inconsistent with the rest of the page.

Specifically:

1. The diagram at the top of the page shows a four-step lifecycle: Added, Deprecated, Removed (legacy), and Deleted.
2. The text at the end says that, when removing an API, we should maintain legacy support by setting `legacy=true`.

So far, so good.

But in the examples in the "Deprecating FIDL APIs" section, the `removed` attribute is added without `legacy=true`.

So, if I just skim the page for examples to follow, I'll fail to preserve ABI.

To avoid that risk, I'd like to update those examples to either include `legacy=true`, or to have `deprecated=N, removed=N+2`.

Alternatively, I could add a cautionary note in that section that those examples are only speaking to what the compiler allows, not what's good practice.

WDYT?

--

see my communication preferences

[Seth Ladd]

Thanks for the feedback! I'm happy to help make the change in the doc, if someone can confirm.

On Wed, Jun 28, 2023 at 1:45 PM 'Mukesh Agrawal' via api-council <api-c...@fuchsia.dev> wrote:

Hi API Council,

In looking over the Fuchsia API evolution guidelines, some of the examples on the page seem inconsistent with the rest of the page.

Specifically:

1. The diagram at the top of the page shows a four-step lifecycle: Added, Deprecated, Removed (legacy), and Deleted.
2. The text at the end says that, when removing an API, we should maintain legacy support by setting `legacy=true`.

So far, so good.

But in the examples in the "Deprecating FIDL APIs" section, the `removed` attribute is added without `legacy=true`.

So, if I just skim the page for examples to follow, I'll fail to preserve ABI.

To avoid that risk, I'd like to update those examples to either include `legacy=true`, or to have `deprecated=N, removed=N+2`.

cc @Hunter Freyer -- WDYT?

 

Alternatively, I could add a cautionary note in that section that those examples are only speaking to what the compiler allows, not what's good practice.

WDYT?

--

see my communication preferences

--
You received this message because you are subscribed to the Google Groups "api-council" group.
To unsubscribe from this group and stop receiving emails from it, send an email to api-council...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/api-council/CAENAxg0J9C_7Cz0ZDX-63uZoFTzD8RdzJ%2BOEcC5pQaT0pC4CPQ%40mail.gmail.com.

[Hunter Freyer]

</lag>

+fuchsia-platform-versioning-wg 

On Thu, Jun 29, 2023 at 2:50 PM Seth Ladd <seth...@google.com> wrote:

Thanks for the feedback! I'm happy to help make the change in the doc, if someone can confirm.

On Wed, Jun 28, 2023 at 1:45 PM 'Mukesh Agrawal' via api-council <api-c...@fuchsia.dev> wrote:

Hi API Council,

In looking over the Fuchsia API evolution guidelines, some of the examples on the page seem inconsistent with the rest of the page.

Specifically:

1. The diagram at the top of the page shows a four-step lifecycle: Added, Deprecated, Removed (legacy), and Deleted.
2. The text at the end says that, when removing an API, we should maintain legacy support by setting `legacy=true`.

So far, so good.

But in the examples in the "Deprecating FIDL APIs" section, the `removed` attribute is added without `legacy=true`.

So, if I just skim the page for examples to follow, I'll fail to preserve ABI.

To avoid that risk, I'd like to update those examples to either include `legacy=true`, or to have `deprecated=N, removed=N+2`.

cc @Hunter Freyer -- WDYT?

Yes, as far as I can tell, any example of `removed=N` should have `legacy=true`, at least until N is no longer supported. And at that point, we should just delete the whole API element.

+Mitchell Kember Are there any situations where we shouldn't have `legacy=true` for a removed API element? Should we just make `legacy=true` the default and remove the flag altogether?

 

Thanks,

Hunter

[Mukesh Agrawal]

I see. 

I guess I'd like to do two things:

Short term

Change the example of deprecating a method, to include legacy=true, perhaps

with a comment referencing the "Preserving ABI" section.

Longer term

Revise the API evolution guidelines to make it clear that

• "API" includes many different elements (libraries, methods, constants, etc)
• Different elements have different deprecation guidelines/policies
• Methods specifically have the policy that they must have legacy=true when they're removed

How does that sound?
 

On Tue, Jul 25, 2023 at 11:42 AM Mitchell Kember <mke...@google.com> wrote:

We can't remove the legacy flag because removed=N does not always mean "removed" in the API lifecycle sense, it can also mean "replaced". For example, changing a string bound:

@available(added=1)

library test;

protocol Foo {

    @available(removed=2) Bar(struct { baz string:10; });

    @available(added=2)   Bar(struct { baz string:20; });

}

Using legacy=true here would be an error because there would be two coexisting methods named "Bar".

There are some other cases where legacy=true is unnecessary, such as removing a constant or a type alias, or removing a table field if you would just be ignoring the field anyway.

[Wez]

On Tue, 25 Jul 2023 at 21:34, 'Mukesh Agrawal' via api-council <api-c...@fuchsia.dev> wrote:

Short term

Change the example of deprecating a method, to include legacy=true, perhaps

with a comment referencing the "Preserving ABI" section.

IIUC "legacy" is only relevant when _removing_ a method - "deprecated" APIs must continue to be supported until they're removed, to allow clients a window during which to migrate off them.

But yes, updating the documentation to state that "legacy=true" should be added alongside "removed" seems reasonable given the current tooling.

Longer term

Revise the API evolution guidelines to make it clear that

• "API" includes many different elements (libraries, methods, constants, etc)
• Different elements have different deprecation guidelines/policies
• Methods specifically have the policy that they must have legacy=true when they're removed

How does that sound?

The semantics around "legacy" seem a little fuzzy, IMO. The documentation refers to the platform's implementation of the API being preserved, but in general that is not sufficient for fuchsia platform APIs, whose implementations in a system may be provided from outside the platform itself.

Taking fuchsia.web, provided by Chromium, as an example, when the fuchsia.web.Frame.CreateView() is removed (since it has been replaced by CreateView2()), in order for Chromium targeting a post-removal API level to continue to support the ABI, it will need to have FIDL bindings generated with that API still available.

It does look like the "legacy" flag was added only after RFC0083 had been accepted - I'd recommend that we review whether the current definition & implementation actually makes sense (perhaps in conjunction with cleaning up "transitional"!).

To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/api-council/CAENAxg0F81O7XrYWuTwT54BX%2BMZrpLCjz859drh8TQsoWhCXwg%40mail.gmail.com.

[Mitchell Kember]

Sure, that sounds good to me.

[Mitchell Kember]

We can't remove the legacy flag because removed=N does not always mean "removed" in the API lifecycle sense, it can also mean "replaced". For example, changing a string bound:

@available(added=1)

library test;

protocol Foo {

    @available(removed=2) Bar(struct { baz string:10; });

    @available(added=2)   Bar(struct { baz string:20; });

}

Using legacy=true here would be an error because there would be two coexisting methods named "Bar".

There are some other cases where legacy=true is unnecessary, such as removing a constant or a type alias, or removing a table field if you would just be ignoring the field anyway.

On Tue, Jul 25, 2023 at 11:08 AM Hunter Freyer <hjfr...@google.com> wrote:

[Mitchell Kember]

On Wed, Jul 26, 2023 at 5:14 AM Wez <w...@google.com> wrote:

IIUC "legacy" is only relevant when _removing_ a method - "deprecated" APIs must continue to be supported until they're removed, to allow clients a window during which to migrate off them.

But yes, updating the documentation to state that "legacy=true" should be added alongside "removed" seems reasonable given the current tooling.

+1, I misread that. It is an error to use legacy=true when something is only deprecated, not removed.

 

The semantics around "legacy" seem a little fuzzy, IMO. The documentation refers to the platform's implementation of the API being preserved, but in general that is not sufficient for fuchsia platform APIs, whose implementations in a system may be provided from outside the platform itself.

Taking fuchsia.web, provided by Chromium, as an example, when the fuchsia.web.Frame.CreateView() is removed (since it has been replaced by CreateView2()), in order for Chromium targeting a post-removal API level to continue to support the ABI, it will need to have FIDL bindings generated with that API still available.

It does look like the "legacy" flag was added only after RFC0083 had been accepted - I'd recommend that we review whether the current definition & implementation actually makes sense (perhaps in conjunction with cleaning up "transitional"!).

 

I agree "legacy" is a bit weird. I don't think a scenario like CreateView was considered. It seems like there's a fundamental challenge in using the same bindings in an asymmetric way, where "removal" has to happen earlier/later in some places. Happy to discuss this further if you have more ideas.

[Seth Ladd]

On Thu, Jul 27, 2023 at 10:33 AM Mitchell Kember <mke...@google.com> wrote:

On Wed, Jul 26, 2023 at 5:14 AM Wez <w...@google.com> wrote:

IIUC "legacy" is only relevant when _removing_ a method - "deprecated" APIs must continue to be supported until they're removed, to allow clients a window during which to migrate off them.

But yes, updating the documentation to state that "legacy=true" should be added alongside "removed" seems reasonable given the current tooling.

+1, I misread that. It is an error to use legacy=true when something is only deprecated, not removed.

IIUC, the above means we should update https://fuchsia.dev/fuchsia-src/development/api/evolution?hl=en ?

If so, @Mitchell Kember , is that something you'd be willing to do?

[Mitchell Kember]

Sorry, I forgot to get back here.

I decided to write a short RFC to improve the way LEGACY works:

https://fuchsia-review.git.corp.google.com/c/fuchsia/+/906797

If this gets accepted, then I'll update that documentation page. It won't need to say to use "legacy=true" because there won't be a "legacy" argument anymore.

[Mukesh Agrawal]

Many thanks!

I haven't chimed in on the RFC, because I didn't follow all of the technical details. But I love that you're asking "is this just a footgun that we can remove?"