-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[class-parse] support Module AttributeInfo #1097
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
@@ -88,9 +88,6 @@ static bool ShouldLoadEntry (ZipArchiveEntry entry) | |||
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
ClassFile
entries of typeClassAccessFlags.Module
, and remove them fromclassFiles
. This prevents them from being written intoapi.xml
. - For the
ClassFile
entries in (1), we find the set of exported packages (inpublicPackages
), and (2) iterate over every entry inclassFiles
. AnyclassFiles
entry with a package name outside ofpublicPackages
has 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#
internal
access modifier:public
types in a non-export
ed package should be treated as"internal", while
public
types in anexport
ed package are"fully public".
Update
Xamarin.Android.Tools.Bytecode.dll
to extract the module-related information, the update
XmlClassDeclarationBuilder
so thatit updates all
public
types outside of the "exported" packages tohave a visibility of
kotlin-internal
.Why a
//*/@visibility
value 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#
internal
visibility, while we want themto 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:Contain a
module-info.java
, which declares acom.xamarin
module.
Add a new
com.xamarin.internal.PublicClassNotInModuleExports
type which is not in the
com.xamarin
package, but insteada nested package. The type is
public
.Build a
xatb.jar
artifactThis makes for a simple one-off test:
Note that
com.xamarin.internal.PublicClassNotInModuleExports
is nowshown as
kotlin-internal
instead ofpublic
.Aside, a discovered oddity:
jar cf …
modifiesmodule-info.class
,adding a
ModulePackages
attribute! (Specifically, if you comparethe "on-disk"
module-info.class
to the one withintests/Xamarin.Android.Tools.Bytecode-Tests/obj/$(Configuration)/xatb.jar
,they differ in size!)