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

Ensure that we don't load multiple compilation assemby versions #1037

Merged
merged 2 commits into from
Nov 30, 2017
Merged

Ensure that we don't load multiple compilation assemby versions #1037

merged 2 commits into from
Nov 30, 2017

Conversation

seesharper
Copy link
Contributor

This PR ensures that we don't load multiple versions of the same compilation assembly. This PR only affects loading metadata for scripts targeting .Net Core.

This PR also resolves the issue discussed here.
dotnet-script/dotnet-script#194

@@ -112,10 +112,15 @@ public void Initalize(IConfiguration configuration)
}
else
{
HashSet<string> loadedFiles = new HashSet<string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be case-insensitive? (e.g. using StringComparer.OrdinalIgnoreCase)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That certainly would not hurt 👍 Fixing

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.

LGTM!

@filipw
Copy link
Member

filipw commented Nov 30, 2017

thanks a lot, we may need to make it more robust (i.e. group by TFM and select reliably - whatever this could mean) but this will address the immediate issue

@david-driscoll david-driscoll merged commit c10d140 into OmniSharp:master Nov 30, 2017
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