[Mono.Android] Enumify, stabilize API-37.1 - #12418
Conversation
7cd9380 to
2ba02cb
Compare
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
Found 1 error and 2 suggestions. The API 37.1 stabilization and dotted-version MSBuild handling are coherent, but the embedded photo-picker constants and their accessor methods currently generate two different managed enum types; that public API inconsistency needs correction before 37.1 is marked stable. I also noted two public enum naming inconsistencies inline.
CI was not green when reviewed: Package Tests macOS > Tests > APKs 2 had failed, and six lanes were still in progress. The available check metadata did not establish that the package-test failure was caused by this diff.
Generated by Android PR Reviewer for #12418 · gpt56 · 167.5 AIC · ⌖ 17.2 AIC · ⊞ 25.6K
Comment /review to run again
c952227 to
f384c70
Compare
|
/review |
1 similar comment
|
/review |
|
❌ Android PR Reviewer was cancelled. Please review the logs for details.
|
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
❌ Reject
Findings: 1 error, 0 warnings, 0 suggestions.
The API 37.1 enum mappings and version-aware MSBuild conditions look consistent, and all 44 CI checks passed. However, manifest-definition.xml resets historical manifest API levels to 35 and must be regenerated from the complete SDK platform set before merge.
Generated by Android PR Reviewer for #12418 · gpt56 · 174.3 AIC · ⌖ 8.85 AIC · ⊞ 25.6K
Comment /review to run again
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
Reviewed all API 37.1 stabilization changes, including the full changed-file context, enum/method mappings, manifest metadata history, dotted API-level MSBuild handling, and the PR’s stated BindingStudio goal. The follow-up commit restores the historical manifest introduction levels, and I found no correctness or compatibility defects in the current revision.
One non-blocking C# formatting suggestion is attached inline. The completed 44-check Azure pipeline is green; one successful macOS emulator lane is currently being rerun, with no failed checks.
Generated by Android PR Reviewer for #12418 · gpt56 · 212.5 AIC · ⌖ 8.9 AIC · ⊞ 25.6K
Comment /review to run again
jonathanpeppers
added
the
ready-to-review
|
@dalexsoto review |
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
All 16 new src/Mono.Android/methodmap.csv rows use slash-separated package names (and two use $ for nested types), but ApiTransform inserts those values verbatim into XPath queries against dotted API XML. None of the mappings match, exact-head CI reports BG8A04 for them, and the affected APIs remain int instead of using the new enums. Please normalize package/nested-type names or use dotted syntax, regenerate API 37.1, and verify the warnings disappear and public signatures use the intended enums.
The *existence* of an Android QPR1 API-37.1 release alongside an Android QPR2 API-37.2 beta release throws us into a new scenario: the need for (at least?) *three* workload packs: * API-37 [stable] * API-37.1 [currently unstable, but see dotnet#12418] * API-37.2 [unstable] (It would not surprise @jonpryor if there's a 4th by the time we're done with API-37.*…) However, thus far we've only ever expected *two* packages, so the need for a third is unique. Add a new `@(AndroidBuildApiLevel)` item group to `Configuration.props`, which lists all the Android API levels to build, along with the information required to build them (associated framework version, platform id, is it *unstable*). Update `ConfigureWorkload.targets` so that instead of "hardcoding" `@(_FrameworkListOutputs)` against a specific list of of API levels (`$(AndroidDefaultTargetDotnetApiLevel)` {?!}, `$(AndroidLatestStableApiLevel)`, `$(AndroidLatestUnstableApiLevel)`), it uses `@(AndroidBuildApiLevel)`. However, MSBuild item expansion *does not like* using multiple item groups, e.g.: <_RuntimeListOutputs Include="…%(AndroidAbiAndRuntimeFlavor.AndroidRuntime).%(AndroidBuildApiLevel.Identity)…" /> Work around this problem by using a "cross join" across item groups by using an intermediate Target + `@(_AndroidBuildInfo)` item group, allowing use of a *single* item group: <_RuntimeListOutputs Include="…%(_AndroidBuildInfo.AndroidRuntime).%(_AndroidBuildInfo.Identity)…" /> Update the `BuildExtraApiLevels` target to build all API levels within `@(AndroidBuildApiLevel)`, overriding MSBuild properties based on the `@(AndroidBuildApiLevel)` item metadata. Update `tests` so that the `_AddMonoAndroidReference` target finds and uses *stable* bindings. If it references an *unstable* binding, we get a bunch of CS0246 errors as *nothing can be found*, as `-p:EnablePreviewFeatures=true` is required when referencing an unstable binding…
Head branch was pushed to by a user without write access
e27c176 to
41fb204
Compare
Context: http://github.com/jpobst/BindingStudio Context: dotnet#10005 Use jpobst/BindingStudio to enumify API-37.1. Note: current dotnet/java-interop emits an "extra" `,` on `map.csv` output, which would make for a "noisy" diff (every line changed!). The diff size is reduced by removing trailing commas: tr -d '\r' < src/Mono.Android/map.csv > src/Mono.Android/new-map.csv sed 's/,$//' < src/Mono.Android/new-map.csv > src/Mono.Android/map.csv This keeps the diff to a reasonable size. Sanity check: during enumification, this should have no matches: git grep '\.[A-Z][a-z]\.' src/Mono.Android/map.csv Two-letter namespace parts should be all upper-case. TODO (lol): update `map.csv` to current BindingStudio output after this is merged.
41fb204 to
4723893
Compare
I was initially confused by this, because every entry in As changing the API-36.1 and API-37 entries would likely break API, I have corrected the API-37.1 entries. Additionally, I have updated |
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
The force-updated head reintroduces the historical manifest corruption in build-tools/manifest-attribute-codegen: 66 existing element and 429 attribute introduction levels are reset to API 35, historical formats/parent data change, six attributes disappear, and four required non-ignored metadata entries are removed. The all-platform generation workflow consequently fails its metadata-accounting check. Please restore every pre-37 definition and metadata entry from the base/restored head while retaining only the verified 37.0/37.1 additions.
Restore the pre-37 manifest definitions and required metadata entries that were lost when API 37.1 was regenerated from an incomplete platform set. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors, 0 warnings, 1 suggestion.
The current revision resolves the earlier enum-mapping and manifest-history defects, and the API 37.1 mappings, dotted-version MSBuild handling, and restored manifest metadata are coherent. I left one non-blocking inline suggestion to automate validation of newly imported methodmap.csv naming syntax.
The current Azure pipeline rerun has no failures, but its Windows, Linux, and macOS build lanes are still in progress and the aggregate check is queued, so CI completion remains the merge gate.
Generated by Android PR Reviewer for #12418 · gpt56 · 165.4 AIC · ⌖ 8.94 AIC · ⊞ 25.7K
Comment /review to run again
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
HowToAddNewApiLevel.md first normalizes BindingStudio method-map package/nested-type names, but the later “Finishing the method map” section exports and copies raw output into methodmap.csv again. Following the document sequentially can append both normalized and raw rows, while following only the final section recreates slash/$ mappings that produce BG8A04 and leave APIs typed as int. Please consolidate the instructions or make the final step append only the normalized new-methodmap2.csv output once.
Clarify that the normalized BindingStudio output is the final method map so contributors do not append the raw JNI-formatted rows a second time. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
The consolidated method-map normalization still leaves BindingStudio interface rows invalid. BindingStudio emits the class column as I:Type, while ApiTransform recognizes only [Interface]; the documented command normalizes / and $ but leaves I: unchanged. Replaying it on an actual interface row matches zero API XML nodes and would produce BG8A04, leaving the managed API typed as int. Please convert I: to [Interface] in the single normalization/export step.
|
Regarding #12418 (review): this does not identify a defect in the API 37.1 changes or require changing any previously shipped API level. None of the new API 37.1 method-map rows target interfaces. The concern applies only to the future BindingStudio workflow documented in this PR: an interface row may be exported as |
|
@dalexsoto stamp |
dalexsoto
left a comment
dalexsoto
left a comment
There was a problem hiding this comment.
Stamping as requested by @jonathanpeppers; no code review performed.
3 participants
Summary
Stabilizes and enumifies Android API 37.1 using BindingStudio, following the API enumification work in #10005.
map.csvformattingCurrent
dotnet/java-interopoutput includes a trailing comma on everymap.csvrow, which would make the generated diff unnecessarily noisy. The trailing commas were removed before applying the API 37.1 changes:As a namespace-casing sanity check, enumification should produce no matches for:
git grep '\.[A-Z][a-z]\.' src/Mono.Android/map.csvTwo-letter namespace components must remain uppercase.
Validation
The manifest attribute generator succeeds with the complete Android platform set installed, confirming that the new API 37.0/37.1 entries coexist with the restored historical manifest data.
A future update can replace
map.csvwith unmodified BindingStudio output after the trailing-comma behavior is corrected upstream.