-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow .NET Core 2.0 projects to reference assemblies on disk #876
Conversation
Is there an issue tracking this on NuGet side? |
IDK. @rrelyea @emgarten @rohit21agrawal - ? |
@@ -28,7 +28,9 @@ public class GivenADependencyContextBuilder | |||
CompilationOptions compilationOptions, | |||
string baselineFileName, | |||
string runtime, | |||
ITaskItem[] satelliteAssemblies) | |||
ITaskItem[] assemblySatelliteAssemblies, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is an assemblySatelliteAssemblies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it is a poor name. But it matches the name in the product (GenerateDepsFile class). And the reason it is named that in the product is because it matches all the other properties:
- AssemblyName
- AssemblyExtension
- AssemblyVersion
- AssemblySatelliteAssemblies
public string FileName => Path.GetFileName(FullPath); | ||
|
||
private List<ResourceAssemblyInfo> _resourceAssemblies; | ||
public IEnumerable<ResourceAssemblyInfo> ResourceAssemblies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public IEnumerable ResourceAssemblies { get; }?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this doesn't work because we need to add to the list, but want to expose the member as IEnumerable.
|
||
public static ResourceAssemblyInfo CreateFromReferenceSatellitePath(ITaskItem referenceSatellitePath) | ||
{ | ||
string destinationSubDirectory = referenceSatellitePath.GetMetadata("DestinationSubDirectory"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the task item does not contain this metadata? Won't this produce a broken relative path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will be broken if we have a satellite item without this metadata. What should we do in that case? Throw an exception? Ignore this satellite item? Try to use it without knowing its culture?
@eerhardt, please retarget to master. |
👍 On it. |
Adding <Reference> items to existing assemblies into the .deps.json file so the host knows to read these assemblies at runtime. Fix dotnet#120
@dsplaisted @nguerrera - can I get a review? I'd like to check this change in tomorrow. |
…0190809.8 (dotnet#876) - Microsoft.AspNetCore.Mvc.Analyzers - 5.0.0-alpha1.19409.8 - Microsoft.AspNetCore.Mvc.Api.Analyzers - 5.0.0-alpha1.19409.8 - Microsoft.AspNetCore.Analyzers - 5.0.0-alpha1.19409.8 - Microsoft.AspNetCore.Components.Analyzers - 5.0.0-alpha1.19409.8
Fix #120
The necessary change to the product is to generate the correct .deps.json file. We were already copying the reference assembly and supporting files into the build/publish folder.
Note this doesn't attempt to fix 'dotnet pack' for this scenario. That will be up to the NuGet team.
@nguerrera @dsplaisted @srivatsn
/cc @terrajobst