Bug/issue166 #169
Bug/issue166 #169
Conversation
| runtimeLibrary.RuntimeAssemblyGroups.FirstOrDefault(rag => | ||
| rag.Runtime == RuntimeHelper.GetPlatformIdentifier()); | ||
|
|
||
| if (runtimeAssemblyGroup == null) |
There was a problem hiding this comment.
one question - shouldn't it work in the following way (I'm not sure myself, but I have vague recollection from project.json):
- grab all from the "empty" assembly group
- 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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
But if you would like to check the empty group for assets that is not the platform counterpart, we can do that. No problem
There was a problem hiding this comment.
let's leave this as is, we can come back to that if it causes problems 👍
|
Are we good to merge?, @filipw Would like to get this into the script packages branch before I move on there :) |
|
thanks! |
This PR fixes #166.
Two things has changed
In order to actually make sure that we can create a connection to a SQL Server according to issue #166, I have created a database up on Azure. The connection is made with a read only user that is only allowed to connect/login.
Note: The
RuntimeDependencyResolverneeds a little cleanup, but we are going to make some changes there anyway to accommodate for script packages