Skip to content

Conversation

@atsushieno
Copy link
Contributor

api-*.xml.in can be generated dynamically (up to [1]) but enumification
helper tools still depended on DroidDoc, meaning that we cannot remove
package download dependencies. This fixes the situation by eliminating
the dependencies. Especially it is easier to just parse api-
.xml.in to
extract int consts from Java API.

... thought so? It turnd out to not be that easy.

Unlike DroidDoc, api-*.xml.in does not contain "api-since" information,
so we need to parse ALL the API XML. And they cannot be precise
because we only have a subset of the API definitions. For example,
"since API Level 1" now becomes "since API Level 10" because we don't
have api-1.xml.in (not even api-4.xml.in). So they will become non-precise.

(A slightly better possibility to "fix" this is to parse
android-sdk-whatever/platform-tools/api/api-versions.xml, but it won't
contain "already vanished" constants so it will be incomplete either.
I didn't bother to do "better" so far. We don't need that for enumification.)

Therefore, there is a lot of changes in map.csv to reflect those
"insignificant" changes to remap "API since" column (e.g. from "1" to "10").

Regeneration from api-LEVEL.xml.in instead of DroidDoc has some other
side effects; some consts are back to the list so that we don't have to
manually copy existing mapped consts that could not be generated from
the latest DroidDocs (due to disappearance) anymore. Instead we have to
remove manually-mapped constants (either from the sources or map.csv).
Therefore, a handful of heading lines in map.csv were removed, and
some definitions in enum-conversion-mappings.xml are back.

[*1] #1038

@atsushieno atsushieno added the use-merge-commit Normally we squash-and-merge PRs. Use this label so we instead use a merge commit. label Nov 21, 2017
atsushieno added a commit to atsushieno/xamarin-android-api-compatibility that referenced this pull request Nov 21, 2017
Changes in enumification-helpers and applying the changes from the new tools dotnet/android#1044 results in  https://jenkins.mono-project.com/job/xamarin-android-pr-builder/2106/ :

	17:49:44 ABI BREAK IN: Mono.Android.dll
	17:49:46 <!-- start namespace Android.Views --> <div>
	17:49:46 <h2>Namespace Android.Views</h2>
	17:49:46 <!-- start type Display --> <div>
	17:49:46 <h3>Type Changed: Android.Views.Display</h3>
	17:49:46 <p>Removed field:</p>
	17:49:46
	17:49:46 <pre>
	17:49:46 	<span class='removed removed-field breaking' data-is-breaking>public static const int StateDozing;</span>
	17:49:46 </pre>
	17:49:46
	17:49:46 </div> <!-- end type Display -->
	17:49:46 <!-- start type View --> <div>
	17:49:46 <h3>Type Changed: Android.Views.View</h3>
	17:49:46 <p>Modified fields:</p>
	17:49:46 <pre>
	17:49:46 <div data-is-breaking>	public const <span class='removed removed-inline removed-breaking-inline'>int</span> <span class='added '>TextAlignment</span> TextAlignmentResolvedDefault = 131072;
	17:49:46 </div></pre>
	17:49:46
	17:49:46 </div> <!-- end type View -->
	17:49:46 <!-- start type WindowManagerLayoutParams --> <div>
	17:49:46 <h3>Type Changed: Android.Views.WindowManagerLayoutParams</h3>
	17:49:46 <p>Removed field:</p>
	17:49:46
	17:49:46 <pre>
	17:49:46 	<span class='removed removed-field breaking' data-is-breaking>public static const int TypeKeyguard;</span>
	17:49:46 </pre>
	17:49:46
	17:49:46 </div> <!-- end type WindowManagerLayoutParams -->
	17:49:46
	17:49:46 </div> <!-- end namespace Android.Views -->
	17:49:47
	make[2]: *** [check] Error 1
	17:49:47 make[1]: *** [run-api-compatibility-tests] Error 2
	17:49:47 make: *** [run-all-tests] Error 2
	17:49:47 + exit 1

But what are they?

- STATE_DOZING in https://developer.android.com/reference/android/view/Display.html ... it doesn't exist!
- TEXT_ALIGNMENT_RESOLVED_DEFAULT in https://developer.android.com/reference/android/view/View.html ... ditto!
- TYPE_KEYGUARD in https://developer.android.com/reference/android/view/WindowManager.LayoutParams.html ... neither!

They are all false alarm. They should vanish.
@atsushieno atsushieno requested a review from jonpryor November 21, 2017 20:54
@jonpryor
Copy link
Contributor

Therefore, there is a lot of changes in map.csv to reflect those
"insignificant" changes to remap "API since" column (e.g. from "1" to "10").

I don't understand this: generator uses integer < for version comparisons, not string comparisons or equality comparisons when parsing CSV files and the API level for an enum mapping.

Why did these lines need to change at all?

@@ -71,19 +49,6 @@
0,,,android/view/Surface.FX_SURFACE_MASK,98304
0,,,android/view/Surface.FX_SURFACE_NORMAL,0

18,Android.Hardware.Location.GeofenceTransition,Entered,android/hardware/location/GeofenceHardware.GEOFENCE_ENTERED,1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this "block" of mappings need to be moved, or otherwise touched? It makes for a noisier diff.

Copy link
Contributor Author

@atsushieno atsushieno Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those lines above around line # 100 were manually added because they could not be automatically generated (because, they didn't exist in DroidDoc anymore!).

Now we generate enum const list based on api-*.xml.in they can be automatically generated again and therefore they went below # 100. It is a good thing!

// - Automatically generated mappings (see tools/enumification-helpers how map.ext.csv is generated.)

1,Org.XmlPull.V1.XmlPullParserNode,Cdsect,I:org/xmlpull/v1/XmlPullParser.CDSECT,5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per a previous comment, I don't understand why the first column here actually needed to change.


static string GetApiLevel (string file)
{
Console.Error.WriteLine ($"[{file}]");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still need this debug spew?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a big deal, we (well most likely only I) use it only when we are going to deal with new API for enumification.

@jonpryor
Copy link
Contributor

For example,
"since API Level 1" now becomes "since API Level 10" because we don't
have api-1.xml.in (not even api-4.xml.in). So they will become non-precise.

I believe that this is a non-issue, as we only generate [Register (..., ApiSince=1)] if the type was never seen before. As a consequence, all types introduced in API-10 have ApiSince=1, as we don't actually know anything before API-10.

We don't currently emit [Register (..., ApiSince=4)] anywhere.

In a post PR #1032 world, we'll replace all instances of [Register (..., ApiSince=11) with [Register (..., ApiSince=1)], and that will be fine.

@jonpryor
Copy link
Contributor

It would be nice if the documentation could be updated to explain the use/workflow of the XML files that generate-const-list-2.exe creates.

@atsushieno
Copy link
Contributor Author

Why did these lines need to change at all?

From the "data" perspective they don't have to change, but for "tools" perspective there is nothing else we CAN do. There is only filename that could give us API level information in api-*.xml.in. And we don't have api-1.xml.in, api-4.xml,in, api-20.xml.in etc.

@atsushieno
Copy link
Contributor Author

In a post PR #1032 world, we'll replace all instances of [Register (..., ApiSince=11) with [Register (..., ApiSince=1)], and that will be fine.

No, that's not going to happen because we still don't have api-1.xml.in.

@atsushieno
Copy link
Contributor Author

It would be nice if the documentation could be updated to explain the use/workflow of the XML files that generate-const-list-2.exe creates.

Good call! It will be updated.

@atsushieno atsushieno force-pushed the enumification-helper-migration branch from 1582d0d to f9b92bd Compare November 30, 2017 17:57
api-*.xml.in can be generated dynamically (up to [*1]) but enumification
helper tools still depended on DroidDoc, meaning that we cannot remove
package download dependencies. This fixes the situation by eliminating
the dependencies. Especially it is easier to just parse api-*.xml.in to
extract int consts from Java API.

... thought so? It turnd out to not be that easy.

Unlike DroidDoc, api-*.xml.in does not contain "api-since" information,
so we need to parse ALL the API XML. And they cannot be **precise**
because we only have a subset of the API definitions. For example,
"since API Level 1" now becomes "since API Level 10" because we don't
have api-1.xml.in (not even api-4.xml.in). So they will become non-precise.

(A slightly better possibility to "fix" this is to parse
`android-sdk-whatever/platform-tools/api/api-versions.xml`, but it won't
contain "already vanished" constants so it will be incomplete either.
I didn't bother to do "better" so far. We don't need that for enumification.)

Therefore, there is a lot of changes in `map.csv` to reflect those
"insignificant" changes to remap "API since" column (e.g. from "1" to "10").

Regeneration from api-LEVEL.xml.in instead of DroidDoc has some other
side effects; some consts are back to the list so that we don't have to
manually copy existing mapped consts that could not be generated from
the latest DroidDocs (due to disappearance) anymore. Instead we have to
remove manually-mapped constants (either from the sources or `map.csv`).
Therefore, a handful of heading lines in `map.csv` were removed, and
some definitions in `enum-conversion-mappings.xml` are back.

[*1] dotnet#1038
@atsushieno atsushieno force-pushed the enumification-helper-migration branch from f9b92bd to 209a285 Compare November 30, 2017 21:35
@jonpryor
Copy link
Contributor

@atsushieno wrote:

In a post PR #1032 world, we'll replace all instances of [Register (..., ApiSince=11) with [Register (..., ApiSince=1)], and that will be fine.

No, that's not going to happen because we still don't have api-1.xml.in.

That doesn't matter. We currently emit ApiSince=1, and we currently do not have an api-1.xml.in file:

$ grep -r -E '\[Register.*ApiSince = 1\)' src/Mono.Android/obj/Debug/android-27/mcw
src/Mono.Android/obj/Debug/android-27/mcw/Android.App.DatePickerDialog.cs:              [Register ("android/app/DatePickerDialog$OnDateSetListener", "", "Android.App.DatePickerDialog/IOnDateSetListenerInvoker", ApiSince = 1)]
src/Mono.Android/obj/Debug/android-27/mcw/Android.App.KeyguardManager.cs:               [Register ("android/app/KeyguardManager$OnKeyguardExitResult", "", "Android.App.KeyguardManager/IOnKeyguardExitResultInvoker", ApiSince = 1)]
...

There are 464 instances of ApiSince = 1 in the code generated for API-27, and those are obviously not coming from an api-1.xml.in.

They are instead coming from $(AndroidSdkDirectory)/platform-tools/api/api-versions.xml, as our generator invocation uses --apiversions, which parses the above file to extract ApiSince values:

        <class name="android/app/DatePickerDialog$OnDateSetListener" since="1">
                <extends name="java/lang/Object" />
                <method name="onDateSet(Landroid/widget/DatePicker;III)V" />
        </class>

Again, I reiterate: I do not believe that src/Mono.Android/map.csv needed to change at all. If it does need to change, I do not understand why.

@atsushieno
Copy link
Contributor Author

You don't really understand what I have been writing.

map.csv is NOT generated BY generator.

It WAS generated by scraping DroidDoc which HAD "since" section.

api-*.xml.in does NOT have that information.

There is NO SOURCE that retrieves that information.

Note that api-versions.xml does not contain any information about old API.

@jonpryor
Copy link
Contributor

jonpryor commented Dec 1, 2017

You don't really understand what I have been writing.

Indeed.

map.csv is NOT generated BY generator.

This implies that map.csv is generated. I was not aware it was generated; I thought it was manually maintained.

The first comment in map.csv doesn't help matters, either:

// this cannot be automatically generated, too exceptional...

If map.csv is generated, then could it please follow some semblance of convention and start with a comment containing the command used to generate the file? Or at least an indication that this is a generated file, and where one should go to properly update it, if only so I don't go manually updating that file in the future?

If map.csv isn't generated, then I'm really confused...

@atsushieno
Copy link
Contributor Author

Everyone can track git history and see how those things are made changes. Did you add "it is automatically generated" comments on EVERYTHING you make it to generate? Which is automatically generated for Configuration.*.props, C# sources and Java sources, timestamp files etc. etc.? No we don't.

If you want to add those lines you can create a PR and ask me for merging permission.

@jonpryor
Copy link
Contributor

jonpryor commented Dec 1, 2017

Everyone can track git history and see how those things are made changes.

No, I can't. Git history isn't going to contain what commands were used to generate the changes, or when those commands were executed, or what the implications are. That's what documentation is for (previously requested).

I've been under the assumption that everything in src/Mono.Android/map.csv is manually maintained, full stop. I have no idea how the other tools fit together, or what parts are hand-written, and what parts aren't, or how they're merged, or...

All I see from git history and PRs is that map.csv changed. That's all.

Did you add "it is automatically generated" comments on EVERYTHING you make it to generate?

Good point. I should update everything to do so.

@atsushieno
Copy link
Contributor Author

The updated documentation is in this revised PR.

@atsushieno
Copy link
Contributor Author

It is going to be unnecessary once PR #1045 gets merged as it contains this change too.

@atsushieno
Copy link
Contributor Author

Superceded by #1045

@atsushieno atsushieno closed this Dec 7, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

use-merge-commit Normally we squash-and-merge PRs. Use this label so we instead use a merge commit.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants