-
Notifications
You must be signed in to change notification settings - Fork 64
[class-parse] support Module AttributeInfo #1097
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ecc04de to
bf9200c
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
Fixes: dotnet#1096 Context: https://stackoverflow.com/questions/57358750/module-info-class-file-is-different-in-the-module-jar-file-and-compiled-module-i Context: 678c4bd JDK 9 adds support for [modules][0], which are (kinda sorta) like .NET Assemblies: modules can depend upon other modules, export packages, etc. In particular: > **exports and exports…to.** An exports module directive specifies > one of the module’s packages whose `public` types (and their nested > `public` and `protected` types) should be accessible to code in all > other modules. This allows an equivalent to the [C# `internal` access modifier][1]: `public` types in a *non-`export`ed package* should be treated as "internal", while `public` types in an `export`ed package are "fully public". Update `Xamarin.Android.Tools.Bytecode.dll` to extract the module- related information, the update `XmlClassDeclarationBuilder` so that it updates all `public` types *outside* of the "exported" packages to have a visibility of `kotlin-internal`. Why a `//*/@visibility` value of `kotlin-internal`? From a [suggestion][2] for the commit message of 678c4bd, which was sadly overlooked in the final merge: > Note: we introduce and use a new `//*/@visibility` value of > `kotlin-internal` because `internal` is an *existing* value that may > be used in `Metadata.xml` files, e.g. making `public` API `internal` > so that it can still be used in the binding, but isn't *public*. If we use `internal`, *those types are still bound*, it's just that the bound types have C# `internal` visibility, while we *want* them to be *skipped entirely*. A visibility value of `kotlin-internal` allows us to skip them, which is desired. `tests/Xamarin.Android.Tools.Bytecode-Tests` has been updated to: 1. Contain a `module-info.java`, which declares a `com.xamarin` module. 2. Add a new `com.xamarin.internal.PublicClassNotInModuleExports` type which is *not* in the `com.xamarin` package, but instead a *nested* package. The type is `public`. 3. Build a `xatb.jar` artifact This makes for a simple one-off test: % dotnet build tests/Xamarin.Android.Tools.Bytecode-Tests/*.csproj % dotnet build tools/class-parse/*.csproj % dotnet bin/Debug-net7.0/class-parse.dll \ tests/Xamarin.Android.Tools.Bytecode-Tests/obj/Debug-net7.0/xatb.jar … <class name="PublicClassNotInModuleExports" … visibility="kotlin-internal" /> Note that `com.xamarin.internal.PublicClassNotInModuleExports` is now shown as `kotlin-internal` instead of `public`. Aside, a discovered oddity: `jar cf …` *modifies* `module-info.class`, adding a `ModulePackages` attribute! (Specifically, if you compare the "on-disk" `module-info.class` to the one within `tests/Xamarin.Android.Tools.Bytecode-Tests/obj/$(Configuration)/xatb.jar`, they differ in size!) [0]: https://www.oracle.com/corporate/features/understanding-java-9-modules.html [1]: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/internal [2]: dotnet#793 (comment)
bf9200c to
b18bfdf
Compare
| if (entry.Length == 0) | ||
| return false; | ||
|
|
||
| if (entry.Name == "module-info.class") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to undo #1093?
That is, will this write module-info.class into api.xml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will "undo" #1093, and No, it will not write module-info.class into api.xml. (I should explicitly call this out in the commit message; doh!)
See FixupModuleVisibility(), in particular https://github.com/xamarin/java.interop/pull/1097/files#diff-cd606380b5886cb27bfcf4f255f042d4cb739a6a33545920a305cba1113dac74R386
We do two things in FixupModuleVisibility():
- We collect all of the
ClassFileentries of typeClassAccessFlags.Module, and remove them fromclassFiles. This prevents them from being written intoapi.xml. - For the
ClassFileentries in (1), we find the set of exported packages (inpublicPackages), and (2) iterate over every entry inclassFiles. AnyclassFilesentry with a package name outside ofpublicPackageshas its visibility updated toClassAccessFlags.Internal.
One problem with this approach, not considered until just now, is that if class-parse is parsing multiple .jar files and writing out a single api.xml, then the module-info.class information would be applied to entries from a different .jar. This may result in "weird" behavior, and I'll need to think about this.
|
It occurs to us that properly supporting Modules may potentially remove API's that AndroidX/GPS (and external bindings) have already shipped as "public". |
Context: dotnet#1097 (comment) Context: dotnet#1097 (comment) Context: b274a67 During code review, two questions came to light: 1. Will this cause API breakage in AndroidX/etc., because `module-info.class` will make types "disappear"? (See also b274a67). 2. `class-parse` can accept *multiple* `.jar` files and `.class` files on the command-line. Should `module-info.class` processing apply to *everything*? Address the first concern by keeping `//*/@visibility` as `public`, and instead if we think the type should be "internal" we instead add an `//*/@annotated-visibility` attribute value of `module-info`. If there is *already* an `//*/@annotated-visibility` value, then we *append* ` module-info` to the attribute value. Address the second concern by re-working `class-parse` command-line parsing. There is now a "global `ClassPath`", which will be used to hold `.class` files provided on the command-line. `.jar` and `.jmod` files provided on the command-line will be given their own `ClassPath` instances, and `module-info.class`-based visibility fixups are specific to each `ClassPath` instance. Global files are processed together. There is thus no way for `module-info.class` visibility changes from `a.jar` to impact `b.jar`. After visibilities are fixed up, we then merge everything into the "global" `ClassPath` instance before writing transforming to XML. Additionally, `class-parse --dump` can now accept `.jar` files, and will dump out *all* `.class` filers within the `.jar` file. To make this output easier, each "entry" starts with a "header" of `-- Begin {ClassFile.FullJniName}`, and a blank link will be printed between each entry.
|
@jpobst: latest commit attempts to resolve the two open issues:
We now keep To address my concern about
This is addressed by altering |
Context: dotnet#1097 (comment) Context: dotnet#1097 (comment) Context: b274a67 During code review, two questions came to light: 1. Will this cause API breakage in AndroidX/etc., because `module-info.class` will make types "disappear"? (See also b274a67). 2. `class-parse` can accept *multiple* `.jar` files and `.class` files on the command-line. Should `module-info.class` processing apply to *everything*? Address the first concern by keeping `//*/@visibility` as `public`, and instead if we think the type should be "internal" we instead add an `//*/@annotated-visibility` attribute value of `module-info`. If there is *already* an `//*/@annotated-visibility` value, then we *append* ` module-info` to the attribute value. Address the second concern by re-working `class-parse` command-line parsing. There is now a "global `ClassPath`", which will be used to hold `.class` files provided on the command-line. `.jar` and `.jmod` files provided on the command-line will be given their own `ClassPath` instances, and `module-info.class`-based visibility fixups are specific to each `ClassPath` instance. Global files are processed together. There is thus no way for `module-info.class` visibility changes from `a.jar` to impact `b.jar`. After visibilities are fixed up, we then merge everything into the "global" `ClassPath` instance before writing transforming to XML. Additionally, `class-parse --dump` can now accept `.jar` files, and will dump out *all* `.class` filers within the `.jar` file. To make this output easier, each "entry" starts with a "header" of `-- Begin {ClassFile.FullJniName}`, and a blank link will be printed between each entry.
cd829ae to
ba8877e
Compare
Fixes: dotnet/java-interop#1096 Changes: dotnet/java-interop@f0e3300...07d5595 * dotnet/java-interop@07d5595b: [class-parse] support Module AttributeInfo (dotnet/java-interop#1097) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Fixes: #1096
Context: https://stackoverflow.com/questions/57358750/module-info-class-file-is-different-in-the-module-jar-file-and-compiled-module-i
Context: 678c4bd
JDK 9 adds support for modules, which are (kinda sorta) like
.NET Assemblies: modules can depend upon other modules, export
packages, etc.
In particular:
This allows an equivalent to the C#
internalaccess modifier:publictypes in a non-exported package should be treated as"internal", while
publictypes in anexported package are"fully public".
Update
Xamarin.Android.Tools.Bytecode.dllto extract the module-related information, the update
XmlClassDeclarationBuilderso thatit updates all
publictypes outside of the "exported" packages tohave a visibility of
kotlin-internal.Why a
//*/@visibilityvalue ofkotlin-internal? From asuggestion for the commit message of 678c4bd, which was sadly
overlooked in the final merge:
If we use
internal, those types are still bound, it's just thatthe bound types have C#
internalvisibility, while we want themto be skipped entirely. A visibility value of
kotlin-internalallows us to skip them, which is desired.
tests/Xamarin.Android.Tools.Bytecode-Testshas been updated to:Contain a
module-info.java, which declares acom.xamarinmodule.
Add a new
com.xamarin.internal.PublicClassNotInModuleExportstype which is not in the
com.xamarinpackage, but insteada nested package. The type is
public.Build a
xatb.jarartifactThis makes for a simple one-off test:
Note that
com.xamarin.internal.PublicClassNotInModuleExportsis nowshown as
kotlin-internalinstead ofpublic.Aside, a discovered oddity:
jar cf …modifiesmodule-info.class,adding a
ModulePackagesattribute! (Specifically, if you comparethe "on-disk"
module-info.classto the one withintests/Xamarin.Android.Tools.Bytecode-Tests/obj/$(Configuration)/xatb.jar,they differ in size!)