Set up Sdk Dependency Model Ids to ignore version information #3205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Version was unset for earlier releases, but is set for the first time in .NET Core 2.1 creating problems matching SdkDependencyModel to NuGet Package references.
Pilchie
reviewed
Feb 1, 2018
| { | ||
| get | ||
| { | ||
| if (_id == null) |
There was a problem hiding this comment.
Is this really worth it? It looks like OriginalItemSpec is passed to the base already constructed. Does the property do anything other than just return it?
There was a problem hiding this comment.
Good point, OriginalItemSpec does not change so we should just return it. I just copied the pattern used in the other derived classes.
Pilchie
approved these changes
Feb 1, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Version was unset for earlier releases, but is set for the first time in .NET Core 2.1 (by dotnet/sdk#1572) creating problems matching SdkDependencyModel to NuGet Package references. See #3184 (comment) for more details
This fix overrides base class implementation and ignores version information. It turns out this approach is used in other derived classes as well. I also considered a fix in which the broken filter checks for SDK using an Id without version and one with version, but this approach seems more consistent with other models in this project.
At this point, it is unclear why the base class implementation needs to include version in its ID since many of its derived classes ignore version or leave it empty. But since this is an escrow fix, I'll avoid making a sweeping change.
Customer scenario
Customers using .NET Core 2.1 SDK see unresolved warning in dependency node, and cannot expand SDK to see its packages
Bugs this fixes:
Fixes #3184
Fixes 556559
Workarounds, if any
Close and reopen solution often resolves this, but this depends on the order of dataflows
Risk
Low, previous SDKs did not include version information either
Performance impact
Low, this just overrides a base class implementation
Is this a regression from a previous update?
Regression from .NET Core 2.0 to .NET Core 2.1 SDK
Root cause analysis:
.NET Core SDK team probably noticed version was missing and added it. 2.1 is still in early preview so problems like this are being weeded out.
How was the bug found?
Test team
/cc @dotnet/project-system