Skip to content

Conversation

@grendello
Copy link
Contributor

When merging we process several API description files, each for a separate
platform. The resulting file currently carries the first platform level in its
<api platform=""> root element attribute. This is a subtle issue that may
cause problems when applying fixups from metadata (for instance) as the
api-since and api-until attributes in fixups may apply to incorrect sets of
API members.

Fix the issue by putting the latest API description platform level as obtained
from the files provided to api-merge (the files are sorted in platform level
ascending order)

When merging we process several API description files, each for a separate
platform. The resulting file currently carries the *first* platform level in its
`<api platform="">` root element attribute. This is a subtle issue that may
cause problems when applying fixups from metadata (for instance) as the
`api-since` and `api-until` attributes in fixups may apply to incorrect sets of
API members.

Fix the issue by putting the latest API description platform level as obtained
from the files provided to api-merge (the files are sorted in platform level
ascending order)
@atsushieno
Copy link
Contributor

I don't quite get it.

What if we have something like preview API and no platform identifier is provided, or something like Wear API which is basically diverged from some existing platform and therefore shouldn't be merged to previous level? With your changes anything in Android Wear API will appear as Kitkat API, which is definitely no-go.

Could you provide more concrete use case / problematic case that let you create this change?

@atsushieno
Copy link
Contributor

build

@grendello
Copy link
Contributor Author

The problem is that the current api-merge generates api.xml with this header:

<?xml version="1.0" encoding="utf-8"?>
<api platform="10">
 <package name="android">

Note the platform="10" attribute - this is value taken from the first api.xml.in and not the last one. The merged file represents the state of the Android API as described by the latest input file (currently API27 in our tree) - as per API deprecation/removal/addition/etc status - and thus platform should have the latest API's platform value. Problem case? Imagine we have a fixup line which has api-until="17" so it should not apply to anything above platform 17. With current merge result it will, however, apply since platform="10". It's a subtle bug, but a bug regardless.

@jonpryor
Copy link
Contributor

@atsushieno wrote:

What if we have something like preview API and no platform identifier is provided

We control that API generation, right? Couldn't we always insert an /api/@platform value which is the ID, e.g. for API-O we could have:

<api platform="O">
  <package name="android"> ...

However, I'm not sure what that would break, if anything.

@atsushieno
Copy link
Contributor

I thought that this fix is about replacing platform="" ("" as empty string) with some latest "numbered" platform, because the top comment literally says so. But it is actually about platform="*" (* as anything), right? Historically the platform identifier could be empty for anything irregular like AndroidWear (which was 4.4.87 but used to mess the exact profile identifier which often resulted in "" while processing API metadata).

So as long as it is about explicitly-specified platform values I'm okay with the change.

@grendello
Copy link
Contributor Author

@jonpryor api-merge currently assumes platform levels are integers, it would break with O

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants