search

dotnet / android

.NET for Android provides open-source bindings of the Android SDK for use with .NET managed languages such as C#
MIT License
1.93k stars 530 forks source link

Support multiple-binding projects in Java Dependency Verification #9013

Closed jpobst closed 2 months ago

jpobst commented 5 months ago

Context: https://github.com/jpobst/Prototype.Android.MavenBindings/issues/13 Context: https://github.com/xamarin/xamarin-android/blob/main/Documentation/docs-mobile/features/maven/java-dependency-verification.md

Today, our Java Dependency Verification feature is built around our recommended practice of 1 binding library per project. This is reflected in the fact that JavaArtifact and JavaVersion only support a single library:

<PackageReference 
  Include="Xamarin.Kotlin.StdLib" 
  Version="1.7.10" 
  JavaArtifact="org.jetbrains.kotlin:kotlin-stdlib" 
  JavaVersion="1.7.10" />

However a user may choose to place multiple Java libraries in a single package (or project), and we have no way to express that.

We should expand our support to allow expressing multiple Java libraries.

Note <PackageReference> is used as an example, but this extends to everywhere, like <ProjectReference> and NuGet package tags.

Solution 1

One possible way to do this is to allow JavaArtifact to contain the JavaVersion, and then allow for multiple libraries to be specified, separated by a semicolon:

<PackageReference 
  Include="Xamarin.Kotlin.StdLib" 
  Version="1.7.10" 
  JavaArtifact="org.jetbrains.kotlin:kotlin-stdlib:1.7.10;org.jetbrains.kotlin:kotlin-stdlib-ktx:1.7.10" />

Solution 2

Another possible way is to add a new JavaArtifacts (plural) metadata with the same semantics as the JavaArtifact described in Solution 1. This new attribute would be mutually exclusive with JavaArtifact/JavaVersion.

<PackageReference 
  Include="Xamarin.Kotlin.StdLib" 
  Version="1.7.10" 
  JavaArtifacts="org.jetbrains.kotlin:kotlin-stdlib:1.7.10;org.jetbrains.kotlin:kotlin-stdlib-ktx:1.7.10" />

It feels like Solution 1 is better.

Since we have only released this as a preview, would it be better to go further and scrap JavaVersion altogether, and always use JavaArtifact with a version? The original idea was that having them separate made it easier to maintain version numbers centrally using Update if desired, like:

<PackageReference 
  Update="Xamarin.Kotlin.StdLib" 
  JavaVersion="1.7.10" />

But I don't know if this is actually useful in practice? 🤷

jonpryor commented 5 months ago

I have a "silly allergy" to lines which are "too wide", and I can see JavaArtifact="A:1;B:2;C:3;…" as resulting in very wide lines. This can be somewhat improved by allowing newlines in the %(JavaArtifact) value a'la https://github.com/xamarin/Java.Interop/pull/1086#discussion_r1126972799:

<PackageReference
    Include="Xamarin.Kotlin.StdLib" 
    Version="1.7.10" 
    JavaArtifact="
        org.jetbrains.kotlin:kotlin-stdlib:1.7.10
        ;org.jetbrains.kotlin:kotlin-stdlib-ktx:1.7.10
        ;insert.another:package.here:1.2.3"
/>

XML would permit this, it's just that whatever parses %(JavaArtifact) would also need to support newlines.

Additionally, this doesn't feel "scalable"; it feels like it'll need to be copied "everywhere". For example, you have Binding.csproj which binds multiple Java libraries, and then you have N projects which use <ProjectReference Include="…\Binding.csproj" />. Presumably every one of these N projects would also need to be updated to mention %(JavaArtifact)?

For existing NuGet packages, needing to mention %(JavaArtifact) at every callsite may be unavoidable. For new NuGet packages and @(ProjectgReference), it should be possible to instead have Binding.csproj provide the required information, instead of having every reference provide the required information.

A way to do this would be to use the <CallTarget/> task. IIRC MSBuild internally does this for lots of things, e.g. to compute the list of output @(Content) items which need to be copied into the referencing project, etc.

jonpryor commented 5 months ago

@jpobst asked:

Since we have only released this as a preview, would it be better to go further and scrap %(JavaVersion) altogether, and always use %(JavaArtifact) with a version?

Yes. And also we should find a way to not need to mention %(JavaArtifact) for most users.

And another plug for my above <CallTarget/> suggestion, NuGet packages should also be able to provide the %(JavaArtifact) information from .targets files included within the NuGet package. This means that for new NuGet packages, "callers"/users of @(PackageReference) wouldn't need to specify %(JavaArtifact) at all. %(JavaArtifact) would only need to be used for older packages.

jpobst commented 5 months ago

Yes. And also we should find a way to not need to mention %(JavaArtifact) for most users.

Note that we already have this today. Users using updated NuGet packages do not need to specify %(JavaArtifact). We parse the NuGet metadata tags for a well-defined string (that we need to add documentation for) to determine what artifact it binds. The issue is we currently only support a single tag, we need to extend this to support multiple tags:

https://github.com/xamarin/xamarin-android/blob/d6e47a84b9622fcea3d52ca8549e1e0b032173a4/src/Xamarin.Android.Build.Tasks/Tasks/JavaDependencyVerification.cs#L411-L419

@(ProjectReference) is more problematic, as you currently always have to specify %(JavaArtifact). Ideally we could scan the referenced project somehow and find them automatically but that doesn't exist yet. The currently existing mechanism of %(JavaArtifact) only supports specifying a single artifact.

NOTE: @(ProjectReference) enhancement has been moved to https://github.com/dotnet/android/issues/9093. This issue will only deal with supporting multiple libraries per deployment unit.

jasells commented 4 months ago

I vote in favor of this as a feature, especially for "framework" dependency scenarios.

Users using updated NuGet packages do not need to specify %(JavaArtifact). We parse the NuGet metadata tags for a well-defined string (that we need to add documentation for) to determine what artifact it binds.

You can see an example of how this could work in a package-project here.

I modded the prototype task to correctly resolve all dependencies in that package here. The usage is a standard package ref: <PackageReference Include="XDev.IO.Ktor" Version="2.3.2.5" /> in the consuming project file.

@(ProjectReference) is more problematic, as you currently always have to specify %(JavaArtifact). Ideally we could scan the referenced project somehow and find them automatically but that doesn't exist yet. The currently existing mechanism of %(JavaArtifact) only supports specifying a single artifact.

That's one reason why I went the route of packing the project with multiple native libs that I had when trying to use the original prototype task. The change to the task code was fairly trivial.