Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support RSP-based configuration for scripting project system #1112

Merged
merged 12 commits into from
May 21, 2018

Conversation

filipw
Copy link
Member

@filipw filipw commented Feb 15, 2018

Fixes #1024

This PR introduces the ability to configure a path to RSP file which is picked up by the scripting project system.
The RSP is not supported in the compiler-specific traditional way (where it can configure almost all of the compilation options) but rather is just a vessel to provide custom assembly references (/r:...) and global namespaces (/u:...). This is consistent with CSI behavior, which also uses RSP files only for these two types of input (when it comes to compilation options at least).

The obvious user benefit of all of this is, if I built my own custom script runner using Roslyn Scripting Engine, I could use OmniSharp for intellisense and language services against it (or at least to certain degree). That's due to the fact that I'd be able to predefine my implicitly available namespaces and assembly references using an RSP file now.

The way it all works:

  • I can specify a path to an RSP file in my omnisharp.json file - at global or local level, relative or absolute
{
    "Script": {
        "EnableScriptNuGetReferences": true,
        "RspFilePath": "C:/omnisharp-ext/foo.rsp"
    }
}
  • if there are any namespaces in the RSP file, they replace the default set we normally have (the CSI-set)
  • if there are any assembly references in the RSP file, they are merged with default set we have (the one we have is really only aiming to bring in object reference - this is default requirement of C# scripting anyway, hence the merge not replace)
  • this feature relies on an API that @DustinCampbell opened up recently Make C# and VB CommandLineParser.Script public dotnet/roslyn#23435. However, that hasn't shipped yet, so the PR still is based on reflection. Not sure whether we want to wait with it until next Roslyn release is out?

As a bonus, I added strongly typed scripting configuration (it was dictionary based).
I will try to invent some creative way to test this, at the moment the PR comes without any tests unfortunately.

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.

I'm fine with the reflection based stuff (It's not like we haven't done this before...)

@david-driscoll
Copy link
Member

There is a conflict that needs merging. 😄


private CSharpCommandLineArguments CreateCommandLineArguments()
{
var scriptRunnerParserProperty = typeof(CSharpCommandLineParser).GetProperty("ScriptRunner", BindingFlags.Static | BindingFlags.NonPublic);
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I don't think you need this anymore. CSharpCommandLineParser.Script appears to be public in Roslyn 2.7.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right! removed the reflection

@filipw filipw force-pushed the feature/rsp-script2 branch from bd5b26c to eb81551 Compare May 2, 2018 18:19
@filipw
Copy link
Member Author

filipw commented May 2, 2018

There is a conflict that needs merging. 😄

thanks - I forgot about this PR 😇😇 It's rebased now. Remarkable in how many conflicts I got in with myself.

@david-driscoll david-driscoll merged commit 52df2b8 into OmniSharp:master May 21, 2018
@filipw filipw deleted the feature/rsp-script2 branch May 22, 2018 12:27
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants