Skip to content

Bug/issue166 #169

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 7, 2017
Merged

Bug/issue166 #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2806730
Added test case for System.Data.SqlClient
seesharper Nov 7, 2017
b380424
Now works if we resolve as netcoreapp1.1 #166
seesharper Nov 7, 2017
b446e2a
Cleanup #166
seesharper Nov 7, 2017
6cd3133
Fix dependency preference and test cleanup
seesharper Nov 7, 2017
18e39cb
Removed launch config from test fixture
seesharper Nov 7, 2017
4b2f43a
Merge branch 'master' into bug/issue166
seesharper Nov 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions src/Dotnet.Script.Core/ScriptCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,26 +97,27 @@ public virtual ScriptCompilationContext<TReturn> CreateCompilationContext<TRetur
var inheritedAssemblyNames = DependencyContext.Default.GetRuntimeAssemblyNames(runtimeId).Where(x =>
x.FullName.StartsWith("system.", StringComparison.OrdinalIgnoreCase) ||
x.FullName.StartsWith("microsoft.codeanalysis", StringComparison.OrdinalIgnoreCase) ||
x.FullName.StartsWith("mscorlib", StringComparison.OrdinalIgnoreCase));

foreach (var inheritedAssemblyName in inheritedAssemblyNames)
{
Logger.Verbose("Adding reference to an inherited dependency => " + inheritedAssemblyName.FullName);
var assembly = Assembly.Load(inheritedAssemblyName);
opts = opts.AddReferences(assembly);
}

x.FullName.StartsWith("mscorlib", StringComparison.OrdinalIgnoreCase)).ToArray();

IList<RuntimeDependency> runtimeDependencies =
RuntimeDependencyResolver.GetDependencies(context.WorkingDirectory).ToList();

foreach (var runtimeDependency in runtimeDependencies)
{
Logger.Verbose("Adding reference to a runtime dependency => " + runtimeDependency);
opts = opts.AddReferences(MetadataReference.CreateFromFile(runtimeDependency.Path));

}

foreach (var inheritedAssemblyName in inheritedAssemblyNames)
{
var runtimeDependencyAssemblyName = runtimeDependency.Name;
if (!inheritedAssemblyNames.Any(an => an.Name == runtimeDependencyAssemblyName.Name))
Logger.Verbose("Adding reference to an inherited dependency => " + inheritedAssemblyName.FullName);
// Always prefer the resolved runtime dependency rather than the inherited assembly.
if (runtimeDependencies.All(rd => rd.Name.Name != inheritedAssemblyName.Name))
{
Logger.Verbose("Adding reference to a runtime dependency => " + runtimeDependency);
opts = opts.AddReferences(MetadataReference.CreateFromFile(runtimeDependency.Path));
}
var assembly = Assembly.Load(inheritedAssemblyName);
opts = opts.AddReferences(assembly);
}
}

var loader = new InteractiveAssemblyLoader();
Expand Down
6 changes: 2 additions & 4 deletions src/Dotnet.Script.DependencyModel/Environment/RuntimeHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@ public static string GetPathToNuGetStoreFolder()

private static string GetProcessArchitecture()
{
return RuntimeEnvironment.RuntimeArchitecture;

return RuntimeEnvironment.RuntimeArchitecture;
}

public static string GetRuntimeIdentifier()
Expand All @@ -55,8 +54,7 @@ public static string GetRuntimeIdentifier()
}

internal static bool AppliesToCurrentRuntime(string runtime)
{
//var regex = new Regex("win.*-x64", RegexOptions.IgnoreCase);
{
return string.IsNullOrWhiteSpace(runtime) || RuntimeMatcher.IsMatch(runtime);
}
}
Expand Down
30 changes: 20 additions & 10 deletions src/Dotnet.Script.DependencyModel/Runtime/RuntimeDependencyResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,19 +104,29 @@ private void ProcessNativeLibraries(RuntimeLibrary runtimeLibrary, string[] nuge
}
private void ProcessRuntimeAssemblies(RuntimeLibrary runtimeLibrary,
HashSet<RuntimeDependency> resolvedDependencies, string[] nugetPackageFolders)
{
foreach (var runtimeAssemblyGroup in runtimeLibrary.RuntimeAssemblyGroups.Where(rag => RuntimeHelper.AppliesToCurrentRuntime(rag.Runtime)))
{
var runtimeAssemblyGroup =
runtimeLibrary.RuntimeAssemblyGroups.FirstOrDefault(rag =>
rag.Runtime == RuntimeHelper.GetPlatformIdentifier());

if (runtimeAssemblyGroup == null)
Copy link
Member

Choose a reason for hiding this comment

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

one question - shouldn't it work in the following way (I'm not sure myself, but I have vague recollection from project.json):

  1. grab all from the "empty" assembly group
  2. look into platform specific assembly group, and grab all from there too. If there are any platform-specific DLLs that have the same names as picked in 1., then platform specific should win.

I'm worried we may miss some dependencies if we just look into platform specific assembly group

Copy link
Collaborator Author

@seesharper seesharper Nov 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We used to do something like this.

dotnet restore script.csproj -r win7-x64

now we do

dotnet restore script.csproj

The difference is that we get a much smaller project.assets.jsonfile, but the semantics around how we read the dependency context also changes a bit then.

What we get is a dependency context that is not tied to an specific runtime.
Think dotnet run someassembly.dll vs publishing to a specific platform that may even produce an exe for us to run on windows.

What we do is look for a runtime assembly group that is specific to the platform. In the case of the System.Data.SqlClient package there are 3 assembly groups, win, unix and the group with an empty "runtime".Each of these groups point to a system.data.sqlclient.dll file If we don't find a group specific to the platform, we look for the one without an specified platform. What we did prior to this PR was that we preferred the one without a runtime identifier and that caused the PlatformNotSupported exception as described in #166 .

Did that make any sense at all?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I wasn't clear.

The question was, isn't it possible in the layout of the restored packages, that the "empty" group would contain assemblies X and Y, but i.e. the "win" group would only contain X (windows version). This way we end up missing Y. Not sure if this is a valid scenario, but I somehow feel like it is? If an assembly in the "empty" group, doesn't have a counterpart in the platform specific group, it feels like it should be loaded too.

Copy link
Collaborator Author

@seesharper seesharper Nov 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

To be honest, I don't know either. But the 99% case for all packages is that they contain a single assembly. Other assemblies are usually brought in through NuGet dependencies.
This package in particular brings in other assets, but that is native dll's that are processed separately. With this PR we don't bring in fewer dependencies, we just bring in the "right" ones :) But, I can't guarantee that the scenario you mention won't happen. It just hasn't happened yet and the more test cases we create covering a wider range of packages, the more certain we can be that we do that we do the right thing:) As I mentioned before we need to revisit this when adding support for script packages, but for now I would say that we are okay :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But if you would like to check the empty group for assets that is not the platform counterpart, we can do that. No problem

Copy link
Member

Choose a reason for hiding this comment

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

let's leave this as is, we can come back to that if it causes problems 👍

{
runtimeAssemblyGroup =
runtimeLibrary.RuntimeAssemblyGroups.FirstOrDefault(rag => string.IsNullOrWhiteSpace(rag.Runtime));
}
if (runtimeAssemblyGroup == null)
{
return;
}
foreach (var assetPath in runtimeAssemblyGroup.AssetPaths)
{
foreach (var assetPath in runtimeAssemblyGroup.AssetPaths)
var path = Path.Combine(runtimeLibrary.Path, assetPath);
if (!path.EndsWith("_._"))
{
var path = Path.Combine(runtimeLibrary.Path, assetPath);
if (!path.EndsWith("_._"))
{
var fullPath = GetFullPath(path, nugetPackageFolders);
var fullPath = GetFullPath(path, nugetPackageFolders);

_logger.Debug($"Resolved runtime library {runtimeLibrary.Name} located at {fullPath}");
resolvedDependencies.Add(new RuntimeDependency(AssemblyName.GetAssemblyName(fullPath), fullPath));
}
_logger.Debug($"Resolved runtime library {runtimeLibrary.Name} located at {fullPath}");
resolvedDependencies.Add(new RuntimeDependency(AssemblyName.GetAssemblyName(fullPath), fullPath));
}
}
}
Expand Down
12 changes: 12 additions & 0 deletions src/Dotnet.Script.Tests/ScriptExecutionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,18 @@ public static void ShouldHandleIssue129()
Assert.Contains("Bad HTTP authentication header", result.output);
}

[Fact]
public static void ShouldHandleIssue166()
{
// System.Data.SqlClient loads native assets
// No story on *nix yet.
if (RuntimeHelper.IsWindows())
{
var result = Execute(Path.Combine("Issue166", "Issue166.csx"));
Assert.Contains("Connection successful", result.output);
}
}

[Fact]
public static void ShouldPassUnknownArgumentToScript()
{
Expand Down
12 changes: 12 additions & 0 deletions src/Dotnet.Script.Tests/TestFixtures/Issue166/Issue166.csx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#! "netcoreapp2.0"
#r "nuget:NetStandard.Library,2.0.0"
#r "nuget:System.Data.SqlClient, 4.4.0"

using System.Data.SqlClient;


using (var connection = new SqlConnection(@"Server=tcp:dotnet-script.database.windows.net,1433;Initial Catalog=Sample;Persist Security Info=False;User ID=readonlylogin;Password=ztygpRhfjgej0we|dxHfxybhmsFT7_&#$!~<!n5,lfvtButr;MultipleActiveResultSets=False;Encrypt=True;TrustServerCertificate=False;Connection Timeout=30;"))
{
connection.Open();
Console.WriteLine("Connection successful");
}