Skip to content

.NET Standard support - #5

Open
timotei wants to merge 20 commits into
v0.46from
v0.46-netstandard
Open

.NET Standard support#5
timotei wants to merge 20 commits into
v0.46from
v0.46-netstandard

Conversation

@timotei

@timotei timotei commented Oct 10, 2019

Copy link
Copy Markdown
Collaborator

Requires first and foremost adding the .NET Standard framework in the NAnt config (netstandard.txt in the root)

@halex2005 halex2005 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for great work!

I've taken a look and reviewed some parts of PR.
I hope someone will review this PR too.

@@ -1,159 +1,78 @@
<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" ToolsVersion="3.5">
<Project Sdk="Microsoft.NET.Sdk">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Now when you use new csproj format, I suggest to use PackageReference instead of direct storing nuget packages folder in git.

Comment thread ikvmc.8.csproj
<Name>ICSharpCode.SharpZipLib</Name>
<HintPath>bin\ICSharpCode.SharpZipLib.dll</HintPath>
</Reference>
<Reference Include="IKVM.Runtime, Version=0.46.0.5, Culture=neutral, PublicKeyToken=null">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think that project reference will be better than assembly reference with explicit version specified

Comment thread ikvmc/Compiler.cs
Console.Error.WriteLine(" -nostdlib Do not reference standard libraries");
Console.Error.WriteLine(" -lib:<dir> Additional directories to search for references");
Console.Error.WriteLine(" -noautoserialization Disable automatic .NET serialization support");
Console.Error.WriteLine(" -netstandard Generate .NET Standard libraries");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

May be useful to mark this line with (default) . Also, here you use two tabs instead of spaces, it may looks ugly in console.

Comment thread ikvmc/Compiler.cs
if (peer.assembly.Equals(reference, StringComparison.InvariantCultureIgnoreCase))
{
ArrayAppend(ref target.peerReferences, peer.assembly);
goto next_reference;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may remove ugly goto and get code more readable if you rewrite code with some LINQ.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If I understood your code right, consider something like:

var peerAssemblies = targets
	.Where(peer => target.unresolvedNetStandardReferences.Contains(peer.assembly,  StringComparison.InvariantCultureIgnoreCase))
	.ToArray();
foreach (var reference in peerAssemblies)
{
	ArrayAppend(ref target.peerReferences, peer.assembly);
}

foreach (var reference in target.unresolvedNetStandardReferences.Where(reference => targets.All(peer => !peer.assembly.Equals(reference, StringComparison.InvariantCultureIgnoreCase))))
{
	int rc = resolver.ResolveReference(cache, ref target.references, reference, true);
	if (rc != 0)
	{
		return rc;
	}
}

Comment thread ikvmc/Compiler.cs
int rc = resolver.ResolveReference(cache, ref target.references, reference, true);
if (rc != 0)
{
return rc;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't like this return statement. Not all peer assemblies could be added to target.peerReferences sometime.

Comment thread openjdk/openjdk.build
<property name="pathsep" value=";" />
</if>

<target name="all.netstandard">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually, I believe we can stop using nant and use something new build script automation tools like cakebuild or nuke. IMO, this will be more readable and supportable.

# jeroen@frijters.net
#

assembly.class

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this file supposed to be included in git history?

Comment thread packages.config
@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="Microsoft.Win32.Registry" version="4.5.0" targetFramework="net461" />

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I suggest to remove this file and packages folder due to new csproj format is used. We could use PackageReference in csproj instead.

Comment thread runtime/CodeEmitter.cs
internal void DefineSymbolDocument(ModuleBuilder module, string url, Guid language, Guid languageVendor, Guid documentType)
{
#if NETSTANDARD
throw new PlatformNotSupportedException();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So many potential runtime errors. Could we workaround this sometime?

Comment thread runtime/CodeEmitter.cs
#if NETSTANDARD
throw new PlatformNotSupportedException();
#else
symbols = module.DefineDocument(url, language, languageVendor, documentType);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

May be targeting .net standard 2.1 brings module builder API https://docs.microsoft.com/ru-ru/dotnet/api/system.reflection.emit.modulebuilder

@zgramana

Copy link
Copy Markdown

Curious why this excellent work was based on such an old branch as opposed to, say, 8.1?

@timotei

timotei commented Jan 19, 2020

Copy link
Copy Markdown
Collaborator Author

@zgramana Unfortunately we're using this version because trying to use it with Java 8 brings breaking changes (e.g., default implemented method) which we cannot afford right now. However, I might be able to give it a try to apply the changes on a newer branch, but can't promise anything as it's not primary focus.

@zgramana

Copy link
Copy Markdown

Interesting. I’ve used this PR as a guide to add support to master. I haven’t completed all of it yet, as I’m torn between replacing csc with exec + dotnet and just using the same approach you took.

I don’t think that I fully understood your comment about Java 8 breaking changes and “default implemented method”. Could you explain that in a little detail?

@VincentVk9373

Copy link
Copy Markdown

We have many dlls compiled from java6-7 on different projects and need them to be compatible together at runtime (dynamic loading). Switching only one of them to java8 means we need to have two different ikvm version at runtime, but also that we need a version of every dlls for both versions, but also that objects cannot be shared between both runtimes.

This was caused indirectly by the way default interfaces methods are handled, which makes interfaces runtime differ, so a class implementing an interface cannot be shared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants