Skip to content
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

Fix for project sstem issue 2069, to specify correct metadata for package for different targets… #1149

Merged
merged 2 commits into from
Apr 26, 2017

Conversation

abpiskunov
Copy link
Contributor

We reused item object for packages across targets instead of cloning them and it caused all package items for all targets to have dependencyIDs property updated.

Fix for dotnet/project-system#2069

@abpiskunov
Copy link
Contributor Author

@natidea @livarcocc @srivatsn

@abpiskunov
Copy link
Contributor Author

@nguerrera

@@ -236,7 +236,7 @@ private DependencyType GetDependencyType(string dependencyTypeString)

var currentPackageUniqueId = $"{parentTargetId}/{currentItemId}";
// add current package to dependencies world
var currentItem = items[currentItemId];
Copy link
Contributor

@natidea natidea Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

items[currentItemId] [](start = 34, length = 20)

So the objects in items are never added to DependenciesWorld? I guess this is unnecessary extra work in Single-targeting projects(?) -- a (perhaps minor) performance enhancement may be to do this only on cross-targetting projects. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed a change for that


In reply to: 113310902 [](ancestors = 113310902)

@natidea
Copy link
Contributor

natidea commented Apr 25, 2017

/cc @MattGertz for Ask Mode

Customer scenario

In cross targeting projects, customers may see incorrect nodes in the dependency tree in solution explorer. This is because the task raising dependency hierarchy incorrectly reuses nodes created for the first TFM when processing the hierarchy for subsequent TFMs. The fix is to clone, rather than reuse nodes in multitargeting situations

Bugs this fixes:

dotnet/project-system#2069

Workarounds, if any

N/A

Risk

Low

Performance impact

Low. Cloning is applied only in cross-targeting projects where this is the correct thing to do

Is this a regression from a previous update?

No

Root cause analysis:

Existing bug exposed by 15.3 work to show all TFMs in solution explorer

How was the bug found?

Dogfooding

@livarcocc
Copy link
Contributor

@natidea @abpiskunov is this for master (preview2) or for release/2.0.0 (preview1)?

@srivatsn
Copy link
Contributor

@abpiskunov can you retarget this to rel/1.0.0?

@abpiskunov abpiskunov changed the base branch from master to rel/1.0.0 April 26, 2017 20:43
@srivatsn srivatsn changed the base branch from rel/1.0.0 to master April 26, 2017 20:51
…kage for different targets (We reused items for packages accross targets instead of cloning them and it caused all package items for all targets to have dependencyIDs property updated)
@srivatsn srivatsn force-pushed the ap/u3/depenndencyIDs branch from ecdf69d to 6ca35d0 Compare April 26, 2017 21:04
@srivatsn srivatsn changed the base branch from master to rel/1.0.0 April 26, 2017 21:04
@livarcocc livarcocc merged commit 82807ab into dotnet:rel/1.0.0 Apr 26, 2017
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
…0191202.2 (dotnet#1149)

- Microsoft.AspNetCore.Analyzers - 5.0.0-alpha1.19602.2
- Microsoft.AspNetCore.Mvc.Api.Analyzers - 5.0.0-alpha1.19602.2
- Microsoft.AspNetCore.Mvc.Analyzers - 5.0.0-alpha1.19602.2
- Microsoft.AspNetCore.Components.Analyzers - 5.0.0-alpha1.19602.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants