-
Notifications
You must be signed in to change notification settings - Fork 6
Fixed PlgxTools building failure and added support for Visual Studio 2017 #6
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
base: master
Are you sure you want to change the base?
Conversation
dlech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I haven't tried using these changes yet, but I think they look OK if they work. I also wonder if there is a more modern way of extending msbuild in nuget packages that might be better.
for example:
| <!-- <xs:include schemaLocation="C:\Program Files (x86)\Microsoft Visual Studio 14.0\Xml\Schemas\1033\Microsoft.Build.xsd" /> --> | ||
|
|
||
| <!-- Support for Visual Studio 2017 Community --> | ||
| <xs:include schemaLocation="C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Xml\Schemas\1033\Microsoft.Build.xsd" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is a variable we could use here instead of requiring a specific version of Visual Studio installed in the default location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point, I did a bit of research beforehand, but I couldn't find anything like that. It is possible to have multiple include elements in it though. For example, line 6 could stay there uncommented but it then it will result in warning messages at compile time as it is not going to be able to find Microsoft Visual Studio 14.0 path if Visual Studio 2015 isn't installed. However, it will compile correctly as long as one of the includes path exists. Another idea: maybe it is possible to create a conditional XSD schema structure to pick the right path based on the Visual Studio version. The problem with it is that the VS version needs to be read from the XSD file somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need this for cross-platform compatibility!
PlgxTools/Program.cs
Outdated
| foreach (XmlNode child in children) { | ||
| var children = itemGroup.ChildNodes.Cast<XmlNode>().Where(child => child.NodeType != XmlNodeType.Comment).ToList(); | ||
|
|
||
| foreach (XmlNode child in children) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent looks wrong on this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed it :)
PlgxTools.exe fails when there are XML comments
<!-- -->into<ItemGroup>sections in the plugin .csproj file. I have applied a little fix to the build process to skip comments.The original solution was not compiling in Visual Studio 2017 due to incapability to locate xsd.exe. Moreover, the initial .csproj setup seemed to have issues passing property group value across targes. Not sure whether it was due to changes with Visual Studio 2017. Anyhow, I have fixed all those little issues.
Please let me know whether the changes look good to you and whether you a new NuGet package with the updates should be released.