Add Framework Assemblies from project assets file to compiler references #489
Conversation
…references. Fixes dotnet#365 and dotnet#409. Note that implicit framework references already address the scenario in dotnet#409 which only relies on 'System'
|
/cc @dsplaisted @dotnet/project-system |
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore" Version="1.0.0" /> |
There was a problem hiding this comment.
Shouldn't this have a reference to Microsoft.NETCore.App until #450 is merged?
There was a problem hiding this comment.
Is this for the case where the SDK is not present? I would have expected Project Sdk="Microsoft.NET.Sdk" to be sufficient here, and it appears to work on all my test machines.
There was a problem hiding this comment.
NerCore.App is needed for netcore projects (even if the SDK is present). This is however a Desktop project - so not needed here.
| var propertyGroup = project.Root.Elements(ns + "PropertyGroup").FirstOrDefault(); | ||
| propertyGroup.Should().NotBeNull(); | ||
|
|
||
| propertyGroup.Add(new XElement(ns + "DisableImplicitFrameworkReferences", "true")); |
There was a problem hiding this comment.
You were probably copying this from code that I wrote, but it might just be easier to invoke the build command with a /p:DisableImplicitFrameworkReferences=true parameter, rather than modifying the XML here.
| } | ||
|
|
||
| [Fact] | ||
| public void It_builds_the_projects_successfully_twice() |
There was a problem hiding this comment.
Is there a specific reason we need to test that these projects build correctly when you do it twice in a row?
| @@ -35,6 +35,7 @@ public void It_publishes_the_project_with_a_refs_folder_and_correct_deps_file() | |||
| propertyGroup.Should().NotBeNull(); | |||
|
|
|||
| propertyGroup.Add(new XElement(ns + "DisableImplicitFrameworkReferences", "true")); | |||
| propertyGroup.Add(new XElement(ns + "DisableLockFileFrameworks", "true")); | |||
There was a problem hiding this comment.
Can you update the comment to point to dotnet/msbuild#1345 (as #367 has been closed in favor of that). Also, these workarounds should only be necessary when targeting .NET Framework, so it would be nice to condition them based on that.
There was a problem hiding this comment.
Thinking about this a bit more, I think that even if dotnet/msbuild#1345 is fixed, that it could still be a problem if there is a reference to the same framework assembly coming both from a NuGet FrameworkReference and the implicit framework references from the SDK.
So can you either disable the implicit framework references if targeting .NET 4.5 or greater (which is the lowest thing supported by .NET Standard), or file a bug to follow up and investigate?
There was a problem hiding this comment.
I've filed #536 to investigate when the MSBuild issue is fixed
| @@ -361,6 +361,11 @@ private void GetFileDependencies(LockFileTargetLibrary package, string targetNam | |||
| item.SetMetadata(MetadataKeys.ParentTarget, targetName); // Foreign Key | |||
| item.SetMetadata(MetadataKeys.ParentPackage, packageId); // Foreign Key | |||
|
|
|||
| if (fileGroup == FileGroup.FrameworkAssembly) | |||
There was a problem hiding this comment.
I think it would be helpful to have a comment here explaining how the FrameworkAssembly reference is expressed in the lockfile. From the code I gather that it has a path which instead of referring to a file in the package is just the name of the framework reference to add.
… as build arguments
|
/cc @srivatsn for Ask mode approval. This is ready to go in. |
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore" Version="1.0.0" /> |
There was a problem hiding this comment.
NerCore.App is needed for netcore projects (even if the SDK is present). This is however a Desktop project - so not needed here.
…214.4 (dotnet#489) This change updates the following dependencies - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19114.4
Add Framework Assemblies raised from project assets file to compiler references. Previously, we were not doing anything with this information.
Fixes #365 and #409. Note that the scenario in #409 is already addressed by implicit framework references because the scenario only relies on 'System'.
To workaround a failure in a preexisting test due to dotnet/msbuild#1345, I added the ability to disable the new targets but they are on by default.