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

Feature/csproj #100

Merged
merged 11 commits into from
Aug 24, 2017
Merged

Feature/csproj #100

merged 11 commits into from
Aug 24, 2017

Conversation

seesharper
Copy link
Collaborator

This PR adds support for csproj when running without an explicit context.
We no longer depend on Nuget350.exe and hence we no longer depend on Mono to be installed either.
The only dependency is the dotnet cli and that should put us in a good place.

The scenario where we use a project.json file to specify packages is still supported (LegacyDependencyResolver)

This PR also adds support for loading native libraries on Windows. *nix still needs some work, but I figured we could do that in another PR.

Known issues
I can't get the tests to run on Mac OS. Complains about "too many open files"
Executing the scripts works fine though.

@seesharper seesharper requested a review from filipw August 15, 2017 14:39
@seesharper
Copy link
Collaborator Author

seesharper commented Aug 16, 2017

Actually I got the tests working on Mac OS
Had to set "ulimit -n512"

bri-mac:Dotnet.Script.Tests bernhardrichter$ dotnet test
Build started, please wait...
Build completed.

Test run for /Users/bernhardrichter/Projects/dotnet-script/src/Dotnet.Script.Tests/bin/Debug/netcoreapp1.1/Dotnet.Script.Tests.dll(.NETCoreApp,Version=v1.1)
Microsoft (R) Test Execution Command Line Tool Version 15.0.0.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
[xUnit.net 00:00:00.9812052]   Discovering: Dotnet.Script.Tests
[xUnit.net 00:00:01.1996489]   Discovered:  Dotnet.Script.Tests
[xUnit.net 00:00:01.3130908]   Starting:    Dotnet.Script.Tests
[xUnit.net 00:00:25.1316343]   Finished:    Dotnet.Script.Tests

Total tests: 4. Passed: 4. Failed: 0. Skipped: 0.
Test Run Successful.

@filipw
Copy link
Member

filipw commented Aug 17, 2017

Thanks, looks great. Been a bit busy these days at NDC will have a look as soon as I can.
I'd still like to push out 0.12 before merging this.

Does it need tooling update in OmniSharp?

@seesharper
Copy link
Collaborator Author

Totally understand, No worries, mate :) No updates are needed in OmniSharp. This only affects script execution, but future updates to OmniSharp will probably be based on this as we move away from project.json in OmniSharp as well.
The OmniSharp side of things only deals with the compile time libraries and does not really affect execution at all.

It would be great if we could get 0.12 out as soon as possible so that we can merge this. I have already started on CI (Appveyor and Travis) in addition to looking into better startup performance.

@nicemd
Copy link

nicemd commented Aug 17, 2017

Does this mean the complete .NET sdk is required on the host?

@seesharper
Copy link
Collaborator Author

Yes, you would need the .Net Core SDK installed.

/// A <see cref="MetadataReferenceResolver"/> decorator that handles
/// references to NuGet packages in scripts.
/// </summary>
public class NuGetMetadataReferenceResolver : MetadataReferenceResolver
Copy link
Member

Choose a reason for hiding this comment

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

why is this internalized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've took most of the code from Dotnet.Script.NuGetMetadataResolver and tweaked it to have it work with csproj based scripting. I didn't want to mess up anything for OmniSharp which uses this package, so I figured we would benefit from having this code internally for now as we figure out all the details. When we have all the stuff working for csproj, we should probably move this out into NuGet packages again so that it can be consumed by for instance OmniSharp. When that time comes, the Dotnet.Script.NuGetMetadataResolver should be as small as possible containing nothing but the actual NuGetMetadataReferenceResolver class since that implementation basically just returns a dummy reference to itself.

The following code is from Dotnet.Script.NuGetMetadataResolver

if (reference.StartsWith("nuget", StringComparison.OrdinalIgnoreCase))
            {
                // HACK We need to return something here to "mark" the reference as resolved. 
                // https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/ReferenceManager/CommonReferenceManager.Resolution.cs#L838
                return ImmutableArray<PortableExecutableReference>.Empty.Add(
                    MetadataReference.CreateFromFile(typeof(NuGetMetadataReferenceResolver).GetTypeInfo().Assembly.Location));
            }

You might ask why we don't simply return a assembly reference to say typeof(string)?
The reason for that is we need to return something that matches the scripts target framework seen from OmniSharp.
When OmniSharp provides metadata references for a script, it does so in the context of the full framework unless the script has the following "shebang"

#! "netcoreapp1.1"

When that is missing, OmniSharp will provide references for the full fx and the we can not simply return a reference to typeof(string).Assembly since that would refer to the wrong "mscorlib".

The drawback of this approach is that we basically provide metadata for the whole Dotnet.Script.NuGetMetadataResolver assembly as a dummy reference.

Back to the scripting side of things, we are always a netcore app and we can safely just return the assembly containg the string type

/ HACK We need to return something here to "mark" the reference as resolved. 
                // https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/ReferenceManager/CommonReferenceManager.Resolution.cs#L838
                return ImmutableArray<PortableExecutableReference>.Empty.Add(
                    MetadataReference.CreateFromFile(typeof(string).GetTypeInfo().Assembly.Location));

Believe me when I say that this is the result of literally days of research and debugging :)

So until we figure out what can be shared between OmniSharp and script runners, I think we should just keep this internally for now as it allows us to move much faster. Hope this at least makes a little sense :)

Copy link
Member

Choose a reason for hiding this comment

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

thanks a lot - now it makes sense. it's a pretty neat workaround, given the limitations of the API

I actually now remember I ended up hitting the exact same thing when working with Paket, where I tried to expand a single reference line into multiple metadata references.
on a side note - interestingly - what Paket added afterwards, was that for inline nuget package references, it will generate a separate CSX file, containing #r reference to each of the assemblies resolved from the nuget package(s). You can then silently #load that into the script as it's being analyzed (for intellisense) or run (for runners). you still have the problem of "suppressing" the single paket #r directive, but the way of resolving the remaining references is done through the externally loaded CSX

anyway, makes sense. Perhaps at some point we should just involve Tomas and fix that silly single metadatareference check in Roslyn 😃

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once you get the next version out the door, maybe we can look at merging this PR. I am really excited to see how this behaves as a .NetCore2.0 app.

@filipw
Copy link
Member

filipw commented Aug 24, 2017

I bumped the version here to 0.13. YOLO let's merge this 😃 thanks!

@filipw filipw merged commit 8823acb into master Aug 24, 2017
@filipw filipw deleted the feature/csproj branch August 24, 2017 14:15
@seesharper
Copy link
Collaborator Author

Nice. Now the real fun begins :) I am going to look into moving to NetCoreApp2.0 and see if that solves the Linux issues 👍

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.

None yet

3 participants