-
Notifications
You must be signed in to change notification settings - Fork 420
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 referencing NuGet packages in C# scripts #813
Conversation
@@ -79,16 +89,29 @@ public class ScriptProjectSystem : IProjectSystem | |||
private readonly Dictionary<string, ProjectInfo> _projects; | |||
private readonly OmniSharpWorkspace _workspace; | |||
private readonly IOmniSharpEnvironment _env; | |||
private readonly ILoggerFactory loggerFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an underscore to match the other fields in this file.
var resolver = NuGetMetadataReferenceResolver.Create(ScriptMetadataResolver.Default, | ||
NugetFrameworkProvider.GetFrameworkNameFromAssembly(),loggerFactory, _env.Path); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra blank line.
@@ -18,6 +18,9 @@ | |||
|
|||
namespace OmniSharp.Script | |||
{ | |||
using Dotnet.Script.NuGetMetadataResolver; | |||
using Microsoft.DotNet.InternalAbstractions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't nest usings inside of namespaces. Put them with the rest of the namespaces at the top of the file.
private static Dictionary<string, ImmutableArray<PortableExecutableReference>> DirectReferenceCache = new Dictionary<string, ImmutableArray<PortableExecutableReference>>(); | ||
private static Dictionary<string, PortableExecutableReference> MissingReferenceCache = new Dictionary<string, PortableExecutableReference>(); | ||
private static MetadataReferenceResolver _defaultRuntimeResolver = ScriptMetadataResolver.Default; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra blank line
{ | ||
var resolver = NuGetMetadataReferenceResolver.Create(ScriptMetadataResolver.Default, | ||
NugetFrameworkProvider.GetFrameworkNameFromAssembly(),loggerFactory, _env.Path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing space between ,
and loggerFactory
|
||
public CachingScriptMetadataResolver(MetadataReferenceResolver defaultReferenceResolver) | ||
{ | ||
this.defaultReferenceResolver = defaultReferenceResolver; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use underscore for private field name.
metadataReferenceResolver: | ||
new CachingScriptMetadataResolver(resolver), | ||
sourceReferenceResolver: ScriptSourceResolver.Default, | ||
assemblyIdentityComparer: DesktopAssemblyIdentityComparer.Default). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you like, maybe move the .
to the next line like you've done elsewhere. That always looks better to me.
|
||
foreach (var inheritedAssemblyName in inheritedAssemblyNames) | ||
{ | ||
var assembly = Assembly.Load(inheritedAssemblyName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally use an IAssemblyLoader
so that we can track assemblies that are loaded at runtime. Just import IAssemblyLoader
in the constructor and use it here.
{ | ||
var resolver = NuGetMetadataReferenceResolver.Create(ScriptMetadataResolver.Default, | ||
NugetFrameworkProvider.GetFrameworkNameFromAssembly(),loggerFactory, _env.Path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_env.Path
changed to _env.TargetDirectory
in dev. That's why the build is failing.
Thanks for the PR! My feedback is mostly nits to try and match the code style of the files you're changing. Also, the CI build is failing because |
Thanks for your feedback. I think I got them issues sorted out now :) |
@seesharper thanks a lot for the PR, always great to see people investing their time in CSX tooling 😀 I'm gonna loop in @tmat for his comments on this. Tomas, does this align with the Roslyn vision of supporting nuget references (seems to me it does)? In general we want OmniSharp to stay aligned with CSI.exe scripting. In this case, we can get ahead of the curve a little bit, which I think would be fine, as long as syntax-wise this is what you have thought of for Roslyn down the road? I also like that it's a done as only a custom |
|
||
private static string ResolveTargetFramework() | ||
{ | ||
return Assembly.GetEntryAssembly().GetCustomAttributes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what problem did you run into that you added this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. I was just making sure that I did not change the way metadata references are being added when running on the full desktop framework.
@@ -47,7 +51,7 @@ public override ImmutableArray<PortableExecutableReference> ResolveReference(str | |||
return DirectReferenceCache[key]; | |||
} | |||
|
|||
var result = _defaultRuntimeResolver.ResolveReference(reference, baseFilePath, properties); | |||
var result = _defaultReferenceResolver.ResolveReference(reference, baseFilePath, properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not visible what happens here for nuget packages, can you explain a little? do they get installed? what if there is no network? what if there is no nuget
in the user's path? what's the lock file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any other feedback to @filipw's question above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seesharper: did this ever get answered?
{ | ||
var resolver = NuGetMetadataReferenceResolver.Create(ScriptMetadataResolver.Default, | ||
NugetFrameworkProvider.GetFrameworkNameFromAssembly(), _loggerFactory, _env.TargetDirectory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this where the nuget framework to be used is determined? if so, I don't think it's correct to assume the version of nuget framework from the version of OmniSharp. A desktop OmniSharp (in fact, in VS Code only that version is used) can be used to provide language services for a .NET Core script and vice versa, a .NET Core OmniSharp can be used to provide language services for a destkop framework script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we within OmniSharp determine if the we a running the script in the context of a .Net Core app when there is no project context? The whole idea here is to support "self-contained" scripts that has no additional context other than the script itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so far - because CSI is desktop FX only and there was no formal specification for how .NET Core scripting should behave, OmniSharp would assume desktop FX mode for scripts in case no project.json
was there, which would be the typical majority use case.
if project.json
was found, then the first runtime FX defined there would be used.
in other words, the only way to force .NET Core scripting was through an external file, and I think for now this is how it should stay because it is non-standard scripting anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this is a problem and that is why .Net Core metadata references are only added when OmniSharp itself is running as a .Net Core app. It's not ideal, but at least it would provide a way for people wanting to try out project-less .Net Core scripting by pointing to the .Net Core OmniSharp binary from VsCode. Also commented on an alternative approach here .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C# for VS Code does not ship a .NET Core version of OmniSharp. It runs OmniSharp with an embedded Mono runtime. So, this approach will not work unless the user downloads or builds a .NET Core-based OmniSharp and sets VS Code to use that. And, if they do that, they may have other problems since .NET Core OmniSharp can't handle csproj projects properly.
Note, this will also be a problem long term as the .NET Core versions of OmniSharp will be going away in favor of a Mono-based approach. See #666 for explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sort of understood #666 wouldn't help me much :)
Jokes aside, is there anything we can do to hint OmniSharp about the scripting runtime?
What if we did this
#r "runtime:NetCoreApp1.1"
In the case of a non-existing project.json, we could create one at runtime based on the runtime reference and feed that into the existing logic that resolves metadata references based on project context,
In other words we create a project context based on script runtime references.
The really nice thing about this approach is that we could also add the NuGet references found in the script to the dynamically created project.json and basically skip the NuGetMetadataReferenceResolver all together.
@filipw We could do the exact same approach in Dotnet-Script with NuGet references. Only difference here is that we actually know the scripting runtime context.
Also, let's wait for input from @tmat but I think it would be better to delimit version with a comma. This is the design used in |
Yes, that was the general idea:) I will get back to you tomorrow with more details around how I ended up with this approach. 11pm here now :) |
There is a related discussion in F# design repo: fsharp/fslang-design#167 Currently the syntax F# is considering is:
It would be good if C# and F# used the same syntax. To make this work for all nupkgs (specifically with CoreFX nupkgs) we'll need to be able to deal with both reference assemblies and implementation assemblies in the resolver. Also someone pointed out that batching the resolution (i.e. have an API that allows the compiler to resolve multiple #r's in one go) would be necessary to avoid perf issues with discovering dependencies. |
It would be useful to write up some specification, how the resolver works. For example, does it check |
SyntaxI did not have much to go after with regards to the syntax other than this issue Add NuGet support in scripting APIs #6900 Simply adopted the suggested syntax with the assumption that this might be implemented in a future release of Roslyn. I do however agree that we should align with F# and adopt the same syntax
Resolver - How does it workThe NuGetMetadataReferenceResolver looks for #r references that starts with the word "nuget" and then parses the package name and version from that reference. It then looks for the package in the global nuget cache and from here two things can happen. Either the package is found in the cache and we reference the package from the global cache. If the package is not found in the global cache, we install the package into a temporary folder which gets deleted after installing. This causes the package also to be installed into the global cache and we can again reference the package from the global cache. The process of actually installing the package is offloaded to the NuGet command line application as NuGet does not offer a .Net Core version of NuGet.PackageManagement After the package is resolved from the global cache we will look for the nearest matching framework from that package. This is done using the FrameworkReducer class provided by the NuGet API. The target framework, meaning the context for which to find the nearest matching framework is either inferred from the host process or it can be specified when creating the resolver. It is important to notice that the resolver does not make any attempts to resolve the package dependencies. Since there is no lock file in play here, it made more sense to explicitly (#r)eference all dependencies including transients. It that sense you could say that the script is also the lock file. The resolver uses the PackageSourceProvider (also provided by the NuGet API) to figure out the feeds to be used. This for instance means that we support a local NuGet.Config. |
The challenge , revisited
So how do OmniSharp know if it should provide metadata for the full framework or .Net Core? The PR uses information from the host process (OmniSharp) to determine if we are running .Net Core or the full framework. This is probably not good enough as @filipw pointed out as OmniSharp can provide language services for both runtimes regardless of the runtime that OmniSharp itself runs in. So how can we provide OmniSharp with enough information about script target runtime? What if we did something similar to SheBang on nix systems?
we could do
In my head, it sort of makes sense that we "reference" the runtime. If we find multiple csx files with different "runtimes" we would have to handle this somehow though. |
I think for the sake of baby steps and incremental progress we could consider for now that for no known runtime context (such as CSI scripts) we just assume latest desktop FX, but maybe I'm being overly simplistic. I fear that if we dive into this now we will never get anything out as the scope would just be too large; not to mention such discussion really belongs in the Roslyn repo as that's the codebase that will drive this, and OmniSharp would merely consume it. |
@seesharper Great write up. Could you add it to the readme.md of https://github.com/seesharper/Dotnet.Script.NuGetMetadataResolver? Re the resolver implementation: One thing I'm worried about is specifying all transitive dependencies explicitly. That's not very script-friendly. Note that Re runtime: We already have support for For example, I think there are two categories of scripts:
|
I am not giving up on this yet :) Question is, should I abandon this PR and start a new PR based on the latest sources? |
@tmat @DustinCampbell @filipw Could you take another look at this in the light of the changes I've made to |
@tmat @DustinCampbell @filipw What do you say, guys. Is this happening or not? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the changes in essence, will wait for more feedback from @DustinCampbell or @filipw however.
NuGet.Config
Outdated
</packageSources> | ||
<disabledPackageSources> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These probably shouldn't be committed (looks like custom sources you have locally)
@@ -47,7 +51,7 @@ public override ImmutableArray<PortableExecutableReference> ResolveReference(str | |||
return DirectReferenceCache[key]; | |||
} | |||
|
|||
var result = _defaultRuntimeResolver.ResolveReference(reference, baseFilePath, properties); | |||
var result = _defaultReferenceResolver.ResolveReference(reference, baseFilePath, properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any other feedback to @filipw's question above?
} | ||
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit, there is extra whitespace here, that isn't needed.
FYI: Just tested this on a Mac and it works equally well there. |
@seesharper thanks - looks got to me. There are still some small things @DustinCampbell pointed out but after that, as far as I'm concerned, we could take this in as an experimental scripting feature 👍 @DustinCampbell @david-driscoll what do you guys think? |
actually one more thing - looks like the configuration is not being consumed at the moment. Did you forget a push? |
@filipw @DustinCampbell @david-driscoll The configuration is being consumed here I have also updated to the latest version of Dotnet.Script.NuGetMetadataResolver (2.0.3) and this PR should be able to be merged. |
I meant that |
Ah, I see. The ScriptHelper class is a static class. Is there any way that I can access the configuration from within the ScriptHelper class? |
@filipw Probably need to make the ScriptHelper class an instance class in order to pass in "IConfiguration" ? |
that's OK - it could be a singleton similar to i.e. https://github.com/OmniSharp/omnisharp-roslyn/blob/dev/src/OmniSharp.Roslyn/MetadataHelper.cs#L32 +
|
@filipw Now the NuGetMetadataResolver is only used when the "enableScriptNuGetReferences" option is "true" |
@filipw @DustinCampbell @david-driscoll |
@filipw @DustinCampbell @david-driscoll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm comfortable with this change so long as @filipw is good with it.
Thanks @DustinCampbell . Build passing. @david-driscoll , @filipw . Are we merging this now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The merge button is unavailable until @david-driscoll approves |
@david-driscoll Would really appreciate your final approval on this PR |
Sorry missed the final approvals here... LGTM |
Thanks a lot, @david-driscoll. Appreciate it 👍 |
This is great! Is there an issue anywhere tracking the addition of this to csi.exe? |
This PR adds support for resolving NuGet packages in C# scripts
Example
The feature is implemented by decorating the default MetadataReferenceResolver with a resolver that will know how to handle references to NuGet packages. The code for the NuGetMetadataReferenceResolver is currently maintained at this repo
https://github.com/seesharper/Dotnet.Script.NuGetMetadataResolver
The reason for keeping the NuGetMetadataReferenceResolver outside of this project is so that it can be used by other script runners such as https://github.com/filipw/dotnet-script