Support runtime packs with differing runtime IDs #12199
Conversation
sfoslund
changed the title
|
Thank you so much for look into it! This fix looks like right (the existing code is too long and convoluted and lack of tests. I cannot say it is good for sure). Let's unblock the insertion first. |
|
@wli3 thanks for taking a look, the tests I added here are ported directly from the ones that failed in installer. Does it make sense to check them in here and remove them in installer or should I take them out of this PR? Also do you want me to leave #12117 open to track adding more test coverage/ refactoring the code? |
| runtimePackForRuntimeIDProcessing = knownFrameworkReference.ToKnownRuntimePack(); | ||
| includeInPackageDownload = true; | ||
| } | ||
| else |
There was a problem hiding this comment.
this "else" I am not too sure. Is it the same logic before #11824 ?
| } | ||
|
|
||
| [WindowsOnlyFact] | ||
| public void ItCanPublishArm64Winforms() |
There was a problem hiding this comment.
You can write unit test for this task like src\Tasks\Microsoft.NET.Build.Tasks.UnitTests\ProcessFrameworkReferencesTests.cs. It will be at least better for documenting this long class
| if (knownFrameworkReference.Name.Equals(knownFrameworkReference.RuntimeFrameworkName, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| bool processedPrimaryRuntimeIdentifier = false; | ||
| // Only add runtime packs where the framework reference name matches the RuntimeFrameworkName |
There was a problem hiding this comment.
Ideally these comments should be able to express in code. But meanwhile this class should really split into 4-5 class to make things clear. I will try to refactor that. (you should try too if you have time. reference http://arlobelshee.com/the-core-6-refactorings/)
|
Merging now to get insertions going again, #12117 will track handling this PR feedback. |
2 participants
Bug fix for #11824
Fixes part of #12117