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

Added support for csproj-based scripts #980

Merged
merged 4 commits into from
Oct 13, 2017
Merged

Added support for csproj-based scripts #980

merged 4 commits into from
Oct 13, 2017

Conversation

seesharper
Copy link
Contributor

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
Copy link
Contributor Author

Looks like logging broke. Looking into it

@seesharper
Copy link
Contributor Author

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

@filipw
Copy link
Member

filipw commented Oct 12, 2017

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
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

LGTM, outside the one minor question about logging.

{
dependencyResolverLogger.LogDebug(message);
}
if (level == LogLevel.Info)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that’s it. No other levels 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

@filipw
Copy link
Member

filipw commented Oct 13, 2017

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

@filipw filipw merged commit d393004 into OmniSharp:master Oct 13, 2017
@@ -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"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

</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"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want me to PR removal of these redirects?

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