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.

Sign up for GitHub

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

Jump to bottom

[RC2 BLOCKING][CLI MIGRATION BLOCKING] Implicit Imports Break DeepClone #1431

Closed
TheRealPiotrP opened this issue Dec 4, 2016 · 5 comments
Closed

[RC2 BLOCKING][CLI MIGRATION BLOCKING] Implicit Imports Break DeepClone #1431

TheRealPiotrP opened this issue Dec 4, 2016 · 5 comments
Assignees
@rainersigwald
Labels
bug triaged
Milestone
Feature: Sdks

Comments

@TheRealPiotrP
Copy link
Contributor

deepclone.zip

See attached repro. dotnet restore and dotnet run this one.

Not having looked at the code, I suspect that DeepClone is trying to insert xml relative to the implicit [and therefore not expressed in xml] imports element. Since the element doesn't actually exist, System.Xml throws:

Unhandled Exception: System.ArgumentException: The reference node is not a child of this node.
   at System.Xml.XmlNode.InsertAfter(XmlNode newChild, XmlNode refChild)
   at Microsoft.Build.Construction.ProjectElementContainer.AddToXml(ProjectElement child)
   at Microsoft.Build.Construction.ProjectElementContainer.InsertAfterChild(ProjectElement child, ProjectElement reference)
   at Microsoft.Build.Construction.ProjectElementContainer.DeepClone(ProjectRootElement factory, ProjectElementContainer parent)
   at Microsoft.Build.Construction.ProjectElementContainer.DeepClone(ProjectRootElement factory, ProjectElementContainer parent)
   at Microsoft.Build.Construction.ProjectRootElement.DeepClone()
   at Program.Main(String[] args)

/cc @rainersigwald @cdmihai @AndyGerlicher @MattGertz
@TheRealPiotrP
Copy link
Contributor Author

This blocks CLI and therefore RC2 because CLI deep clones the dotnet new template csproj as a skeleton for migrated apps.

@TheRealPiotrP
Copy link
Contributor Author

TheRealPiotrP commented Dec 4, 2016 •
edited
Loading

Fix likely is here: https://github.com/Microsoft/msbuild/blob/45cecd3a65090cafc6a09c2dd59ed85d1b805c88/src/XMakeBuildEngine/Construction/ProjectElementContainer.cs#L488

Predicate<ProjectElement> siblingIsSameAsChild = _ => 
      _.ExpressedAsAttribute == false 
   && _.IsImplicit == false;

@rainersigwald rainersigwald self-assigned this Dec 4, 2016
@rainersigwald
Copy link
Member

That does look like the right fix. Testing it out now.

For context, migration always happens through the CLI, right? If so, I'd like to propose taking this fix only for CLI--the risk of being one change apart seems lower than the risk of taking a change to VS this late.

rainersigwald added a commit to rainersigwald/msbuild that referenced this issue Dec 4, 2016
@rainersigwald
Test DeepClone of project with Sdk
a20cb40 
Test case for dotnet#1431
rainersigwald added a commit to rainersigwald/msbuild that referenced this issue Dec 4, 2016
@rainersigwald
Consider Implicit status to find insertion loc
69c6ec2 
Fixes dotnet#1431.

If a project had an Sdk attribute (and thus implicit Import elements), calling DeepClone on it could throw:

```
Unhandled Exception: System.ArgumentException: The reference node is not a child of this node.
   at System.Xml.XmlNode.InsertAfter(XmlNode newChild, XmlNode refChild)
   at Microsoft.Build.Construction.ProjectElementContainer.AddToXml(ProjectElement child)
   at Microsoft.Build.Construction.ProjectElementContainer.InsertAfterChild(ProjectElement child, ProjectElement reference)
   at Microsoft.Build.Construction.ProjectElementContainer.DeepClone(ProjectRootElement factory, ProjectElementContainer parent)
   at Microsoft.Build.Construction.ProjectElementContainer.DeepClone(ProjectRootElement factory, ProjectElementContainer parent)
   at Microsoft.Build.Construction.ProjectRootElement.DeepClone()
```

This was because we traverse the ProjectElement tree to find a sibling XML element to insert next to. If an adjacent ProjectElement's XML element has no physical representation, it is unparented and therefore not a good target for manipulations.

Add an implicitness check to the existing is-valid-for-referencing check to cause the next-XML-tree-sibling search to pass over implicit elements.
@rainersigwald rainersigwald mentioned this issue Dec 4, 2016
Merged
@rainersigwald rainersigwald added this to the MSBuild Sdks milestone Dec 4, 2016
rainersigwald added a commit that referenced this issue Dec 4, 2016
@rainersigwald
Test DeepClone of project with Sdk
04f14be 
Test case for #1431
rainersigwald added a commit that referenced this issue Dec 4, 2016
@rainersigwald
Consider Implicit status to find insertion loc
8db3670 
Fixes #1431.

If a project had an Sdk attribute (and thus implicit Import elements), calling DeepClone on it could throw:

```
Unhandled Exception: System.ArgumentException: The reference node is not a child of this node.
   at System.Xml.XmlNode.InsertAfter(XmlNode newChild, XmlNode refChild)
   at Microsoft.Build.Construction.ProjectElementContainer.AddToXml(ProjectElement child)
   at Microsoft.Build.Construction.ProjectElementContainer.InsertAfterChild(ProjectElement child, ProjectElement reference)
   at Microsoft.Build.Construction.ProjectElementContainer.DeepClone(ProjectRootElement factory, ProjectElementContainer parent)
   at Microsoft.Build.Construction.ProjectElementContainer.DeepClone(ProjectRootElement factory, ProjectElementContainer parent)
   at Microsoft.Build.Construction.ProjectRootElement.DeepClone()
```

This was because we traverse the ProjectElement tree to find a sibling XML element to insert next to. If an adjacent ProjectElement's XML element has no physical representation, it is unparented and therefore not a good target for manipulations.

Add an implicitness check to the existing is-valid-for-referencing check to cause the next-XML-tree-sibling search to pass over implicit elements.
@rainersigwald
Copy link
Member

Fix is available in packages versioned 15.1.0-preview-000454-01.

@piotrpMSFT

rainersigwald added a commit to rainersigwald/cli that referenced this issue Dec 4, 2016
@rainersigwald
Update to MSBuild 15.1.0-preview-000454-01
6611629 
To pick up a fix for dotnet/msbuild#1431.
@rainersigwald rainersigwald mentioned this issue Dec 4, 2016
Merged
TheRealPiotrP pushed a commit to TheRealPiotrP/cli that referenced this issue Dec 4, 2016
@rainersigwald
Update to MSBuild 15.1.0-preview-000454-01
a8452e5 
To pick up a fix for dotnet/msbuild#1431.
TheRealPiotrP pushed a commit to TheRealPiotrP/cli that referenced this issue Dec 4, 2016
@rainersigwald
Update to MSBuild 15.1.0-preview-000454-01
20f91b5 
To pick up a fix for dotnet/msbuild#1431.
@TheRealPiotrP
Copy link
Contributor Author

Fix implemented in #1432 confirmed. Closing this issue.

TheRealPiotrP pushed a commit to dotnet/cli that referenced this issue Dec 5, 2016
use Sdks attribute in dotnet new templates (#4916)
d6cab4a 
* Move dotnet-new templates to Sdk attribute

* Update to MSBuild 15.1.0-preview-000454-01

To pick up a fix for dotnet/msbuild#1431.

* Fix template newlines

* Fix casing on Microsoft.Net.Sdk

* Move migration test csproj's to Sdk attribute

* Disable parallel sdk restore

Each SDK restore operation will try to manipulate the same assets.json file since the dependency name&version are injected into a common csproj file. This can cause runtime failures when two NuGets try to restore the project at once.

* Make casing of SDK 'NET' and not 'Net'

* Remove redundatn imports

* Fix test string

* Additional race

* Replacing the SDK with the Web.Sdk when it is a Web project.

* Fixing the test by writting the csproj before running the migration rule.
@rainersigwald rainersigwald mentioned this issue Dec 6, 2016
Closed
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rainersigwald rainersigwald

Labels
bug triaged
Projects
None yet
Milestone
Feature: Sdks
Development

No branches or pull requests
3 participants
@TheRealPiotrP @rainersigwald @AR-May