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

Bump Dotnet.Script.DependencyModel to version 0.50.0 #1609

Merged
merged 17 commits into from
Oct 2, 2019

Conversation

seesharper
Copy link
Contributor

This PR bumps the Dotnet.Script.DependencyModel and Dotnet.Script.DependencyModel.NuGet packages to version 0.50.0 adding support for .Net Core 3.0 based scripts.

<ItemGroup>
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0-beta2-18618-05" PrivateAssets="All" />
</ItemGroup>
<Project ToolsVersion="14.0"
Copy link
Member

Choose a reason for hiding this comment

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

could you please reset the changes to this file so that only the relevant lines are updated? we often look here at the blame to identify when a specific dependency was introduced - and this will break it

@@ -66,8 +69,9 @@ public ScriptContext CreateScriptContext(ScriptOptions scriptOptions)
CompilationDependency[] compilationDependencies = null;
try
{
var allCsxFiles = _fileSystemHelper.GetFiles("**/*.csx").ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

the same files are already discovered in ScriptProjectSystem; since this is potentially an expensive operation at start up, we should not do it twice. can we pass a list of files here instead?

_scriptOptions = new ScriptOptions();
_scriptOptions.CsxFiles = allCsxFiles;
Copy link
Member

Choose a reason for hiding this comment

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

actually it would be better to just add an extra argument to CreateScriptContext(...).
ScriptOptions is bound from the omnisharp.json so it's not good idea to use it for just transporting objects or data from one place to another

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 - thanks!

@seesharper seesharper changed the title Bump Dotnet.Script.DependencyModel to version 0.50.0 NO MERGE Bump Dotnet.Script.DependencyModel to version 0.50.0 Sep 27, 2019
@seesharper
Copy link
Contributor Author

. Net Core 3.0 compilation deps needs more work

@seesharper seesharper changed the title NO MERGE Bump Dotnet.Script.DependencyModel to version 0.50.0 Bump Dotnet.Script.DependencyModel to version 0.50.0 Sep 30, 2019
@filipw
Copy link
Member

filipw commented Sep 30, 2019

@mholo65 any thoughts here?
@david-driscoll you OK with bumping Nuget deps to 5.2.0?

@@ -92,7 +111,13 @@ public ScriptContext CreateScriptContext(ScriptOptions scriptOptions)
isDesktopClr = false;
HashSet<string> loadedFiles = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

foreach (var compilationAssembly in compilationDependencies.SelectMany(cd => cd.AssemblyPaths).Distinct())
// Pick the highest version
var resolvedAssemblyPaths = compilationDependencies.SelectMany(cd => cd.AssemblyPaths)
Copy link
Member

Choose a reason for hiding this comment

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

"one liner" 😂

@david-driscoll
Copy link
Member

@filipw yeah I'm fine with nuget 5.2, we need to stay more up to date on these things anyway.

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.

3 participants