Add support for transitive framework references#3221
Add support for transitive framework references#3221dsplaisted merged 7 commits intodotnet:masterfrom
Conversation
Fixes dotnet/cli#10666
|
|
||
| private void WriteFrameworkReferences() | ||
| { | ||
| foreach (var library in _runtimeTarget.Libraries) |
There was a problem hiding this comment.
Let's add a DisableTransitiveFrameworkReferences here to match the DisableTransitiveProjectReferences. Also, I think we should dedupe here like we do for framework assemblies. Better to keep the cache small than to dedupe in targets afterwards.
Finally, if transitive framework references are disabled, we should change resolve framework references to not download all packs.
There was a problem hiding this comment.
@nguerrera I'm not sure there's value in opting out of transitive FrameworkReferences. There's no split between compile and runtime assets for framework references, so disabling transitive framework references would either have no effect (since you already have the same direct references), or would break your app at runtime (since the framework your dependency needs isn't written to the runtimeconfig).
What do you think?
There was a problem hiding this comment.
I'd still like there to be an escape hatch.
| <_TransitiveFrameworkReferenceToResolve Include="@(TransitiveFrameworkReferences->Distinct())"/> | ||
| <_TransitiveFrameworkReferenceToResolve Remove="@(FrameworkReference)" /> | ||
|
|
||
| <FrameworkReference Include="@(_TransitiveFrameworkReferenceToResolve)" /> |
There was a problem hiding this comment.
After deduping in C#, I think this could be:
| <FrameworkReference Include="@(_TransitiveFrameworkReferenceToResolve)" /> | |
| <FrameworkReference Include="@(TransitiveFrameworkReferences)" Exclude="@(FrameworkReference)" /> |
dsplaisted
force-pushed
the
transitive-framework-references
branch
from
963676a to
1e55736
Compare
| } | ||
| if (!frameworkReferences.Contains(frameworkReference)) | ||
| { | ||
| frameworkReferences.Add(frameworkReference); |
There was a problem hiding this comment.
What did we decide about case sensitivity of framework references?
|
|
||
| // Currently there are only 2 different frameworks that we expect to | ||
| // show up as framework references, so use a list here instead of a | ||
| // HashSet |
There was a problem hiding this comment.
I'd just use a HashSet, I don't think it will be much worse for 2 elements (looks like it will be backed by 3 element Slot struct array) and then we don't need to think hard about this every time we come across the O(N^2) here. Unless we've measured meaningful gain, I prefer the natural data structure that scales than one that needs a comment like this.
There was a problem hiding this comment.
Hmm, but we probably want the order to be deterministic, meaning we'd need SortedSet or a List AND a HashSet.... Less sure now.
There was a problem hiding this comment.
I implemented it the same way other similar methods work: the HashSet keeps track of items for which you've already called WriteItem
| BlobFeedUrl: $(PB_PublishBlobFeedUrl) | ||
| PublishType: $(_PublishType) | ||
|
|
||
| - task: PublishTestResults@1 |
There was a problem hiding this comment.
This should not be needed. It is done by arcade. Any reason why you are adding it back?
There was a problem hiding this comment.
Arcade doesn't give you meaningful names for the test results or the zip files. See here for example:
There's discussion of this on this arcade PR: dotnet/arcade#2525. I made a similar change to core-sdk here: dotnet/installer#1609
dsplaisted
force-pushed
the
transitive-framework-references
branch
from
4639b1e to
5032fcc
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dsplaisted
force-pushed
the
transitive-framework-references
branch
from
5032fcc to
4d31c70
Compare
|
@nguerrera @livarcocc This is passing now, does anyone want to sign off on it? |
….6 (dotnet#3221) - Microsoft.DotNet.Cli.Runtime - 3.1.100-preview2.19517.6
3 participants
Fixes dotnet/cli#10666