[Java.Interop.Tools.Maven] Initial commit. #1179

jpobst · 2024-01-12T20:23:51Z

Create a new Java.Interop.Tools.Maven.dll assembly that contains functionality for:

Retrieving artifacts from Maven
Parsing and understanding POM files

This assembly is used by dotnet/android#8649 and replaces our previous usage of MavenNet.dll.

src/Java.Interop.Tools.Maven/Repositories/InMemoryMavenRepository.cs

jonpryor · 2024-02-13T18:47:31Z

src/Java.Interop.Tools.Maven/Extensions/PropertyStack.cs

+	{
+		foreach (var property_set in stack) {
+			foreach (var prop in property_set)
+				value = value.Replace ($"${{{prop.Key}}}", prop.Value);


This gives me some concern, if only because this could produce lots of garbage, all depending on stack.

I think I'd prefer a "optimization check" + use of StringBuilder.Replace():

if (stack.Count == 0 || !value.Contains("${") { return value; } var s = new StringBuilder (value); foreach (var property_set in stack) { foreach (var prop in property_set) { s.Replace ($"${{{prop.Key}}}", prop.Value); } } return s.ToString ();

Added the optimization check. 👍

From my research looking at POM files, this code will rarely be hit with the optimization check, and when it is hit it will generally be for small numbers of properties. The general guidance seems to be that string.Replace is faster than StringBuilder.Replace for small numbers of replaces, despite the extra garbage, as it avoids the overhead of creating the StringBuilder and converting it ToString at the end.

I don't recall reading the guidance about "string.Replace is faster … for small numbers of replace". Would be interesting to read more.

jonpryor · 2024-02-13T18:53:43Z

Convention elsewhere is to use src/[Assembly Base Name]/[Dotted Namespace Name]/[Type Name].cs.

We should thus have:

src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven/DefaultPomResolver.cs
src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.Extensions/MavenNetExtensions.cs
etc.

I also wonder if the "sub-namespaces" are being too granular here; do we really need .Extensions, .Models, .Repositories? You have a naming convention of *Repository as a suffix, and there's not that many types here, so would it be "bad" to just have them all in the Java.Interop.Tools.Maven namespace?

jonpryor · 2024-02-13T19:00:28Z

src/Java.Interop.Tools.Maven/Models/Project.cs

+    [System.Diagnostics.DebuggerStepThroughAttribute()]
+    [System.ComponentModel.DesignerCategoryAttribute("code")]
+    [System.Xml.Serialization.XmlRootAttribute("project")]
+    public partial class Project


…then again, maybe we want generated code to go into its own sub-namespace…?

jonpryor · 2024-02-13T19:00:57Z

src/Java.Interop.Tools.Maven/Models/Project.cs

+
+// https://github.com/mganss/XmlSchemaClassGenerator
+// This code was generated by XmlSchemaClassGenerator version 2.1.1057.0 using the following command:
+// xscgen maven-4.0.0.xsd --nullable --nc --nr


Where is maven-4.0.0.xsd? Should that be added to this project for "reference" purpsoes?

I don't think we should add it to our repo, but adding a URL for it to the comment seems like a good idea.

Given that this is a generated file, should we move this comment block & related into Project.Partial.cs, so that we don't have to manually update this file whenever we regenerate it?

Relatedly, I imagine you didn't build mganss/XmlSchemaClassGenerator yourself, but instead used a NuGet package such as https://www.nuget.org/packages/XmlSchemaClassGenerator.Console. Would be useful to put this into a .targets file to make updating easier.

jonpryor · 2024-02-13T19:07:30Z

src/Java.Interop.Tools.Maven/Repositories/IMavenRepository.cs

+
+public interface IMavenRepository
+{
+	// This name is used for on-disk caching purposes, and thus must be compatible with file system naming rules.


Not sure if this means that directory names can also be included or not. Is foo/bar.xml valid?

If directories names can't be included, perhaps this should be named FileName?

Basically we need to place each Maven repository in its own directory in our cache to prevent collisions if they contain the same Java artifact. In this example, it is "central" or "google":

- /MavenCacheDirectory - /central/androidx.core/core/1.9.0/core-1.9.0.aar - /google/androidx.core/core/1.9.0/core-1.9.0.aar

I will try to describe it a little better in the comment.

jonpryor · 2024-02-13T19:19:04Z

src/Java.Interop.Tools.Maven/Repositories/MavenRepository.cs

+		Name = name;
+		base_url = baseUrl;
+
+		client = new HttpClient {


General guidelines for HttpClient usage is that HttpClient instances should either be long-lived and shared (and static) or through IHttpClientFactory (though I'm not quite sure what that actually looks like in practice…).

newing up and ~immediately disposing of HttpClient instances is not a good idea.

See also: https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-net-http-httpclient#instancing

Related: HttpClient.BaseAddress is a read+write property.

This shouldn't have been too bad, as MavenRepository itself is generally expected to be long-lived, but switched to using a static HttpClient to be better.

jonpryor · 2024-02-13T19:20:00Z

src/Java.Interop.Tools.Maven/Repositories/MavenRepository.cs

+		return true;
+	}
+
+	public static MavenRepository Google = new MavenRepository ("https://dl.google.com/android/maven2/", "google");


This should be readonly, as I can't imagine a scenario where someone should be able to do MavenRepository.Google = someOtherValue.

jonpryor · 2024-02-13T19:20:13Z

src/Java.Interop.Tools.Maven/Repositories/MavenRepository.cs

+
+	public static MavenRepository Google = new MavenRepository ("https://dl.google.com/android/maven2/", "google");
+
+	public static MavenRepository Central = new MavenRepository ("https://repo1.maven.org/maven2/", "central");


This should be readonly.

jonpryor · 2024-02-13T19:33:16Z

src/Java.Interop.Tools.Maven/Repositories/CachedMavenRepository.cs

+/// <summary>
+/// Wraps an <see cref="IMavenRepository"/> and caches files in a local directory.
+/// </summary>
+public class CachedMavenRepository : IMavenRepository


Naming question: I think this should be named MavenRepositoryCache.

Rationale/5 minutes of investigation: consider dotnet/runtime, commit dotnet/runtime@4765dd1:

# class contains `Cache` % git grep 'public.*class.*Cache' src/libraries | wc -l 83 # class uses `Cached` % git grep 'public.*class.*Caching' src/libraries | wc -l 1 # class uses `Caching` % git grep 'public.*class.*Caching' src/libraries | wc -l 10 # class ends with `Cache` % git grep 'public.*class.*Cache\>' src/libraries | wc -l 20

Type names containing Cache include:

CacheEntryExtensions

MemoryCacheEntryExtensions

These don't feel like a good pattern to me, even if this has the most matches.

Cached has one match:

XmlCachedSchemaSetResolver

Cache as a suffix feels better:

MemoryDistributedCache

RuleCache<T>

CredentialCache

Caching, meanwhile, only shows up as sub-namespaces:

src/libraries/Microsoft.Extensions.Caching.Memory/ref/Microsoft.Extensions.Caching.Memory.cs: public partial class MemoryDistributedCache : Microsoft.Extensions.Caching.Distributed.IDistributedCache src/libraries/Microsoft.Extensions.Caching.Memory/ref/Microsoft.Extensions.Caching.Memory.cs: public partial class MemoryCache : Microsoft.Extensions.Caching.Memory.IMemoryCache, System.IDisposable src/libraries/Microsoft.Extensions.Caching.Memory/ref/Microsoft.Extensions.Caching.Memory.cs: public partial class MemoryCacheOptions : Microsoft.Extensions.Options.IOptions<Microsoft.Extensions.Caching.Memory.MemoryCacheOptions> src/libraries/Microsoft.Extensions.Caching.Memory/ref/Microsoft.Extensions.Caching.Memory.cs: public partial class MemoryDistributedCacheOptions : Microsoft.Extensions.Caching.Memory.MemoryCacheOptions src/libraries/Microsoft.Extensions.Diagnostics/tests/DefaultMetricsFactoryTests.cs: public class NoCachingMeterFactory : IMeterFactory src/libraries/System.Linq.Expressions/tests/Dynamic/CallSiteCachingTests.cs: public class CallSiteCachingTests src/libraries/System.Runtime.Caching/ref/System.Runtime.Caching.cs: public abstract partial class CacheEntryChangeMonitor : System.Runtime.Caching.ChangeMonitor src/libraries/System.Runtime.Caching/ref/System.Runtime.Caching.cs: public abstract partial class FileChangeMonitor : System.Runtime.Caching.ChangeMonitor src/libraries/System.Runtime.Caching/ref/System.Runtime.Caching.cs: public sealed partial class HostFileChangeMonitor : System.Runtime.Caching.FileChangeMonitor src/libraries/System.Runtime.Caching/ref/System.Runtime.Caching.cs: public partial class MemoryCache : System.Runtime.Caching.ObjectCache, System.Collections.IEnumerable, System.IDisposable

So Caching feels like a bad idea.

I think MavenRepositoryCache implies that this is a cache of MavenRepository, which it is not.

It is a wrapper around a MavenRepository which adds caching features to it.

It feels like CachedMavenRepository is better as MavenRepository is the noun and Cached (or Caching) is the adjective describing the modification to the MavenRepository.

jonpryor · 2024-02-13T19:36:06Z

src/Java.Interop.Tools.Maven/Repositories/CachedMavenRepository.cs

+			using (var sw = File.Create (file))
+				repo_stream!.CopyTo (sw);
+
+			using (repo_stream) { }


This feels weird.

I think this would feel better:

using (var sw = File.Create (file)) using (repo_stream) { repo_stream.CopyTo (sw); }

Indeed that is better.

jonpryor · 2024-02-13T19:37:05Z

src/Java.Interop.Tools.Maven/Repositories/CachedMavenRepository.cs

+
+		if (repository.TryGetFile (artifact, filename, out var repo_stream)) {
+			using (var sw = File.Create (file))
+				repo_stream!.CopyTo (sw);


Since you've annotated TryGetFile() with [NotNullWhen (true)] elsewhere, why is repo_stream! required?

It doesn't appear to be, it was likely meant to be temporary. Removed.

src/Java.Interop.Tools.Maven/Repositories/CachedMavenRepository.cs

jonpryor · 2024-02-13T19:40:33Z

tests/Java.Interop.Tools.Maven-Tests/Extensions/TestDataExtensions.cs

+		};
+
+		if (parent is not null)
+			p.Parent = new Parent {


This makes me wonder if there should be a Parent(Parent) constructor…

Honestly, a better case could be made that Project should not have a public constructor, so I made it internal and changed this to generate xml that can be deserialized.

jonpryor · 2024-02-13T19:41:26Z

tests/Java.Interop.Tools.Maven-Tests/Extensions/TestDataExtensions.cs

+
+static class TestDataExtensions
+{
+	public static Project CreateProject (Artifact artifact, Project? parent = null)


Given that this is within an *Extensions class, I'm surprised this isn't an extension method.

Meh, it's a test assembly and I didn't want an entire class for it. 😁

jpobst · 2024-02-14T23:34:49Z

Convention elsewhere is to use src/[Assembly Base Name]/[Dotted Namespace Name]/[Type Name].cs.

Unfortunately this convention seems outdated, as modern tooling does not play nicely with it.

For one, it's kinda annoying that it forces a minimum size of a file explorer in order to know which directory you want to expand:

The other issue is that the "new item" templating system builds your namespace based on [Assembly Base Name].[Dotted Namespace Name]

Thus adding a new class Foo.cs to the Java.Interop.Tools.JavaCallableWrappers folder in VS results in:

namespace Java.Interop.Tools.JavaCallableWrappers.Java.Interop.Tools.JavaCallableWrappers
{
  class Foo
  {
  }
}

I also wonder if the "sub-namespaces" are being too granular here; do we really need .Extensions, .Models, .Repositories?

This is largely related. I don't really care about the "sub-namespaces", but I do want them to be in "sub-folders" rather than all sitting in the project root. Then when you do Add New Item VS automatically puts them in the matching "sub-namespace".

I've always just manually fixed them before to not be in "sub-namespaces", but I thought this seems to be the way modern .NET tooling is pushing and maybe I should stop fighting it on new projects. 🤷‍♂️

I can remove the "sub-namespaces" if needed.

jonpryor · 2024-02-21T20:29:21Z

src/Java.Interop.Tools.Maven/DefaultPomResolver.cs

+
+	public virtual Project ResolveRawProject (Artifact artifact)
+	{
+		if (poms.TryGetValue (artifact.ToString (), out var project))


It feels "implicitly weird" that we require that Project.ToString() be identical to Artifact.ToString(). While Artifact.ToString() does have a comment stating "This is a "well-known" format we use, it should not be changed", Project.ToString() doesn't (currently), nor is there a comment stating that these formats should be consistent with each other.

I think it would be clearer and more explicit if we instead had "something" that was identically named between the two types to name the association. Possible ideas:

ArtifactString ("rhymes" with ResolvedDependency.ToArtifactString())

Group_Artifact_Version (that's what it contains…)

I went with ArtifactString and VersionedArtifactString throughout the codebase to make this more explicit and consistent.

jonpryor · 2024-02-21T20:30:11Z

src/Java.Interop.Tools.Maven/IPomResolver.cs

+
+public interface IPomResolver
+{
+	Project ResolveRawProject (Artifact artifact);


What is "Raw" serving in this name? Would this be better as just ResolveProject()?

Leftover from earlier designs, changed simply to Resolve.

jonpryor · 2024-02-21T20:30:54Z

src/Java.Interop.Tools.Maven/IPomResolver.cs

+
+namespace Java.Interop.Tools.Maven;
+
+public interface IPomResolver


Naming? Why not name this IProjectResolver, as it (currently) only resolves Projects? Is it so we can resolve other things in the future while reusing the same abstraction?

Pom and Project are synonyms, standardized on Project.

jonpryor · 2024-02-21T20:32:30Z

src/Java.Interop.Tools.Maven/Extensions/MavenNetExtensions.cs

+
+static class MavenNetExtensions
+{
+	public static string GetInheritedProperty (this ResolvedDependency dependency, ResolvedProject project, Func<ResolvedDependency, string?> property)


Why extension methods and not actual methods, when the type you're providing extension methods for is in the same assembly? Is this so that the method isn't visible via IntelliSense by default?

Moved to actual methods.

jonpryor · 2024-02-21T20:38:57Z

src/Java.Interop.Tools.Maven/Models/MavenVersion.cs

+
+	MavenVersion (string rawVersion) => RawVersion = rawVersion;
+
+	public static MavenVersion Parse (string version)


Should this mirror Artifact and have a TryParse() method as well?

…except it can't fail…. Weird semantics, especially around .IsValid…

Yeah, it is weird. There doesn't seem to be any validation on Maven version numbers, users can't technically put anything they want. If it isn't a "valid" Maven version, it just gets treated like an opaque string for sorting and version matching purposed.

If you do not follow Maven versioning standards in your project versioning scheme, then for version comparison, Maven interprets the entire version as a simple string.

From https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN8855.

jonpryor · 2024-02-21T21:14:49Z

src/Java.Interop.Tools.Maven/Models/Project.Partial.cs

+
+public partial class Project
+{
+	public static Project Parse (Stream stream)


Given that Stream will also contain XML, why have a ParseXml() method vs naming both (Stream) and (string) overloads Parse()?

Relatedly, there should probably also be a Parse(XmlTextReader) overload, so that both Parse(Stream) and Parse(string) can share most of their logic.

Alternate naming logic: do what XDocument does, which has:

XDocument.Load() loads from Streams, XmlReaders, etc. Load(string) loads from a file.

XDocument.Parse() loads from XML.

Thus, I think we should rename Parse(Stream) to Load(Stream), add a Load(XmlReader) overload, and rename ParseXml(string) to Parse(string):

public static Project Load(XmlReader reader) { var serializer = new XmlSerializer (typeof (Project)); return (Project) serializer.Deserialize (reader); } public static Project Load(Stream stream) { return Load (new XmlTextReader (stream) { Namespaces = false; }); } public static Project Parse(string xml) { using (var sr = new StringReader (xml)) return Load (new XmlTextReader (sr) { Namespaces = false; }); }

I use Parse and ParseXml to roughly match XmlDocument.Load and XmlDocument.LoadXml for the reason you state: Parse/Load (string) generally refers to a file rather than an XML snippet.

But Load and Parse work as well.

jonpryor · 2024-02-21T21:23:48Z

src/Java.Interop.Tools.Maven/Models/Project.Partial.cs

+
+		var serializer = new XmlSerializer (typeof (Project));
+
+		using (var sr = new StreamReader (stream))


Why does this wrap a Stream in a StreamReader? The XmlTextReader(Stream) constructor exists.

jonpryor · 2024-02-21T21:26:38Z

src/Java.Interop.Tools.Maven/Models/ResolvedProject.cs

+
+	void ResolveCore (PropertyStack properties)
+	{
+		if (IsSuperPom)


Why is it called a "super POM" instead of a "root POM"? Maybe it's just me, but "super" doesn't imply (to me) that it has no parents; "root" does. (/ has no parent directory, C:\ has no parent directory, etc.)

…and "super POM" might be "better" in that it's the existing terminology of the space, but it doesn't feel "self documenting", as it were.

Correct, using Maven's terminology for this concept:
https://maven.apache.org/guides/introduction/introduction-to-the-pom.html#super-pom

jonpryor · 2024-02-21T21:32:17Z

src/Java.Interop.Tools.Maven/Repositories/MavenRepository.cs

+		base_url = baseUrl.TrimEnd ('/');
+	}
+
+	static MavenRepository ()


Any reason to have an explicit static constructor vs. just initializing in the field?

jonpryor · 2024-02-21T22:10:38Z

Addendum/followup to my comment:

this patch is what I'm thinking:

diff --git a/NuGet.Config b/NuGet.Config
index 7d88f585..4e263ee0 100644
--- a/NuGet.Config
+++ b/NuGet.Config
@@ -6,7 +6,6 @@
 -->
 <configuration>
   <packageSources>
-    <clear />
     <add key="dotnet-public" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json" protocolVersion="3" />
     <!-- For Microsoft.DotNet.GenAPI -->
     <add key="dotnet-eng" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json" protocolVersion="3" />
diff --git a/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.csproj b/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.csproj
index 9c0f50dc..9b296e87 100644
--- a/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.csproj
+++ b/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.csproj
@@ -22,6 +22,13 @@
 
   <ItemGroup>
     <PackageReference Include="Microsoft.SourceLink.GitHub" PrivateAssets="All" />
+    <PackageReference
+        Include="XmlSchemaClassGenerator.Console"
+        Version="2.1.1057"
+        ReferenceOutputAssembly="false"
+        GeneratePathProperty="true"
+    />
   </ItemGroup>
 
+  <Import Project="Java.Interop.Tools.Maven.targets" />
 </Project>
diff --git a/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.targets b/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.targets
new file mode 100644
index 00000000..2d6016c2
--- /dev/null
+++ b/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.targets
@@ -0,0 +1,18 @@
+<Project>
+  <Target Name="UpdateProjectSchema"
+        AfterTargets="ResolveAssemblyReferences">
+    <PropertyGroup>
+      <_Xscgen>"$(PkgXmlSchemaClassGenerator_Console)/tools/net462/XmlSchemaClassGenerator.Console.exe"</_Xscgen>
+      <MavenXsd Condition=" '$(MavenXsd)' == '' ">maven-4.0.0.xsd</MavenXsd>
+    </PropertyGroup>
+    <ItemGroup>
+      <_XscgenOpt Include="$(MavenXsd)" />
+      <_XscgenOpt Include="--nullable" />
+      <_XscgenOpt Include="--nc" />
+      <_XscgenOpt Include="--nr" />
+      <_XscgenOpt Include="-o" />
+      <_XscgenOpt Include="&quot;$(MSBuildThisFileDirectory)&quot;" />
+    </ItemGroup>
+    <Exec Command="$(Runtime) $(_Xscgen) @(_XscgenOpt, ' ')" />
+  </Target>
+</Project>

which I then executed with:

% curl -o maven-4.0.0.xsd 'https://maven.apache.org/xsd/maven-4.0.0.xsd'
% dotnet build src/Java.Interop.Tools.Maven/*.csproj -t:UpdateProjectSchema -p:MavenXsd=`pwd`/maven-4.0.0.xsd

There are multiple problems with this approach, which are all solvable:

It re-adds NuGet.org to NuGet.config. We can't actually do this; we'll need to add XmlSchemaClassGenerator.Console to dotnet-public.
I'm not sure that xscgen maven-4.0.0.xsd --nullable --nc --nr was the actually used command-line, because when I did that, as with the above patch, it generated a Pom.cs file, and the names were different.

And by different, Pom.cs has:

namespace Pom
{
    public partial class Model {}
    // …
}

This PR (#1179) does not contain a partial class Model type. Where did it go? Similarly, you have a partial class Project, but my Pom.cs does not have a Project type declared.

I'm not using "xscgen", but I am using the NuGet package that the github repo links to, with the command-line arguments you mention, but the output doesn't match.

Am I using the right XSD? https://maven.apache.org/xsd/maven-4.0.0.xsd?

jonpryor · 2024-02-22T16:14:34Z

@jpobst: I'm increasingly wondering how you created Project.cs. This is the closest I've gotten to Project.cs:

% mono $HOME/.nuget/packages/xmlschemaclassgenerator.console/2.1.1057/tools/net462/XmlSchemaClassGenerator.Console.exe \
    maven-4.0.0.xsd \
    --namespace http://maven.apache.org/POM/4.0.0=Java.Interop.Tools.Maven.Models \
    --typeNameSubstitute T:Model=Project \
    --nullable --netCore --nullableReferenceAttributes \
    -o _o

Using --namespace http://maven.apache.org/POM/4.0.0=Java.Interop.Tools.Maven.Models allows me to have namespace Java.Interop.Tools.Maven.Models.

Using --typeNameSubstitute T:Model=Project allows me to have partial class Project instead of partial class Model.

However, there's still lots of differences:

% git diff --no-index -w Models/Project.cs _o/Java.Interop.Tools.Maven.Models.cs

@@ -26,10 +25,10 @@ namespace Java.Interop.Tools.Maven.Models
     [System.ComponentModel.DescriptionAttribute(@"3.0.0+ The &lt;code&gt;&amp;lt;project&amp;gt;&lt;/code&gt; element is the root of the descriptor. The following table lists all of the possible child elements. 3.0.0+ The &lt;code&gt;&amp;lt;project&amp;gt;&lt;/code&gt; element is the root of the descriptor. The following table lists all of the possible child elements.")]
     [System.CodeDom.Compiler.GeneratedCodeAttribute("XmlSchemaClassGenerator", "2.1.1057.0")]
     [System.SerializableAttribute()]
-    [System.Xml.Serialization.XmlTypeAttribute("Project")]
+    [System.Xml.Serialization.XmlTypeAttribute("Model", Namespace="http://maven.apache.org/POM/4.0.0")]
     [System.Diagnostics.DebuggerStepThroughAttribute()]
     [System.ComponentModel.DesignerCategoryAttribute("code")]
-    [System.Xml.Serialization.XmlRootAttribute("project")]
+    [System.Xml.Serialization.XmlRootAttribute("project", Namespace="http://maven.apache.org/POM/4.0.0")]
     public partial class Project
     {

Even though we renamed Model to Project, we still have XmlTypeAttribute("Model"). This feels like a bug in xcsgen. Additionally, we have lots of Namespace="…" properties on the XmlRootAttribute custom attribute; this happens throughout, and I won't be repeating them.

@@ -225,9 +224,9 @@ namespace Java.Interop.Tools.Maven.Models
         }
         
         /// <summary>
-        /// <para xml:lang="en">Initializes a new instance of the <see cref="Model" /> class.</para>
+        /// <para xml:lang="en">Initializes a new instance of the <see cref="Project" /> class.</para>
         /// </summary>
-        internal Project()
+        public Project()
         {
             this._licenses = new System.Collections.ObjectModel.Collection<License>();
             this._developers = new System.Collections.ObjectModel.Collection<Developer>();

Your comment mentions Model, even though it's on Project. Your constructor is internal when the generated one is public. This feels like the generated file has been manually edited after generation.

@@ -2593,17 +2592,17 @@ namespace Java.Interop.Tools.Maven.Models
     
     [System.CodeDom.Compiler.GeneratedCodeAttribute("XmlSchemaClassGenerator", "2.1.1057.0")]
     [System.SerializableAttribute()]
-    [System.Xml.Serialization.XmlTypeAttribute("ModelProperties", AnonymousType=true)]
+    [System.Xml.Serialization.XmlTypeAttribute("ModelProperties", Namespace="http://maven.apache.org/POM/4.0.0", AnonymousTyp
e=true)]
     [System.Diagnostics.DebuggerStepThroughAttribute()]
     [System.ComponentModel.DesignerCategoryAttribute("code")]
     public partial class ModelProperties
     {
         
         [System.Xml.Serialization.XmlIgnoreAttribute()]
-        private System.Collections.ObjectModel.Collection<System.Xml.Linq.XElement> _any;
+        private System.Collections.ObjectModel.Collection<System.Xml.XmlElement> _any;
         
         [System.Xml.Serialization.XmlAnyElementAttribute()]
-        public System.Collections.ObjectModel.Collection<System.Xml.Linq.XElement> Any
+        public System.Collections.ObjectModel.Collection<System.Xml.XmlElement> Any
         {
             get
             {

More additions of XmlTypeAttribute.Namespace (which I said I'd ignore…), but more oddly is the collection type: you have Collection<XElement>, I got Collection<XmlElement>.

The only way I could find to get xcsgen to emit XElement is to use --pcl, which does allow it to use XElement, but also introduces tons of other changes, removing lots System.ComponentModel.DescriptionAttribute. Additionally, your Project.cs contains both Collection<System.Xml.XmlElement> (e.g. DeveloperProperties._any) and Collection<System.Xml.Linq.XElement> (e.g. ModelProperties._any). Again, this feels like the generated file was manually edited.

If I checkout https://github.com/mganss/XmlSchemaClassGenerator and build xcsgen.proj and run dotnet path/to/xcsgen.dll instead of using mono XmlSchemaClassGenerator.Console.exe, I see the same differences.

jonpryor · 2024-02-22T16:29:53Z

@jpobst: I'm also increasingly thinking that my above comments about the differences between my generated Project.cs and the Project.cs within this PR are barking up the wrong tree:

This is processing maven-4.0.0.xsd. There is also a maven-3.0.0.xsd as well as a maven-4.1.0-alpha-10.xsd, implying that 4.1 will eventually come out, and maybe 5.0, or 6.0, or…

Why should the generated types be public? There is a XmlSchemaClassGenerator.Console.exe --assemblyVisible option which makes the generated types internal. Using internal visibility would allow us to:

Put the generated types into a versioned sub-namespace, e.g. Java.Interop.Tools.Maven.Models.v4_0, all internal
Have a "version agnostic" Project type which we full control, which wraps an appropriate version-specific type. Project.Load(Stream) could support v4.0 now, and v5.0 in the future, and because we fully control the Project API, we can ensure this doesn't break anybody.

However, this idea mirrors that of "full bindings for .jar files" vs. "slim bindings": making xcsgen output public means a lower barrier to entry (less work to write a Project wrapper), and means that other assemblies using Java.Interop.Tools.Maven have full access to everything in the XML. The downside is we lose easy control for future versioning -- what happens if v5 is significantly different from v4? -- and "expose everything in the XML" might make for an unwieldy API.

I'm currently inclined to "make xcsgen output internal" and put into a versioned namespace. Even if we want xcsgen output to be public, I still think it should be in a versioned sub-namespace, and we could do:

namespace Java.Interop.Tools.Maven.Models {
    // All hand-written
    public partial class Project {};
}
namespace Java.Interop.Tools.Maven.Models.v4_0 {
    // generated output, using the default names, e.g. `Model` instead of `Project`
    public partial class Model {}

    // hand-written partial class declaration
    public partial class Model : Project {}
}

This way, generated "stuff" is off in a separate (versioned) namespace, and we provide a common abstraction that we control in the form of Project.

jpobst · 2024-02-22T20:26:31Z

This feels like the generated file has been manually edited after generation.

Correct, the generated output was manually edited to suit our purposes (and to initially be compatible with https://github.com/Redth/MavenNet/blob/master/MavenNet/Models/PomMavenXsdModels.cs).

I think we're overthinking this. In general, we consider JI tool assemblies to be internal API. (For example, we changed the public API of JavaCallableWrappers in #1174.) So I don't think we should be very concerned with versioning the schema class or potential future API breakages.

We likely won't update the schema class unless a new version includes new dependency resolution features that we want to support.

jonpryor · 2024-02-24T18:53:35Z

@jpobst wrote:

I think we're overthinking this.

Indeed!

That said, even ignoring the API compatibility concerns, I generally prefer that generated stuff remain generatable.

To that end, I've added XmlSchemaClassGenerator.Console to the dotnet-public feed, which -- after a Version update -- should allow my previous patch to work.

That leaves the question of XElement vs. XmlElement, and if we just use --pcl we'll consistently get XElement. We'll lose some custom attributes, but I don't think those are relevant for our purposes:

diff --git a/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.csproj b/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.csproj
index 9c0f50dc..2c9774a6 100644
--- a/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.csproj
+++ b/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.csproj
@@ -22,6 +22,13 @@
 
   <ItemGroup>
     <PackageReference Include="Microsoft.SourceLink.GitHub" PrivateAssets="All" />
+    <PackageReference
+        Include="XmlSchemaClassGenerator.Console"
+        Version="2.1.1107"
+        ReferenceOutputAssembly="false"
+        GeneratePathProperty="true"
+    />
   </ItemGroup>
 
+  <Import Project="Java.Interop.Tools.Maven.targets" />
 </Project>
diff --git a/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.targets b/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.targets
new file mode 100644
index 00000000..f57c545b
--- /dev/null
+++ b/src/Java.Interop.Tools.Maven/Java.Interop.Tools.Maven.targets
@@ -0,0 +1,20 @@
+<Project>
+  <Target Name="UpdateProjectSchema"
+        AfterTargets="ResolveAssemblyReferences">
+    <PropertyGroup>
+      <_Xscgen>"$(PkgXmlSchemaClassGenerator_Console)/tools/net462/XmlSchemaClassGenerator.Console.exe"</_Xscgen>
+      <MavenXsd Condition=" '$(MavenXsd)' == '' ">maven-4.0.0.xsd</MavenXsd>
+    </PropertyGroup>
+    <ItemGroup>
+      <_XscgenOpt Include="$(MavenXsd)" />
+      <_XscgenOpt Include="--namespace http://maven.apache.org/POM/4.0.0=Java.Interop.Tools.Maven.Models" />
+      <_XscgenOpt Include="--typeNameSubstitute T:Model=Project" />
+      <_XscgenOpt Include="--nullable" />
+      <_XscgenOpt Include="--pcl" />
+      <_XscgenOpt Include="--netCore" />
+      <_XscgenOpt Include="--nullableReferenceAttributes" />
+      <_XscgenOpt Include="-o &quot;Models&quot;" />
+    </ItemGroup>
+    <Exec Command="$(Runtime) $(_Xscgen) @(_XscgenOpt, ' ')" />
+  </Target>
+</Project>

This results in a rather sizeable 3348 line diff between Models/Project.cs and Models/Java.Interop.Tools.Maven.cs (not sure how to control the output filename), most of which looks irrelevant:

@@ -23,13 +22,9 @@ namespace Java.Interop.Tools.Maven.Models
     /// <para>The &lt;code&gt;&amp;lt;project&amp;gt;&lt;/code&gt; element is the root of the descriptor.
     ///        The following table lists all of the possible child elements.</para>
     /// </summary>
-    [System.ComponentModel.DescriptionAttribute(@"3.0.0+ The &lt;code&gt;&amp;lt;project&amp;gt;&lt;/code&gt; element is the 
root of the descriptor. The following table lists all of the possible child elements. 3.0.0+ The &lt;code&gt;&amp;lt;project&a
mp;gt;&lt;/code&gt; element is the root of the descriptor. The following table lists all of the possible child elements.")]
-    [System.CodeDom.Compiler.GeneratedCodeAttribute("XmlSchemaClassGenerator", "2.1.1057.0")]
-    [System.SerializableAttribute()]
-    [System.Xml.Serialization.XmlTypeAttribute("Project")]
-    [System.Diagnostics.DebuggerStepThroughAttribute()]
-    [System.ComponentModel.DesignerCategoryAttribute("code")]
-    [System.Xml.Serialization.XmlRootAttribute("project")]
+    [System.CodeDom.Compiler.GeneratedCodeAttribute("XmlSchemaClassGenerator", "2.1.1107.0")]
+    [System.Xml.Serialization.XmlTypeAttribute("Model", Namespace="http://maven.apache.org/POM/4.0.0")]
+    [System.Xml.Serialization.XmlRootAttribute("project", Namespace="http://maven.apache.org/POM/4.0.0")]
     public partial class Project
     {
         
@@ -37,7 +32,6 @@ namespace Java.Interop.Tools.Maven.Models
         /// <para>4.0.0+</para>
         /// <para>Declares to which version of project descriptor this POM conforms.</para>
         /// </summary>
-        [System.ComponentModel.DescriptionAttribute("4.0.0+ Declares to which version of project descriptor this POM conforms.")]
         [System.Diagnostics.CodeAnalysis.AllowNullAttribute()]
         [System.Diagnostics.CodeAnalysis.MaybeNullAttribute()]
         [System.Xml.Serialization.XmlElementAttribute("modelVersion")]
… *lots* of DescriptionAttribute removals…
@@ -225,9 +201,9 @@ namespace Java.Interop.Tools.Maven.Models
         }
         
         /// <summary>
-        /// <para xml:lang="en">Initializes a new instance of the <see cref="Model" /> class.</para>
+        /// <para xml:lang="en">Initializes a new instance of the <see cref="Project" /> class.</para>
         /// </summary>
-        internal Project()
+        public Project()
         {
             this._licenses = new System.Collections.ObjectModel.Collection<License>();
             this._developers = new System.Collections.ObjectModel.Collection<Developer>();
…

Change: dotnet/java-interop@3436a30...5bca8ad * dotnet/java-interop@5bca8ad6: [build] Automatically add NullableAttributes.cs for netstandard2.0 (dotnet/java-interop#1188) * dotnet/java-interop@45437e22: [Java.Interop] suppress IL3050 with `#pragma` (dotnet/java-interop#1201) * dotnet/java-interop@1c9c8c9c: [Java.Interop.Tools.Maven] Initial commit. (dotnet/java-interop#1179) With dotnet/java-interop@5bca8ad6, all instances of nullable attributes types defined in `NullableAttributes.cs` are now `internal`. However, `Xamarin.Android.Build.Tasks` was consuming the `public` types from its reference to `Java.Interop.Tools.JavaCallableWrappers`, so this change resulted in a build break: MavenExtensions.cs(26,32): error CS0122: 'NotNullWhenAttribute' is inaccessible due to its protection level MamJsonParser.cs(92,43): error CS0122: 'NotNullWhenAttribute' is inaccessible due to its protection level MamJsonParser.cs(92,81): error CS0122: 'NotNullWhenAttribute' is inaccessible due to its protection level We can apply almost the same logic from dotnet/java-interop@5bca8ad6 to `xamarin-android`; the difference is that neither of the `netstandard2.0` projects in xamarin-android that need the attributes have `$(Nullable)='enabled'` and thus our `Condition` fails. (`Xamarin.Android.Build.Tasks.csproj` and `Xamarin.Android.Tools.JavadocImporter.csproj` include `.cs` files from `external/Java.Interop` that have been NRT annotated and thus need the nullable attribute types.) We add `$(Nullable)='annotations'` to the `@(Compile)` which allows one to use NRT syntax but does not emit any NRT warnings since these assemblies have not been converted. We then modify the `NullableAttributes.cs` inclusion logic to additionally key off the `$(Nullable)=annotations` value. Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jonathan Pobst <[email protected]>

jpobst force-pushed the java-interop-maven branch from 64b12e1 to c1b64bb Compare January 12, 2024 20:26

jpobst force-pushed the java-interop-maven branch 2 times, most recently from 8178ac5 to 1bb76b2 Compare January 24, 2024 22:36

jpobst changed the title ~~[Java.Interop.Maven] Initial commit.~~ [Java.Interop.Tools.Maven] Initial commit. Jan 24, 2024

jpobst force-pushed the java-interop-maven branch from 1bb76b2 to cf1efa0 Compare January 24, 2024 22:52

jpobst force-pushed the java-interop-maven branch 2 times, most recently from 2ac4f22 to d7be7c4 Compare February 6, 2024 17:38

jonpryor reviewed Feb 7, 2024

View reviewed changes

src/Java.Interop.Tools.Maven/Repositories/InMemoryMavenRepository.cs Outdated Show resolved Hide resolved

jpobst force-pushed the java-interop-maven branch from 60e7b95 to 52addaf Compare February 9, 2024 21:40

jpobst marked this pull request as ready for review February 10, 2024 01:24

jpobst mentioned this pull request Feb 10, 2024

[Xamarin.Android.Build.Tasks] Enable POM verification features. dotnet/android#8649

Merged

jonpryor reviewed Feb 13, 2024

View reviewed changes

src/Java.Interop.Tools.Maven/Repositories/CachedMavenRepository.cs Show resolved Hide resolved

jonpryor reviewed Feb 13, 2024

View reviewed changes

jpobst force-pushed the java-interop-maven branch from dbf91ae to f938d56 Compare February 15, 2024 20:37

jpobst requested a review from jonpryor February 21, 2024 18:30

jonpryor reviewed Feb 21, 2024

View reviewed changes

jpobst added 5 commits February 22, 2024 09:29

[Java.Interop.Tools.Maven] Initial commit.

2b1dd0a

Remove obsolete file.

624f8a5

Address review feedback.

726b72c

Move MavenVersion and MavenVersionRange from XABT.

dd7d720

Address review feedback.

77087b7

jpobst force-pushed the java-interop-maven branch from 6896260 to 77087b7 Compare February 22, 2024 19:29

Merge branch 'main' into java-interop-maven

c7ea62c

jpobst and others added 2 commits February 26, 2024 10:36

Add target for generating Project.cs.

107a9d4

Whitespace updates.

7a2b105

jonpryor merged commit 1c9c8c9 into main Mar 11, 2024
1 of 4 checks passed

jonpryor deleted the java-interop-maven branch March 11, 2024 17:36

github-actions bot locked and limited conversation to collaborators Apr 12, 2024


		public static MavenRepository Google = new MavenRepository ("https://dl.google.com/android/maven2/", "google");

		public static MavenRepository Central = new MavenRepository ("https://repo1.maven.org/maven2/", "central");


		namespace Java.Interop.Tools.Maven;

		public interface IPomResolver


		MavenVersion (string rawVersion) => RawVersion = rawVersion;

		public static MavenVersion Parse (string version)


		var serializer = new XmlSerializer (typeof (Project));

		using (var sr = new StreamReader (stream))

[Java.Interop.Tools.Maven] Initial commit. #1179

[Java.Interop.Tools.Maven] Initial commit. #1179

Conversation

jpobst commented Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonpryor commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpobst commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonpryor commented Feb 21, 2024

jonpryor commented Feb 22, 2024

jonpryor commented Feb 22, 2024

jpobst commented Feb 22, 2024

jonpryor commented Feb 24, 2024

jpobst commented Jan 12, 2024 •

edited

Loading