Support .NET Core #17

sbomer · 2017-02-23T19:59:55Z

We would like to run the linker on .NET Core. Some changes have already been made to Cecil to support a netstandard build. This change adds a NetCore.props file, which supports building against netcoreapp1.1, using the netcore_Debug and netcore_Release configurations, which will depend on the netstandard_Debug and netstandard_Release configurations of Cecil, respectively.

The CI build scripts have been updated to use the netcore configuration, but the project file still supports building the old Debug and Release configurations on desktop (in addition to the netcore configurations).

This change includes a DirectoryAssemblyResolver, which is a simple resolver following the example of BaseAssemblyResolver in cecil.

For now, this change will not fall back on the default parameterless ReadSymbols() if there is no SymbolReaderProvider. Once cecil supports ReadSymbols() on .NET Core (335), we can remove the #if !NET_CORE.

@erozenfeld, would you mind taking a look?

This change adds a Mono.Linker.new.csproj, using the new cli's msbuild-based .csproj format. The build scripts have been updated to download an appropriate version of the cli, with scripts for both windows and unix. A few API calls have been changed to use APIs available in netcoreapp1.1.

With a few modifications to Mono.Linker.csproj, this allows us to use a single project file to support the old build configuration as well as the build that targets netcoreapp1.1. To build against .NET core, use the netcore_Debug and netcore_Release configurations. The build scripts have been updated to use these by default.

dnfclas · 2017-02-23T19:59:59Z

@sbomer,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

sbomer · 2017-02-23T21:11:40Z

test ci please

sbomer · 2017-02-23T21:16:57Z

Just confirming that the ci job passes with my changes to netci.groovy: Job output. (I'm not sure how long this temporary job will exist, so the link might break.)

erozenfeld · 2017-02-24T05:33:11Z

linker/Mono.Linker.Steps/BlacklistStep.cs

@@ -46,7 +46,7 @@ protected override bool ConditionToProcess()

 		protected override void Process ()
 		{
-			foreach (string name in Assembly.GetExecutingAssembly ().GetManifestResourceNames ()) {
+			foreach (string name in typeof (BlacklistStep).GetTypeInfo ().Assembly.GetManifestResourceNames ()) {


Why do you need this change (and similar changes elsewhere)? It looks like Assembly.GetExecutingAssembly is available in coreclr: https://github.com/dotnet/coreclr/blob/68f72dd2587c3365a9fe74d1991f93612c3bc62a/src/mscorlib/src/System/Reflection/Assembly.cs#L327

See 1784. It looks like some of the removed APIs are not available to build against, despite existing in the coreclr sources.

It's quite strange that Assembly.Load(byte[]) exists in CoreCLR project: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Reflection/Assembly.cs#L392
Can someone explain me why it's there and we can't use it?

@exyi It actually gets modified to be internal in the build process. It is complicated by various factors around how this code gets shared in many different configurations.

Can you please check with @terrajobst regarding all of the appi's you had to change? I believe they should all be available in 2.0.

erozenfeld · 2017-02-24T05:41:20Z

linker/Mono.Linker/LinkContext.cs

@@ -147,7 +147,7 @@ public AssemblyDefinition Resolve (string name)
 				return assembly;
 			}

-			return Resolve (new AssemblyNameReference (name, new Version ()));
+			return Resolve (new AssemblyNameReference (name, new Version (0, 0)));


Why is this change (and a similar one in LoadI18nAssemblies.cs) needed? Version constructor with no parameters is available in coreclr: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Version.cs#L99

Another API that was removed. dotnet/coreclr#6810 gives a clue as to how the APIs available to build against differ from the coreclr sources. I don't fully understand when and how these are used, but it looks like everything is compiled against the reference assemblies such as (mscorlib.cs), which was missing the parameterless constructor. I think these are taken from nuget packages during the build.

erozenfeld · 2017-02-24T05:44:44Z

linker/Mono.Linker/LinkContext.cs

@@ -189,8 +189,12 @@ public void SafeReadSymbols (AssemblyDefinition assembly)

 					_annotations.AddSymbolReader (assembly, symbolReader);
 					assembly.MainModule.ReadSymbols (symbolReader);
-				} else
+				}
+#if !NET_CORE


Do we still need this after this commit: jbevain/cecil@261e199 ?

We won't once it's in mono/cecil.

Can you create a PR for merging it to mono/cecil?

Merged: bd98f212

erozenfeld · 2017-02-24T05:47:39Z

linker/Mono.Linker/Annotations.cs

@@ -316,9 +316,10 @@ public void SaveDependencies ()
 			writer.WriteEndElement ();
 			writer.WriteEndDocument ();
 			writer.Flush ();
+#if !NET_CORE


Can you explain why this is needed and what's the plan to eliminate the #if ?

XmlWriter and GZipStream don't have Close methods on core: 4797. Looking at this again, I think it would be safe to get rid of the calls to Close() altogether, as Dispose() should take care of it for us on desktop: GZipStream, XmlWriter. What do you think?

erozenfeld · 2017-02-24T05:57:59Z

linker/Mono.Linker/DirectoryAssemblyResolver.cs

+using System.IO;
+using Mono.Collections.Generic;
+using Mono.Cecil;
+


It's unfortunate that we copied so much code from Cecil's BaseAssemblyResolver.cs. I hope it's temporary and we find a way to share the code.

This file doesn't follow the coding conventions used in the linker:
method calls should have a space before ();
{ goes on the same line as the construct;
there should be a space before [ in array expressions.

Please keep the same coding convention as in BaseAssemblyResolver.cs.

I agree about copying code. For now this gives us the flexibility to modify our assembly resolution for the scenarios we care about, as discussed with @jbevain. If cecil ends up supporting the resolution methods we need (see 306) it should be easy enough to remove this.

I mostly took this directly from BaseAssemblyResolver.cs, but I must have changed a few of the conventions. I'll fix those - thanks!

I adjusted the formatting in a new commit.

As an fyi, I made some changes related to the AssemblyResolver in our fork. We have a custom assembly resolver that we need to use. Sounds like you have a similar problem.

Here is the PR to our fork that will allow us to rebase on top of the latest linker code. (I will be upstreaming most of this soon)
Unity-Technologies#1

If you want to see what we did, Search for ILinkerAssemblyResolver. That's the abstract I came up with to let us hook in our own resolver.

erozenfeld · 2017-02-24T05:59:16Z

linker/Mono.Linker/DirectoryAssemblyResolver.cs

+
+		AssemblyDefinition SearchDirectory(AssemblyNameReference name, IEnumerable<string> directories, ReaderParameters parameters)
+		{
+			var extensions = new [] { ".dll" };


This needs to include ".exe". Have you tested this resolver on an app? Did it not have an exe?

On .NET Core, the main assembly is a .dll, and the main executable (.exe, or no extension on linux) is just a renamed host. I tested this on musicstore.

That's not always true. You can certainly have a .exe main assembly and run it via corerun.

erozenfeld · 2017-02-24T06:09:07Z

linker/Mono.Linker/Driver.cs

+					string line;
+					while ((line = reader.ReadLine ()) != null)
+						lines.Add (line);
+				}
 			}


StreamReader(string) seems to be available in coreclr: https://github.com/dotnet/coreclr/blob/c35e8dbc37e5380f46553510d0368aad04a677d2/src/mscorlib/src/System/IO/StreamReader.cs#L160

This is another one where it's not available in the apis despite existing in the source. I believe this is being reintroduced in 2.0 (see http://stackoverflow.com/questions/40697200/no-streamreader-constructor-accepting-a-string), but I tried building against netcoreapp2.0 and that didn't fix the problems I was having with these APIs not existing yet - seems to be a work in progress.

marek-safar · 2017-02-24T14:27:04Z

What are all these infrastructure files for and can we move them to separate folder?

sbomer · 2017-02-24T17:05:04Z

Some of the infrastructure files are for ci, and some are scripts that bootstrap a specific cli version used to build the linker with the build scripts (also used in ci). It's still possible to build using the plain project files if desired.

More specifically:

restore.{cmd/sh}, build.{cmd/sh}: wrapper scripts to restore and build the netcore version of the linker. These pass arguments through run.{cmd/sh}
run.{cmd/ps1/sh}: bootstraps the cli and build tools using bootstrap.{ps1/sh}, and uses the cli to invoke the run.exe tool with the passed-in arguments. Run.exe is provided by a package in .toolversions.
config.json: Run.exe reads this to determine how to translate its arguments to arguments to underlying tools.
.cliversion: version of the cli we use to build
.toolversions: extra tools used in the build, including run.exe
bootstrap.{ps1/sh}: This downloads and runs the dotnet-install.ps1 script, which is used to obtain the version set in .cliversion. Extra tools in .toolversions are then restored. This replaces init-tools.cmd.
netci.groovy: script read by the jenkins jobs to determine what to run in ci

None of these are necessary if all you want to do is restore and build the linker and you already have an installed cli.

It's possible to move these to a separate infrastructure folder, but I'm not sure if it's worth the effort, as the examples I followed all use a similar layout with these files in the root of the repo (see coreclr, corefx, cli, buildtools), and I'd prefer not to diverge too much from them.

- Adjust the conventions in DirectoryAssemblyResolver.cs to match BaseAssemblyResolver.cs from mono/cecil - Remove calls to Clos() in Annotations.cs. Dispose() should take care of this.

This undoes an earlier modification that was made to support netcoreapp1.1, and instead targets netcoreapp2.0, which supports the APIs modified earlier. This also adds a new NuGet.config, which provides the source from which to restore the correct version of Microsoft.NETCore.App.

sbomer · 2017-02-28T00:04:34Z

@erozenfeld, I figured out how to properly target netcoreapp2.0 with some pointers from @terrajobst (thanks!). This lets me revert most of the code changes I made. Could you take another look?

erozenfeld · 2017-02-28T00:16:25Z

Sven, thank you for following up on netcoreapp2.0 targeting and reverting code changes.

Regarding location of the scripts: I agree with @marek-safar that they should go into a separate folder. The linker can run in a variety of environments (desktop, mono, .net core, maybe others) and is used by several teams so having files named run.{cmd/ps1/sh}, restore.{cmd/sh}, bootstrap.{ps1/sh}, etc. at the root is confusing when they are .NET Core - specific.

sbomer · 2017-02-28T00:45:46Z

@joncham, could you take a look? I'd like to ensure that my change will keep it easy to share this repo across the different scenarios, which you seem to know about. Note that I'll be moving the build infrastructure scripts to a separate directory as per @marek-safar and @erozenfeld 's suggestions.

mrvoorhe · 2017-02-28T03:32:20Z

@sbomer This PR looks ok. There's going to be a minor conflict with the new AseemblyResolver and an ILinkerAssemblyResolver interface I'll be attempting to upstream soon. But that's not a big deal.

When it comes to the .NET Core build stuff, I don't see us jumping on that in the short term, so no concerns from us regarding the build scripts at this point.

Currently the build paths we use are JAM, xbuild, and various IDE's (VS, Project Rider, VS for Mac). As long as the .csproj's continue to work we'll be ok.

sbomer · 2017-02-28T15:49:41Z

test ci please

sbomer · 2017-02-28T15:53:22Z

test ci please

sbomer · 2017-02-28T16:08:52Z

test ci please

sbomer · 2017-02-28T16:12:40Z

test ci please

sbomer · 2017-02-28T16:23:17Z

test ci please

Also add .exe to resolver

sbomer · 2017-02-28T16:25:55Z

test ci please

erozenfeld · 2017-02-28T21:26:14Z

linker/Mono.Linker/DirectoryAssemblyResolver.cs

+			this.reference = reference;
+		}
+	}
+


This may conflict with my PR that was recently merged: #16. In that PR I'm catching AssemblyResolutionException, which is now ambiguous. Can't we make Mono.Cecil.AssemblyResolutionException available under NET_CORE and use it here?

Sure, we can do that. I opened a PR on cecil: jbevain/cecil#340. I'll update the submodule when it gets merged.

erozenfeld · 2017-02-28T21:26:34Z

linker/Mono.Linker/DirectoryAssemblyResolver.cs

+			directories.Remove (directory);
+		}
+
+		public string[] GetSearchDirectories ()


string[] --> string []

erozenfeld · 2017-02-28T21:28:36Z

linker/Mono.Linker/DirectoryAssemblyResolver.cs

+			if (assembly != null)
+				return assembly;
+
+			var framework_dir = Path.GetDirectoryName(typeof (object).GetTypeInfo ().Module.FullyQualifiedName);


Add a space before (typeof.
Also, do you need GetTypeInfo() ?

We'll use cecil's AssemblyResolutionException

sbomer · 2017-03-01T15:55:08Z

@erozenfeld, I've removed AssemblyResolutionException, and updated cecil now that my change is in mono/cecil. @marek-safar, could you merge this if everything looks ok to you?

marek-safar · 2017-03-01T22:13:29Z

Merged but someone needs to update netcore CI

sbomer · 2017-03-02T00:41:17Z

Thanks to everyone who helped with this!

@marek-safar, I believe the ci job for the merge failed because it was using the old job definitions. This change already updates the ci jobs for .NET core. I manually kicked off the new jobs that were generated by this merge, and they passed on both ubuntu and windows:
https://ci.dot.net/job/mono_linker/job/master/job/ubuntu/1/
https://ci.dot.net/job/mono_linker/job/master/job/windows_nt/8/

* Add support for .NET core This change adds a Mono.Linker.new.csproj, using the new cli's msbuild-based .csproj format. The build scripts have been updated to download an appropriate version of the cli, with scripts for both windows and unix. A few API calls have been changed to use APIs available in netcoreapp1.1. * Replace Mono.Linker.new.csproj with NetCore.props With a few modifications to Mono.Linker.csproj, this allows us to use a single project file to support the old build configuration as well as the build that targets netcoreapp1.1. To build against .NET core, use the netcore_Debug and netcore_Release configurations. The build scripts have been updated to use these by default. * Allow setting runtimeidentifier, and clarify behavior of scripts * Remove old comments * Add Ubuntu build jobs * Fix formatting, and remove an #if !NET_CORE - Adjust the conventions in DirectoryAssemblyResolver.cs to match BaseAssemblyResolver.cs from mono/cecil - Remove calls to Clos() in Annotations.cs. Dispose() should take care of this. * Set echo off in cmd scripts * Update Cecil and remove #if !NET_CORE around ReadSymbols() * Target netcoreapp2.0 and reintroduce new API calls This undoes an earlier modification that was made to support netcoreapp1.1, and instead targets netcoreapp2.0, which supports the APIs modified earlier. This also adds a new NuGet.config, which provides the source from which to restore the correct version of Microsoft.NETCore.App. * Move infrastructure files to corebuild folder Also add .exe to resolver * Fix formatting, and remove AssemblyResolutionException We'll use cecil's AssemblyResolutionException * Update cecil submodule Commit migrated from dotnet/linker@9401a4b

sbomer added 4 commits February 22, 2017 13:12

Allow setting runtimeidentifier, and clarify behavior of scripts

f093118

Remove old comments

432ab81

dnfclas added the cla-already-signed label Feb 23, 2017

Add Ubuntu build jobs

e0e90c3

erozenfeld requested changes Feb 24, 2017

View reviewed changes

sbomer added 2 commits February 24, 2017 10:38

Fix formatting, and remove an #if !NET_CORE

63a4e2b

- Adjust the conventions in DirectoryAssemblyResolver.cs to match BaseAssemblyResolver.cs from mono/cecil - Remove calls to Clos() in Annotations.cs. Dispose() should take care of this.

Set echo off in cmd scripts

5e755fd

sbomer force-pushed the netcore branch 3 times, most recently from 0339bce to 85a0ef7 Compare February 27, 2017 16:14

Update Cecil and remove #if !NET_CORE around ReadSymbols()

c39bd6f

sbomer force-pushed the netcore branch from 85a0ef7 to c39bd6f Compare February 27, 2017 16:16

sbomer force-pushed the netcore branch from 548e377 to bb396d5 Compare February 28, 2017 15:52

sbomer force-pushed the netcore branch from bb396d5 to 7380436 Compare February 28, 2017 16:08

sbomer force-pushed the netcore branch from 7380436 to ea09888 Compare February 28, 2017 16:12

sbomer force-pushed the netcore branch from ea09888 to afcd147 Compare February 28, 2017 16:22

Move infrastructure files to corebuild folder

9fc2bc3

Also add .exe to resolver

sbomer force-pushed the netcore branch from afcd147 to 9fc2bc3 Compare February 28, 2017 16:25

erozenfeld reviewed Feb 28, 2017

View reviewed changes

sbomer added 2 commits February 28, 2017 14:25

Fix formatting, and remove AssemblyResolutionException

df256e2

We'll use cecil's AssemblyResolutionException

Update cecil submodule

111c218

erozenfeld approved these changes Mar 1, 2017

View reviewed changes

marek-safar merged commit 9401a4b into dotnet:master Mar 1, 2017

Support .NET Core #17

Support .NET Core #17

Conversation

sbomer commented Feb 23, 2017 • edited Loading

dnfclas commented Feb 23, 2017

sbomer commented Feb 23, 2017

sbomer commented Feb 23, 2017

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

marek-safar commented Feb 24, 2017

sbomer commented Feb 24, 2017

sbomer commented Feb 28, 2017

erozenfeld commented Feb 28, 2017 • edited Loading

sbomer commented Feb 28, 2017

mrvoorhe commented Feb 28, 2017

sbomer commented Feb 28, 2017 • edited Loading

sbomer commented Feb 28, 2017 • edited Loading

sbomer commented Feb 28, 2017

sbomer commented Feb 28, 2017

sbomer commented Feb 28, 2017

sbomer commented Feb 28, 2017

erozenfeld Feb 28, 2017 • edited Loading

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

sbomer commented Mar 1, 2017

marek-safar commented Mar 1, 2017

sbomer commented Mar 2, 2017

sbomer commented Feb 23, 2017 •

edited

Loading

erozenfeld commented Feb 28, 2017 •

edited

Loading

sbomer commented Feb 28, 2017 •

edited

Loading

sbomer commented Feb 28, 2017 •

edited

Loading

erozenfeld Feb 28, 2017 •

edited

Loading