Remove API adjuster #789
Labels
generator
Issues binding a Java library (generator, class-parse, etc.)
Comments
|
Note: in the case of dotnet/android#5580, the above If I manually replace all instances of Instead, I can add <class
abstract="false"
managedName="HttpRequest._Serializer"
…Something I hadn't considered. Regardless, |
This was referenced Feb 3, 2021
jpobst
added
enhancement
|
I just happened to notice the |
|
@jpobst when is this planned... we are stuck with are release... |
jonpryor
pushed a commit
that referenced
this issue
Apr 22, 2021
Context: #789 One issue with the proposal to flip the order of `ApiXmlAdjuster` and `metadata` in Issue #789 is that the "apply metadata" step is intertwined with the "parse API xml document into POCO's" step. This means that the adjuster step cannot be inserted between them, as the code is currently written. Separate out the two steps so that they can be reordered in the future. Additionally, refactor the metadata fixup step so that it can be unit tested more easily by moving it to `Java.Interop.Tools.Generator.dll`. Unit tests for applying metadata have been added.
jonpryor
pushed a commit
that referenced
this issue
Oct 26, 2021
Fixes: #739 Fixes: #852 Fixes: #853 Context: #789 Context: #814 Context: #815 Today, we use a process called `ApiXmlAdjuster` to convert our "new" `class-parse` format to the old `jar2xml` format. This process attempts to resolve every Java type referenced by the library we intend to bind. However, performing this as a separate step has some disadvantages: - It duplicates work by reading in `api.xml.class-parse` and Cecil'ing all reference assemblies, then writing it back out as `api.xml`. `generator` then reads in `api.xml` and Cecils all the reference assemblies again. - This work is performed as a separate step before `generator` and thus cannot be influenced by `metadata`. - This work is cached, which is "good", but it creates issues for users because they only see warnings from it for full Rebuilds. "Replace" `ApiXmlAdjuster` with `Java.Interop.Tools.JavaTypeSystem`, which is a much more robust Java model to perform this work, and contains enough information to be reused by `generator`. This prevents us from needing to have a second Java and Cecil import step, and we can run `metadata` directly on `api.xml.class-parse` before the import. NOTE: This commit sets the groundwork for the above benefits, but does not achieve them yet. Initially we are only focused on achieving parity with `ApiXmlAdjuster` to make correctness comparisons possible. The new `JavaTypeSystem` infrastructure is used by default; the previous `ApiXmlAdjuster` system can be used instead by using: generator --use-legacy-java-resolver=true … We will provide an MSBuild property to control this when we integrate with xamarin-android in the future. ~~ Warning Messages ~~ In order to better surface potentially real errors to users, we have tweaked the warnings displayed. Errors we encounter during the initial phase of Java type resolution are emitted as warnings. These are missing external Java types and generally indicate the user is missing a reference `.jar` or binding NuGet: warning BG8605: The Java type 'androidx.appcompat.widget.AppCompatTextView' could not be found (are you missing a Java reference jar/aar or a Java binding library NuGet?) The remaining resolution errors are generally the fallout from those missing external types. For example, if the above missing type caused us to remove `com.example.MyTextView`, then that may result in us removing the method `GetMyTextView()` due to a missing return type. Today these messages are only displayed in a Diagnostic log and are often missed. Instead, we are going to write them to a log file called `java-resolution-report.log`, and then display a warning notifying the user that some types could not be bound, which can be double clicked to view the log file. warning BG8606: Some types or members could not be bound because referenced Java types could not be found. See the 'java-resolution-report.log' file for details. Example `java-resolution-report.log`: ==== Cycle 1 ==== '[Class] com.example.MyTextView' was removed because the Java type 'androidx.appcompat.widget.AppCompatTextView' could not be found. ==== Cycle 2 ==== '[Method] com.example.MyActivity.getMyTextView' was removed because the Java type 'com.example.MyTextView' could not be found. "Cycles" represent the passes through the type collection when resolving the collection. For example: - Cycle 1 removed type `com.example.MyType` because its external base type `android.util.List` was missing - Cycle 2 removed type `com.example.MyDerivedType` because its base type `com.example.MyType` is now missing - Cycle 3 removed method `com.example.Foo.getBar()` because it returns `com.example.MyDerivedType`, which is now missing This distinction can be interesting, because Cycle 1 removals are generally due to missing `.jar` dependencies, whereas the remaining cycles are just the internal fallout from previous cycles, which will largely be fixed by fixing the first cycle issues. ~~ Additional Fixes ~~ There are several issues with the `ApiXmlAdjuster` process that *can* be fixed by this change. For the initial purpose of being able to verify that we produce the same output, these have been disabled in the new `JavaTypeSystem` code. * [ApiXmlAdjuster removes required methods from interfaces][0] - Currently disabled via flag `TypeResolutionOptions.RemoveInterfacesWithUnresolvableMembers`. * [ApiXmlAdjuster fails to resolve parent type parameters for nested types][1] - Currently commented out in `JavaTypeModel.GetApplicableTypeParameters()`. * [ApiXmlAdjuster doesn't remove nested types][2] - Currently enabled, compares have been done with a custom `ApiXmlAdjuster` where this has been fixed. * [ApiXmlAdjuster prefers JavaList over ArrayList][3] - Currently enabled, compares have been done with a custom `ApiXmlAdjuster` where this has been fixed. ~~ Performance ~~ Although performance isn't really a focus, there are wins here as well, largely due to only resolving reference types that are actually referenced. Example: `ApiXmlAdjuster` fully resolves all 5K+ types in `Mono.Android.dll`, while `JavaTypeSystem` only resolves the ones needed by the library being bound. Time to resolve `androidx.appcompat.appcompat`: * ApiXmlAdjuster: 2849ms * JavaTypeSystem: 1514ms ~~ Testing ~~ - Unit tests have been ported from `ApiXmlAdjuster-Tests` - Tested for identical output for 136 current AndroidX packages - Tested for identical output for 140 current GPS/FB packages Note `Mono.Android.dll` does not use `ApiXmlAdjuster`, so no testing was performed with it. [0]: #814 [1]: #815 [2]: #852 [3]: #853
|
I'm not sure this really explains what the issue is, or what the desired fix is. Resolving all Java types and removing types and members that depend on types we cannot resolve is a needed step in the binding process, lest we create broken bindings. I think we would need to explore:
For example, in this reported case: Why is |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Context: dotnet/android#5580
The current binding workflow with
class-parseresults in threeapi.xmlfiles:class-parseoutput, inapi.xml.class-parseapi.xml(viagenerator --only-xml-adjuster --xml-adjuster-output=api.xml)Metadata.xmltoapi.xml, inapi.xml.fixed.The problem is (2): it removes types present in (1).
Take for example dotnet/android#5580, in which the
HttpRequest..serializertype is not bound.The problem is that it's removed by (2):
and thus is not present in
api.xml. Consequently, you can't write Metadata.xml transforms which attempt to reference the type, as the type doesn't exist when Metadata.xml is being processed.The only workaround here is to use
<add-node/>with Metadata.xml, copying the XML fragment fromapi.xml.class-parse, a'laThe text was updated successfully, but these errors were encountered: