Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Dec 7, 2021

Changes: dotnet/java-interop@7f1a5ab...bc5bcf4

  * dotnet/java-interop@bc5bcf4f: [Java.Base] Begin binding JDK-11 java.base module (dotnet#909)
  * dotnet/java-interop@af91b9c2: [ci] Use descriptive test run titles. (dotnet#926)
  * dotnet/java-interop@aae8f251: [Java.Interop.Tools.Generator] Add some enumification helper methods (dotnet#866)
  * dotnet/java-interop@111ebca8: [Java.Interop] Treat warnings as errors. (dotnet#925)
  * dotnet/java-interop@7a32bb97: [java-source-utils] Transform XML using XSLT (dotnet#924)
  * dotnet/java-interop@7f55b2dc: [logcat-parse] Use C# verbatim strings for paths (dotnet#927)
  * dotnet/java-interop@2601146b: [java-source-utils] Fix regresion from 77c9c5fa (dotnet#923)
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Dec 7, 2021
Context: dotnet/android#6549

When attempting to bump xamarin-android to use bc5bcf4, the build
failed because bc5bcf4 added a "duplicate" type
`Java.Interop.JavaTypeParametersAttribute`:

	error CS0433: The type 'JavaTypeParametersAttribute' exists in both
	'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and
	'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'.

The plan was to use type forwarders for this type from
`Mono.Android.dll` to `Java.Interop.dll`, but in retrospect this
sounds like a potential "forward-incompatible ABI break", as
assemblies built against the newer `Mono.Android.dll` (with type
forwarders) won't be usable on previous Xamarin.Android SDKs.

Make `JavaTypeParametersAttribute` conditional on `NET`, so that it's
only included in .NET 6 builds.  .NET SDK for Android will then use
type forwarders, which won't be an issue as there's no previous .NET
release to be forward compatible with.
@jonpryor jonpryor force-pushed the jonp-bump-ji-bc5bcf4f branch 2 times, most recently from 42b5532 to fd17f1a Compare December 7, 2021 18:48
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Dec 7, 2021
Context: dotnet/android#6549

When attempting to bump xamarin-android to use bc5bcf4, the build
failed because bc5bcf4 added a "duplicate" type
`Java.Interop.JavaTypeParametersAttribute`:

	error CS0433: The type 'JavaTypeParametersAttribute' exists in both
	'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and
	'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'.

The plan was to use type forwarders for this type from
`Mono.Android.dll` to `Java.Interop.dll`, but in retrospect this
sounds like a potential "forward-incompatible ABI break" (e56a8c8)
as assemblies built against the newer `Mono.Android.dll` (with type
forwarders) won't be usable on previous Xamarin.Android SDKs.

Make `JavaTypeParametersAttribute` conditional on `NET`, so that it's
only included in .NET 6 builds.  .NET SDK for Android will then use
type forwarders, which won't be an issue as there's no previous .NET
release to be forward compatible with.

Update `tests/generator-Tests` so that `JavaTypeParametersAttribute`
is once again provided for non-`JAVA_INTEROP1` builds.
jonpryor added a commit to jonpryor/java.interop that referenced this pull request Dec 7, 2021
Context: dotnet/android#6549

When attempting to bump xamarin-android to use bc5bcf4, the build
failed because bc5bcf4 added a "duplicate" type
`Java.Interop.JavaTypeParametersAttribute`:

	error CS0433: The type 'JavaTypeParametersAttribute' exists in both
	'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and
	'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'.

The plan was to use type forwarders for this type from
`Mono.Android.dll` to `Java.Interop.dll`, but in retrospect this
sounds like a potential "forward-incompatible ABI break" (e56a8c8)
as assemblies built against the newer `Mono.Android.dll` (with type
forwarders) won't be usable on previous Xamarin.Android SDKs.

Make `JavaTypeParametersAttribute` conditional on `NET`, so that it's
only included in .NET 6 builds.  .NET SDK for Android will then use
type forwarders, which won't be an issue as there's no previous .NET
release to be forward compatible with.

Update `tests/generator-Tests` so that `JavaTypeParametersAttribute`
is once again provided for non-`NET` builds.
@jonpryor
Copy link
Contributor Author

jonpryor commented Dec 7, 2021

Commit message:

Changes: https://github.com/xamarin/java.interop/compare/7f1a5ab1606e4055eb4705aeafa7a22117998a33...aac3e9acca503c38c5616ef84d8af28197da254c

  * xamarin/java.interop@aac3e9ac: [Java.Interop, Java.Base] Fix xamarin-android integration issues (#929)
  * xamarin/java.interop@bc5bcf4f: [Java.Base] Begin binding JDK-11 java.base module (#909)
  * xamarin/java.interop@af91b9c2: [ci] Use descriptive test run titles. (#926)
  * xamarin/java.interop@aae8f251: [Java.Interop.Tools.Generator] Add some enumification helper methods (#866)
  * xamarin/java.interop@111ebca8: [Java.Interop] Treat warnings as errors. (#925)
  * xamarin/java.interop@7a32bb97: [java-source-utils] Transform XML using XSLT (#924)
  * xamarin/java.interop@7f55b2dc: [logcat-parse] Use C# verbatim strings for paths (#927)
  * xamarin/java.interop@2601146b: [java-source-utils] Fix regresion from 77c9c5fa (#923)

(So many inadvertent issues as part of this bump…)

xamarin/java.interop@bc5bcf4f added an *altered*
`Java.Interop.JavaTypeParametersAttribute` custom attribute, altered
to "appease" various Code Analysis warnings such as [CA1813][0]
"Avoid unsealed attributes".

This introduction caused some unit tests to fail to build:

	error CS0433: The type 'JavaTypeParametersAttribute' exists in both
	'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and
	'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'
	[C:\…\temp\LibraryProjectZipWithLint\BindingsProject\BindingsProject.csproj]

The original idea had been to use [type forwarders][1], so that
`Java.Interop.JavaTypeParametersAttribute` from `Mono.Android.dll`
would be forwarded to the same type in `Java.Interop.dll`.  This idea
was rejected after further consideration, as it would likely break
[forward compatibility][2] (xamarin/Java.Interop@e56a8c8e).

xamarin/java.interop@aac3e9ac fixes this by making
`JavaTypeParametersAttribute` conditional on `NET`, causing the type
to only be present when targeting .NET 6+.  `Mono.Android.dll` for
.NET 6 -- and *not* Classic Xamarin.Android -- can then use a type
forwarder.   This can be done because there's no forward
compatibility constraint on .NET 6.

However, by making `JavaTypeParametersAttribute` conditional on `NET`,
the `_CheckApiCompatibility` target started failing -- rightfully! --
because the `JavaTypeParametersAttribute` type had been removed.
"Worse", there was no way to accept this change in e.g.
`tests/api-compatibility/acceptable-breakages-vReference.txt`,
because the `<CheckApiCompatibility/>` task requires that there be no
extra entries.

Square this circle by adding a `CheckApiCompatibility.TargetFramework`
property, and looking for
`acceptable-breakages-*-{TargetFramework}.txt` *before* looking for
`acceptable-breakages-*.txt`.  This allows us to special-case the
acceptable breakages based on `$(TargetFramework)`.

Finally, update `azure-pipelines.yaml` so that the `java-source-utils`
unit tests are *not* run on Windows.  This is because they currently
fail on Windows, which is something we'll worry about later.  For now,
disable the `java-source-utils` tests when building on Windows.

[0]: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1813
[1]: https://docs.microsoft.com/en-us/dotnet/standard/assembly/type-forwarding
[2]: https://en.wikipedia.org/wiki/Forward_compatibility

@jonpryor
Copy link
Contributor Author

jonpryor commented Dec 8, 2021

The attempted fix didn't fix, e.g. LibraryProjectZipWithLint:

  /Users/runner/work/1/a/TestRelease/12-07_20.01.12/temp/LibraryProjectZipWithLint/BindingsProject/obj/Debug/generated/src/Androidx.Fragment.App.FragmentHostCallback.cs(10,24,10,42): error CS0433: The type 'JavaTypeParametersAttribute' exists in both 'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and 'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' [/Users/runner/work/1/a/TestRelease/12-07_20.01.12/temp/LibraryProjectZipWithLint/BindingsProject/BindingsProject.csproj]
  /Users/runner/work/1/a/TestRelease/12-07_20.01.12/temp/LibraryProjectZipWithLint/BindingsProject/obj/Debug/generated/src/Androidx.Fragment.App.FragmentManager.cs(803,25,803,43): error CS0433: The type 'JavaTypeParametersAttribute' exists in both 'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and 'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' [/Users/runner/work/1/a/TestRelease/12-07_20.01.12/temp/LibraryProjectZipWithLint/BindingsProject/BindingsProject.csproj]

What's doubly odd is that when I build locally, my Java.Interop.dll doesn't have that type!

% monodis --typedef bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0/Java.Interop.dll | grep JavaType
# no output

However, the Java.Interop.dll in the installer does have that type:

% monodis --typedef Library/Frameworks/Xamarin.Android.framework/Versions/12.1.99.126/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0/Java.Interop.dll | grep JavaTypeP
34: Java.Interop.JavaTypeParametersAttribute (flist=98, mlist=376, flags=0x100101, extends=0x71)

Something fishy is going on…

@jonpryor
Copy link
Contributor Author

jonpryor commented Dec 8, 2021

"Unrelated" (?), Windows fails to build:

  CSC : error CS8032: An instance of analyzer System.Text.Json.SourceGeneration.JsonSourceGenerator cannot be created from C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\6.0.0\analyzers\dotnet\cs\System.Text.Json.SourceGeneration.dll : Could not load file or assembly 'Microsoft.CodeAnalysis, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.. [C:\a\_work\1\s\external\Java.Interop\tools\jcw-gen\jcw-gen.csproj]

No idea what's going on there either.

@jonpryor jonpryor force-pushed the jonp-bump-ji-bc5bcf4f branch from fd17f1a to 44a7beb Compare December 8, 2021 02:18
@jonpryor
Copy link
Contributor Author

jonpryor commented Dec 8, 2021

I'm currently assuming that I didn't test the Java.Interop commits I thought I was testing.

Or, insanity.

In particular, consider: https://github.com/xamarin/java.interop/blob/bc5bcf4f0ef07aab898f2643d2a25f66512d98ed/src/Java.Interop/Java.Interop/JniRuntime.JniMarshalMemberBuilder.cs#L30-L36

This is the JniRuntime.SetMarshalMemberBuilder() method:

		[System.Diagnostics.CodeAnalysis.SuppressMessage ("Design", "CA1031:Do not catch general exception types", Justification = "the *.Export assemblies are optional, so we don't care when they cannot be loaded (they are presumably missing)")]
#if NET
		[DynamicDependency (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor, "Java.Interop.MarshalMemberBuilder", "Java.Interop.Export")]
		[UnconditionalSuppressMessage ("Trimming", "IL2026", Justification = "DynamicDependency should preserve the constructor.")]
		[UnconditionalSuppressMessage ("Trimming", "IL2035", Justification = "Java.Interop.Export.dll is not always present.")]
#endif
		partial void SetMarshalMemberBuilder (CreationOptions options)

Note in particular that it has custom attributes wrapped in NET.

Also note that my fix for JavaTypeParametersAttribute was to likewise wrap it in #if NET: https://github.com/xamarin/java.interop/pull/929/files#diff-fd9c9afe3c2714f79442afdb021394950e51b6791f835fc8f448e2b3e147e852R3

Thus, the only way JavaTypeParametersAttribute should exist is if NET is defined. Yet if I disassemble the Java.Interop.dll that was packaged by the installer, none of the attributes are emitted.

(Which is differently odd, as [SuppressMessage] isn't present either! It's apparently never emitted to IL.)

On the assumption that I Did Something Wrong, I've re-pushed the commit, using the current latest Java.Interop commit. Hopefully things will be saner?

@jonpryor
Copy link
Contributor Author

jonpryor commented Dec 8, 2021

Only the macOS build has finished at the time of this writing, but I think my guess was correct: when I download the updated .pkg file, Java.Interop.dll no longer contains JavaTypeParametersAttribute. This looks better.

jonpryor added a commit to dotnet/android-libraries that referenced this pull request Dec 8, 2021
@jonpryor jonpryor force-pushed the jonp-bump-ji-bc5bcf4f branch from 44a7beb to 0acaa76 Compare December 8, 2021 15:55
Context: dotnet/java-interop#929

See if dotnet/java-interop#929 + type forwarders fixes the build
failure originally observed:

	error CS0433: The type 'JavaTypeParametersAttribute' exists in both
	'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and
	'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'
	[C:\a\_work\1\a\TestRelease\12-07_04.08.15\temp\LibraryProjectZipWithLint\BindingsProject\BindingsProject.csproj]
@jonpryor jonpryor force-pushed the jonp-bump-ji-bc5bcf4f branch from 0acaa76 to 32bca42 Compare December 8, 2021 18:12
@jonpryor jonpryor requested a review from pjcollins as a code owner December 8, 2021 19:21
@jonpryor jonpryor force-pushed the jonp-bump-ji-bc5bcf4f branch from 1537620 to 8e3e709 Compare December 8, 2021 21:57
"Fixes" failures on Windows

	RunTests:
	  "C:\a\_work\2\s\external\Java.Interop\build-tools\gradle\gradlew" --stacktrace --no-daemon test
	  …
	  > Task :test FAILED
	  …

	  BUILD FAILED in 7s
	C:\a\_work\2\s\external\Java.Interop\tools\java-source-utils\java-source-utils.targets(43,5):
	error MSB3073: The command ""C:\a\_work\2\s\external\Java.Interop\build-tools\gradle\gradlew" --stacktrace --no-daemon test" exited with code 1.
	[C:\a\_work\2\s\external\Java.Interop\tools\java-source-utils\java-source-utils.csproj]
@jonpryor jonpryor force-pushed the jonp-bump-ji-bc5bcf4f branch from 8e3e709 to 4f64a4c Compare December 8, 2021 23:20
jonpryor added a commit to dotnet/java-interop that referenced this pull request Dec 9, 2021
Context: dotnet/android#6549
Context: 7f1a5ab...bc5bcf4

When attempting to bump xamarin-android from 7f1a5ab to bc5bcf4,
the build failed for a number of reasons, including because bc5bcf4
added a "duplicate" type `Java.Interop.JavaTypeParametersAttribute`,
which caused unit tests to fail:

	error CS0433: The type 'JavaTypeParametersAttribute' exists in both
	'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' and
	'Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065'.

The plan was to use type forwarders for this type from
`Mono.Android.dll` to `Java.Interop.dll`, but in retrospect this
sounds like a potential "forward-incompatible ABI break" (e56a8c8),
as assemblies built against the newer `Mono.Android.dll` (with type
forwarders) won't be usable on previous Xamarin.Android SDKs.

Make `JavaTypeParametersAttribute` conditional on `NET`, so that it's
only included in .NET 6 builds.  .NET SDK for Android will then use
type forwarders, which won't be an issue as there's no previous .NET
release to be forward compatible with.

Update `tests/generator-Tests` so that `JavaTypeParametersAttribute`
is once again provided for non-`NET` builds.

Additionally, commit 111ebca enabled `$(TreatWarningsAsErrors)`=True,
which had an inadvertent side effect of preventing VS2019 from
building the Java.Interop repo, as warning CS8032 is now an error:

	error CS8032: An instance of analyzer System.Text.Json.SourceGeneration.JsonSourceGenerator cannot be created from
	C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\6.0.0\analyzers\dotnet\cs\System.Text.Json.SourceGeneration.dll :
	Could not load file or assembly 'Microsoft.CodeAnalysis, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'
	or one of its dependencies.
	The system cannot find the file specified.

As it happens, the xamarin-android Windows PR builds are on VS2019,
and thus promptly broke.

Fix this error by adding CS8032 to `$(NoWarn)`, and updating various
`.csproj` files so that the `$(NoWarn)` in `Directory.Build.props`
is used everywhere needed.

Finally, update `Java.Base.targets` so that it looks for
`class-parse.dll` and `generator.dll` from `$(UtilityOutputFullPath)`
*not* `$(ToolOutputFullPath)`.  `class-parse.csproj` and
`generator.csproj` both set `$(OutputPath)` to
`$(UtilityOutputFullPath)`, and the correct path must be used in
order for `Java.Base.targets` to find the required utilities.
@jonpryor jonpryor merged commit 5319110 into dotnet:main Dec 9, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant