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

Allow defining script target framework via configuration #1154

Merged

Conversation

filipw
Copy link
Member

@filipw filipw commented Apr 11, 2018

At the moment, in the script project system, the following model is used:

  • default everything to desktop CLR / net46, without Nuget support in scripts - to support typical usage with csi.exe for Desktop .NET
  • if the user enabled Nuget support by setting EnableScriptNuGetReferences = true in the OmniSharp settings, then OmniSharp additionally can resolve inline script NuGet packages (also in net46 context)
  • on top of that, if the script contained a framework directive: !# "netcoreapp2.0" then OmniSharp used .NET Core context instead of Desktop .NET to resolve the packages - the typical usage scenario are folks running scripts with dotnet-script for .NET Core

Since !# "{framework}" has no runtime meaning for the script runner - it's only used for OmniSharp to know how to resolve the compilation dependencies - this PR allows implicitly setting the framework version via the configuration file, instead of just reading it via the !# "{framework}".

For example, I could have an omnisharp.json file:

{
    "script": {
        "defaultTargetFramework": "netcoreapp2.0"
    }
}

This means that I can now author all my scripts without !# "netcoreapp2.0". This is beneficial for users who do not care about csi.exe and want to always use Nuget package resolution in scripts + .NET Core only for scripting.
Additionally, the way the code is set up, setting .NET Core as defaultTargetFramework would automatically opt into nuget package references since the two must go together.

As a bonus I added a strongly typed config object for script project system.

@filipw
Copy link
Member Author

filipw commented Apr 11, 2018

related issue: dotnet-script/dotnet-script#246

@seesharper
Copy link
Contributor

seesharper commented Apr 11, 2018

The #! "netcoreapp2.0" directive is used to set the target framework in the csproj file we generate behind the scenes to retrieve the compilation dependencies for a .Net Core script.

It is not used during script execution since we only support scripts that targets netcoreapp2.0.
When .Net Core 2.1 ships that will change to netcoreapp2.1.

It is not actually up to the script to determine if the target framework should be netcoreapp2.0 or netcoreapp2.1.
For .Net Core scripts it should be set to the latest netcoreapp version supported by the runner, in this case dotnet-script.

So how do we indicate that the script should be treated as a .Net Core script?

My suggestion is that we make this option define the target runtime rather than the target runtime.

{
    "script": {
        "defaultTargetRuntime": ".NetCore"
    }
}

If no shebang is defined in the script, this is the runtime we use. If not defined as an option, it defaults to NET46
Now for Dotnet.Script.DependencyModel we should stop treating the shebang as the target framework when resolving the compilation dependencies.
It should instead just expose the shebang so that OmniSharp can reason about the target runtime.

Imagine the following script

#!/usr/bin/env dotnet-script

This also means that on *nix, we can execute the script like this.

./main.csx

That would not of course work on Windows, but the shebang can still be used by OmniSharp to determine the target runtime.

From OmniSharp we could make a convention that if the sheban contains "dotnet script" or "dotnet-script", we set the runtime
to .Net Core overriding the defaultRuntime setting if specified.

If the runtime is .NetCore we will let Dotnet.Script.DependencyModel provide the compilation dependencies based on whatever the target framework for .Net Core will be at that moment.

If the shebang is missing we resolve to the setting if specified. If neither, we fall back to "NET46".

Dotnet.Script.DependencyModel if of no use for NET46 since we cannot retrieve the compilation dependencies for full .Net Framework.

Just my two cents 😀

@filipw
Copy link
Member Author

filipw commented Apr 11, 2018

Let's stick to only controlling this over TFM. This is the runner agnostic approach that we'd like to have in OmniSharp.

In general I like the idea of inferring the script runtime based on {...something...} if possible, but that is something better suited for a plugin into OmniSharp, I'd not put that into the core functionality.

@filipw
Copy link
Member Author

filipw commented Apr 11, 2018

Also updated the changelog - should be good to go 👍

@seesharper
Copy link
Contributor

I agree. LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants