Skip to content
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

[XABT] Allow deployment artifacts to specify multiple JavaDependencyVerification JavaArtifact libraries. #9112

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Jul 15, 2024

Fixes: #9013

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. Instead of using JavaArtifact/JavaVersion to specify a Java dependency, we will only use JavaArtifact and we will update the specification to include the artifact version: org.jetbrains.kotlin:kotlin-stdlib:1.7.10. Additionally, this attribute now allows multiple Java artifacts to be specified by separating them with a comma, semicolon, or newline.

This change affects @(PackageReference), @(ProjectReference), @(AndroidLibrary), and @(AndroidIgnoredJavaDependency).

Examples:

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

<ProjectReference 
  Include="..\My.Other.Binding\My.Other.Binding.csproj" 
  JavaArtifact="my.other.binding:mylib:1.0.0,my.other.binding:helperlib:1.0.0" />

<AndroidLibrary 
  Include="mydependency.jar" 
  JavaArtifact="my.library:dependency-library:1.0.0" />

<AndroidIgnoredJavaDependency 
  Include="com.google.errorprone:error_prone_annotations:2.15.0" />

Existing support for looking at NuGet package tags to automatically find Java libraries contained within has also been updated to support multiple artifact tags. Note that separate tags are expected, not a single tag with multiple artifacts specified:

<!-- Correct -->
<PackageTags>
  artifact=my.other.binding:mylib:1.0.0
  artifact=my.other.binding:helperlib:1.0.0
</PackageTags>

<!-- Incorrect -->
<PackageTags>
  artifact=my.other.binding:mylib:1.0.0,my.other.binding:helperlib:1.0.0
</PackageTags>

@jpobst jpobst changed the title [XABT] Allow deployment artifacts to specify mutiple JavaDependencyVerification JavaArtifact libraries. [XABT] Allow deployment artifacts to specify multiple JavaDependencyVerification JavaArtifact libraries. Jul 15, 2024
@jpobst jpobst force-pushed the multi-version-java-artifact branch from c19adb8 to 6b87b0c Compare July 16, 2024 15:28
@jpobst jpobst marked this pull request as ready for review August 6, 2024 19:26
@jpobst jpobst requested a review from jonpryor August 6, 2024 19:26
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding all of the different delimiters options, should we use ; as the standard MSBuild delimiter? Could ; be used as the top-level delimiter?

I'm wondering if we can use MSBuild to parse/split the top-level data, and not have to do that ourselves.

@jpobst jpobst force-pushed the multi-version-java-artifact branch from ce00e69 to 7b8a54c Compare August 12, 2024 19:56
@jpobst
Copy link
Contributor Author

jpobst commented Aug 12, 2024

Regarding all of the different delimiters options, should we use ; as the standard MSBuild delimiter? Could ; be used as the top-level delimiter?

I don't really have a preference here, but this conflicts with @jonpryor's request to support newlines.

@jonathanpeppers
Copy link
Member

I tried to ask if there is a standard way for us to split like MSBuild does:

I will ask the same question in Teams, maybe we could get a better answer there.

@jpobst jpobst force-pushed the multi-version-java-artifact branch from 7b8a54c to bbfbf35 Compare August 13, 2024 23:15
@jonathanpeppers
Copy link
Member

I asked in Teams, but there isn't a standard thing for splitting MSBuild item metadata.

So, we can delimit on whichever character(s) we like.

@jonpryor
Copy link
Member

@jonathanpeppers wrote:

should we use ; as the standard MSBuild delimiter? Could ; be used as the top-level delimiter?

My concern is legibility: I find "very long lines" difficult to read.

Earlier this year I was looking into some stripe-binding related question, and had a reason to play with @(AndroidMavenLibrary), resulting in a .csproj with 43 @(AndroidMavenLibrary) items. Were I to reference this project from another project, this would presumably require a %(ProjectReference.JavaArtifacts) value with 43 items (!):

    <ProjectReference Include="..\stripe_terminal\stripe_terminal.csproj"
        JavaArtifacts="com.google.code.gson:gson:2.10.1;com.google.dagger:dagger:2.51;com.jaredrummler:android-device-names:1.1.9;com.neovisionaries:nv-i18n:1.29;com.scottyab:rootbeer-lib:0.1.0;…"

(My <CallTarget/> suggestion would mean that %(JavaArtifacts) isn't needed at all…, but that's not done here.)

I don't find the above to be readable, and it's not even complete yet!

Staying within MSBuild, we could introduce another item group:

<ItemGroup>
  <_StripJavaArtifact Include="com.google.code.gson:gson:2.10.1" />
  <_StripJavaArtifact Include="com.google.dagger:dagger:2.51" />
  <_StripJavaArtifact Include="com.jaredrummler:android-device-names:1.1.9" />
  <_StripJavaArtifact Include="com.neovisionaries:nv-i18n:1.29" />
  <_StripJavaArtifact Include="com.scottyab:rootbeer-lib:0.1.0" />
  <!---->
  <ProjectReference
      Include="..\stripe_terminal\stripe_terminal.csproj"
      JavaArtifacts="@(_StripJavaArtifact)"
   />
</ItemGroup>

Which works, but as soon as you have multiple items which require %(JavaArtifacts), you need to introduce new names for each such "containing" item.

This is why I was advocating for newlines and trimming/ignoring whitespace, which would allow "inlining" things:

    <ProjectReference Include="..\stripe_terminal.csproj"
        JavaArtifacts="
          com.google.code.gson:gson:2.10.1
          com.google.dagger:dagger:2.51
          com.jaredrummler:android-device-names:1.1.9
          com.neovisionaries:nv-i18n:1.29
          com.scottyab:rootbeer-lib:0.1.0
"
    />

Additionally, MSBuild default behavior of turning ;-separated strings into an item group doesn't help us, as %(JavaArtifact) is a string, not an item group.

I thus believe we should split on ;, ,, and whitespace (space, tab, newline, etc.).


// Convert "../../src/blah/Blah.csproj" to "Blah.csproj"
if (type == "ProjectReference")
item_name = Path.GetFileName (item_name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want to do this? What if the same project name is used multiples but for different paths: Lib1/Helper.csproj, Lib2/Helper.csproj, …?

artifacts.Add (art.ArtifactString, art);
if (MavenExtensions.TryParseArtifacts (id, log, out var parsed)) {
foreach (var art in parsed) {
log.LogMessage ("Ignoring Java dependency '{0}:{1}' version '{2}'", art.GroupId, art.Id, art.Version);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we're now standardizing on "group:id:version" within %(JavaArtifacts), should we use that here as well?

@jonpryor jonpryor merged commit e70ae00 into main Aug 20, 2024
58 checks passed
@jonpryor jonpryor deleted the multi-version-java-artifact branch August 20, 2024 19:20
grendello added a commit that referenced this pull request Aug 27, 2024
* main: (47 commits)
  Bump to dotnet/sdk@5642787dac 9.0.100-rc.2.24426.2 (#9247)
  LEGO: Merge pull request 9246
  Bump to 34.0.137 of the .NET 8 Android workload (#9243)
  Bump external/Java.Interop from `d30d554` to `51b784a` (#9241)
  Bump dotnet/android-tools@6575743 (#9235)
  Bump to mono/debugger-libs@d5664344 (#9238)
  [ci] Improve push_signed_nugets job condition (#9240)
  Bump to dotnet/android-tools@657574378a6 and xamarin/monodroid@8bd4bae7 (#9216)
  Bump to dotnet/java-interop@d30d554 (#9234)
  Localized file check-in by OneLocBuild Task (#9236)
  Bump to dotnet/sdk@e2b7b9d2b4 9.0.100-rc.2.24420.1 (#9228)
  $(AndroidPackVersionSuffix)=rc.2; net9 is 35.0.0-rc.2 (#9233)
  [Xamarin.Android.Build.Tasks] Scan for JCWs for each ABI in parallel (#9215)
  [Xamarin.Android.Build.Tasks] %(JavaArtifact) is a list (#9112)
  [native/monodroid] Fix error demangling satellite assembly names (#9166)
  [build] Update package metadata (#9230)
  [Xamarin.Android.Build.Tasks] Remove ILRepack (#9226)
  [Xamarin.Android.Build.Tasks] Fix an issue with incremental builds (#9183)
  [Xamarin.Android.Build.Tasks] remove `$XAMARIN_BUILD_ID` (#9223)
  [Mono.Android] Use .NET version of mdoc (#9225)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 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.

Support multiple-binding projects in Java Dependency Verification
3 participants