Added support for csproj-based scripts #980

seesharper · 2017-10-12T13:06:32Z

This PR adds support for csproj-based scripts and marks the end of supporting project.json as a way to resolve compilation dependencies for C# scripts.

The dependency to the Microsoft.DotNet.ProjectModel package is gone and we now resolve compilation dependencies using the Dotnet.Script.DependencyModel package being maintained in the Dotnet.Script repo. See the release notes for Dotnet.Script 0.14.0 for more details

For scripts that contain inline NuGet references, we still have this as an opt-in feature as before.

{
  "script": {
    "enableScriptNuGetReferences": true
  }
}

Nothing has changed for scripts that targets the full framework. This PR only affects scripts that targets .Net Core.

With this update to the project system we can now resolve compilation dependencies for all packages that targets netstandard2.0.

The following section provides more details about the packages now being referenced from OmniSharp.Script

Dotnet.Script.DependencyModel

Provides an API to resolve runtime and compilation dependencies for csproj-based scripts.

This library has no dependencies to other dotnet.script libraries or other third-party packages, which means that it can be used by all script runners that needs to resolve runtime dependencies.

In addtion to being compiled for netstandard2.0, it is also compiled for net46 so that it can be used from OmniSharp.Scripting

Dotnet.Script.DependencyModel.NuGet

This package contains ONLY the NuGetMetadataReferenceResolver that is used to handle inline NuGet package references.

#r "nuget: AutoMapper, 9.1.0"

This MetadataReferenceResolver does not really do anything besides allowing the actual NuGet reference and returning an assembly reference.

Prerequisites

The Dotnet 2.0 sdk will have to be installed since we use dotnet restore under the hood to provide a DependencyContext from which we resolve runtime and compilation dependencies. On the execution side ,this is not a problem since the user would have to have the dotnet cli installed anyway to execute dotnet.script. On the OmniSharp side, relying on the dotnet cli might sound a little more questionable. But since dotnet.script is the only script runner that actually understands either an explicit csproj file or a csproj file generated from script NuGet references, we should be good also on the OmniSharp side. In the event of the dotnet cli being unavailable, we will restore script packages using NuGet version 4.3.0 that comes embedded with Dotnet.Script.DependencyModel.

Logging

Instead of relying on a logging framework, we simply provide a delegate that can be used from the consuming library to forward log messages to the actual logging framework.
This is to done to reduce the number of third party dependencies needed by Dotnet.Script.DependencyModel

Example:

LogFactory logFactory = type =>
{
    // Create actual logger here
    return ((level, message) =>
    {
        // Forward log message to actual logger here.
    });

Note: If we pass DebugLevel.Debug, we are being verbose and it should probably be logged as a debug statement. Otherwise it should appear in the standard log output.

Resolving runtime dependencies (Execution)

LogFactory logFactory = type =>
{
    // Create actual logger here
    return ((level, message) =>
    {
        // Forward log message to actual logger here.
    });   

var resolver  = new RuntimeDependencyResolver(logFactory);
var runtimeDependencies = resolver.GetDependencies(targetDirectory);

Resolving packages with native/unmanaged dependencies

Some NuGet packages ship with native/unmanaged libraries. An example would be the Microsoft.Data.SQLite package that relies on the e_sqlite3 native library. This library needs to be accessible and discovered by the scripting runtime and we do that on Windows using LoadLibrary.

It is sort of a hack, but it seems to make the scripting runtime aware of the DLL. Preferably we would somehow set the NATIVE_DLL_SEARCH_DIRECTORIES property of the AppDomain, but we can't do that once the CoreHost is up and running. We simply come in to late.

The LoadLibrary method can only be used on Windows, but the *nix counterpart, dlopen does not do the trick here.

Resolving compilation dependencies (OmniSharp)

Compilation dependencies (or compile-time dependencies) are meant to be used by OmniSharp.Script to provide intellisense for script files.

LogFactory logFactory = type =>
{
    // Create actual logger here
    return ((level, message) =>
    {
        // Forward log message to actual logger here.
    }); 

bool enableScriptNuGetReferences = true;
var resolver = new CompilationDependencyResolver(logFactory)
var compilationDependencies = resolver.GetDepedencies(targetDirectory, enableScriptNugetReferences);

The reason that we deal with runtime dependencies on the execution side, is that the Roslyn scripting engine currently only supports compiling against runtime assemblies.

Take a look here for more information.

Say goodbye to project.json

For a long time during the beta days of .Net Core, it looked like Microsoft was heading for a new project format for .Net projects. project.json was introduced as the new lightweight project format and a lot of time and effort was put in to convert projects from the old csproj format to this new shiny project.json format. After a while or more specifically towards the end of the beta period, Microsoft decided to ditch project.json and go back to csproj. Not the old and rather verbose csproj, but a new simpler csproj format.

project.json practically died over night.

OmniSharp.Script still uses project.json as a way to provide a "runtime context" from which the compilation dependencies can be retrieved. This project.json file can contain references to NuGet packages and hence OmniSharp can provide intellisense for them as well.

The problem with project.json today is that we barely have any tooling left that can understand the format. NuGet stopped supporting project.json after version 3.5 and the dotnet sdk never understood the format after going out of beta. So for instance, if you want to use project.json today , you will need an outdated preview version of the dotnet sdk that most developers do not install.

seesharper · 2017-10-12T13:38:34Z

Looks like logging broke. Looking into it

seesharper · 2017-10-12T14:01:01Z

Looks like the old Dotnet.Script.NuGetMetadataReferenceResolver pulled in Microsoft.Extensions.Logging 1.1.1. Without it logging starts to fail all over

filipw · 2017-10-12T14:03:08Z

This PR adds support for csproj-based scripts and marks the end of supporting project.json as a way to resolve compilation dependencies for C# scripts.

🎉 ✨ 👑

seesharper · 2017-10-12T14:09:39Z

Got it. Binding redirect for Microsoft.Extensions.Logging needs to be 1.1.0.0 now that we don't pull in 1.1.1.0 anymore. Fixing

filipw · 2017-10-12T14:10:59Z

src/OmniSharp.Script/ScriptProjectSystem.cs

+            _compilationDependencyResolver = new CompilationDependencyResolver(type =>
+            {
+                // Prefix with "OmniSharp" so that we make it through the log filter.
+                var categoryName = $"OmniSharp.{type.FullName}";


I think this maybe needs one more level nesting for better context i.e. OmniSharp.Script

filipw

david-driscoll

LGTM, outside the one minor question about logging.

david-driscoll · 2017-10-12T15:44:05Z

src/OmniSharp.Script/ScriptProjectSystem.cs

+                    {
+                        dependencyResolverLogger.LogDebug(message);
+                    }
+                    if (level == LogLevel.Info)


Are there any other related log levels (that matter?)

No, that’s it. No other levels 😊

Does this mean it will only log informational messages if the LogLevel == Info? I would expect it to send informational messages if LogLevel <= Info. Same for debug.

There are only two possible levels logged from Dotnet.Script.DependencyModel

Debug = 0

Info = 1

I would expect it to send informational messages if LogLevel <= Info.

Debug is the lowest level and it would sound a little strange to me to log that as a informational message?

The LogLevel that we pass into the delegate is not from Microsoft.Extensions.Logging. It is defined in Dotnet.Script.DependencyModel with only those two levels.

Dotnet.Script.DependencyModel does not depend on Microsoft.Extensions.Logging. The code here simply forwards log messages to the actual logging framework which for OmniSharp.Roslyn means Microsoft.Extensions.Logging

Did that make any sense ?

I see, I thought these were coming from Microsoft.Extensions.Logging.

Figured you might think so:) Didn’t want to introduce a shared library version problem again. Therefore we don’t reference Microsoft.Extensions.Logging.

seesharper · 2017-10-13T12:18:43Z

Are we good to rock'n roll on this one?

filipw · 2017-10-13T12:26:56Z

Yeah let's punch this in - we are now in a really nice place on the scripting side. Thanks!

DustinCampbell · 2017-10-13T13:27:27Z

tests/app.config

@@ -37,11 +37,11 @@

            <dependentAssembly>
                <assemblyIdentity name="Microsoft.Extensions.Logging" publicKeyToken="adb9793829ddae60" culture="neutral"/>
-                <bindingRedirect oldVersion="0.0.0.0-1.1.1.0" newVersion="1.1.1.0"/>
+                <bindingRedirect oldVersion="0.0.0.0-1.1.1.0" newVersion="1.1.0.0"/>


I think these got added because Dotnet.Script.NuGetMetadataResolver depended on 1.1.1. So, it might be that these could just be removed now.

DustinCampbell · 2017-10-13T13:27:32Z

tests/app.config

            </dependentAssembly>
            <dependentAssembly>
                <assemblyIdentity name="Microsoft.Extensions.Logging.Abstractions" publicKeyToken="adb9793829ddae60" culture="neutral"/>
-                <bindingRedirect oldVersion="0.0.0.0-1.1.1.0" newVersion="1.1.1.0"/>
+                <bindingRedirect oldVersion="0.0.0.0-1.1.1.0" newVersion="1.1.0.0"/>


Want me to PR removal of these redirects?

seesharper added 2 commits October 12, 2017 12:33

Added support for csproj-based scripts

94b9dd6

Merge remote-tracking branch 'upstream/master'

c1cfb98

filipw reviewed Oct 12, 2017

View reviewed changes

filipw approved these changes Oct 12, 2017

View reviewed changes

seesharper added 2 commits October 12, 2017 16:12

Redirect to Microsoft.Extensions.Logging 1.1.0

23d5447

LogContext (OmniSharp.Script)

6280c3a

david-driscoll approved these changes Oct 12, 2017

View reviewed changes

filipw merged commit d393004 into OmniSharp:master Oct 13, 2017

DustinCampbell reviewed Oct 13, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for csproj-based scripts #980

Added support for csproj-based scripts #980

seesharper commented Oct 12, 2017

seesharper commented Oct 12, 2017

seesharper commented Oct 12, 2017

filipw commented Oct 12, 2017

seesharper commented Oct 12, 2017

filipw Oct 12, 2017

seesharper Oct 12, 2017

filipw left a comment

david-driscoll left a comment

david-driscoll Oct 12, 2017

seesharper Oct 12, 2017

DustinCampbell Oct 13, 2017

seesharper Oct 13, 2017

DustinCampbell Oct 13, 2017

seesharper Oct 13, 2017

seesharper commented Oct 13, 2017

filipw commented Oct 13, 2017

DustinCampbell Oct 13, 2017

DustinCampbell Oct 13, 2017

seesharper Oct 13, 2017

Added support for csproj-based scripts #980

Added support for csproj-based scripts #980

Conversation

seesharper commented Oct 12, 2017

Dotnet.Script.DependencyModel

Dotnet.Script.DependencyModel.NuGet

Prerequisites

Logging

Resolving runtime dependencies (Execution)

Resolving packages with native/unmanaged dependencies

Resolving compilation dependencies (OmniSharp)

Say goodbye to project.json

seesharper commented Oct 12, 2017

seesharper commented Oct 12, 2017

filipw commented Oct 12, 2017

seesharper commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipw left a comment

Choose a reason for hiding this comment

david-driscoll left a 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

seesharper commented Oct 13, 2017

filipw commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment