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

WPF support #27

Merged
merged 17 commits into from
Dec 12, 2017
Merged

WPF support #27

merged 17 commits into from
Dec 12, 2017

Conversation

wjk
Copy link
Contributor

@wjk wjk commented Dec 8, 2017

This PR adds support for WPF-based projects to the new, SDK-based project system, including full designer support. It should support C#, F#, and VB.NET projects (although I have only tested it with C#).

Note that for each project that wants to include WPF code, the following code must be placed into its project file:

<PropertyGroup>
  <EnableWpfTargets>true</EnableWpfTargets>
</PropertyGroup>

This is because I didn't want the WPF targets being included where they are not needed, because they are fairly invasive. If this code is not added, the WPF targets will not be included (and weird build breakage will likely result).

This PR supersedes #20, which should be closed once this is merged. Thanks!

sharwell and others added 9 commits September 5, 2017 09:18
A WPF-based application now compiles, runs, and is fully debuggable.
By making projects opt-into the WPF support code, they can avoid having
their build processes modified for WPF compatibiltiy if they don't need it.
The SDK already includes these projects, and more, if targeting full .NET Framework.
I didn't catch this one in the last commit.
@wjk
Copy link
Contributor Author

wjk commented Dec 8, 2017

Note that you’ll have to add **/*.xml to the <None /> line in the csproj, or else the MSBuild rules files won‘t get included in the NuGet package, and weird failures will result. I’d do this myself, but it’s getting late. 😄 I can either add that code tomorrow, or you can do it yourself once this is merged. Thanks!

@davkean
Copy link

davkean commented Dec 8, 2017

Can we contribute the built-in item rules to http://github.com/dotnet/project-system instead of adding them to this project? Rules should only be added for items that you own yourself. I will happily take the contribution.

I would definitely recommend you run this targets file under the new evaluation profiler: dotnet/msbuild#2765. Globs and updates are not free - and this adds a bunch. Slow evaluation leads to increase project load time, larger design-time builds for those projects that reference you (including .NET Framework based on the legacy project system), which can lead to UI delays and slow project manipulation.

Copy link
Collaborator

@clairernovotny clairernovotny left a comment

Choose a reason for hiding this comment

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

Can you please also add the *.XAML None include to ensure it gets all packed correctly?


<PropertyGroup>
<!-- Workaround for lack of XAML support in the new project system -->
<LanguageTargets>$(MSBuildExtensionsPath)\$(VisualStudioVersion)\Bin\Microsoft.$(_SdkLanguageName).targets</LanguageTargets>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any differences in the file between the VS location and the SDK location. It's likely that the VS location just includes "more."

Given that projects built with these targets aren't supported by dotnet build (they require msbuild.exe, I think it's safe to always include these WPF additions. no need for a conditional import.

<Project>
<PropertyGroup>
<MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
<EnableWpfTargets Condition="'$(EnableWpfTargets)' == ''">false</EnableWpfTargets>
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment below. I think it's safe to always import these.

</PropertyGroup>

<ItemGroup Condition="'$(EnableDefaultItems)' == 'True'">
<None Remove="**/*.xaml" Condition="'$(EnableDefaultWpfItems)' == 'True'" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

</ItemGroup>

<ItemGroup>
<Page Update="@(Page)" SubType="Designer" Generator="MSBuild:Compile" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do this without an update? Just put this on the Page include above?

</ItemGroup>

<ItemGroup>
<PropertyPageSchema Include="$(MSBuildThisFileDirectory)Rules/Wpf.ProjectItemsSchema.xml">
Copy link
Collaborator

Choose a reason for hiding this comment

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

@davkean I'm going to accept this here for now. I really wish there was a VS Version property passed into the design time build that had the full version info. Closest I see now is 15.0, which isn't good enough.

I want to get these in ASAP but would prefer to condition them on specific builds of VS so we could use the in-box ones.

@wjk
Copy link
Contributor Author

wjk commented Dec 8, 2017

@davkean How should I run the MSBuild profiler, and how should I interpret the results? This is my first time using this feature. Thanks!

wjk and others added 5 commits December 11, 2017 19:24
These files won't be deleted, either automatically by VS or during a clean.
Therefore, they will clutter the obj directory more and more as times goes on.
@clairernovotny clairernovotny merged commit fa415e0 into novotnyllc:master Dec 12, 2017
@clairernovotny
Copy link
Collaborator

@wjk @sharwell I'm seeing errors while trying to use this as a package.

https://github.com/onovotny/MSBuildSdkExtras/tree/wpf-sample

It seems to work in the test projects when they're directly included. Any ideas?

@clairernovotny
Copy link
Collaborator

There's a start of workaround that I added to the branch, but I still get this:

1>Target GenerateTemporaryTargetAssembly:
1>  Target CoreCompile:
1>    Using shared compilation with compiler from directory: C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Roslyn
1>    MainWindow.xaml.cs(25,13,25,32): error CS0103: The name 'InitializeComponent' does not exist in the current context
1>  Done building target "CoreCompile" in project "jyurx4g3.tmp_proj" -- FAILED.

From what I can tell looking at the binlog, it's not generating the file for the XAML files.

@wjk
Copy link
Contributor Author

wjk commented Dec 12, 2017

@onovotny Yeah, that's what I'm seeing as well. I'm not seeing the MainWindow.g.cs file anywhere. It is generating the App.g.cs file, though. How weird.

@clairernovotny
Copy link
Collaborator

Wait, nm, I had an error...was missing the x:Class.

@clairernovotny
Copy link
Collaborator

Still have a nasty work around for NuGet/Home#5894 :(

@wjk
Copy link
Contributor Author

wjk commented Dec 12, 2017

Yep, that was it re the MainWindow.g.cs issue. Will submit a second PR re the NuGet bug using material from dotnet/msbuild#2701. Thanks!

@wjk wjk deleted the wpf branch December 12, 2017 21:19
@wjk
Copy link
Contributor Author

wjk commented Dec 12, 2017

Actually, as it turns out, the linked MSBuild issue depends on another PR (NuGet/NuGet.Client#1786), which may take a while to merge because of licensing issues (they say it contains some code that hasn't gotten the official OK from MS Legal). Nevermind my previous comment, as it wouldn't have worked.

@sharwell
Copy link
Contributor

sharwell commented Feb 5, 2018

Still have a nasty work around for NuGet/Home#5894 :(

Can you point out the hack in code?

@clairernovotny
Copy link
Collaborator

@sharwell It's because PackageReference for WPF projects is currently broken in general due to the two-pass compile and the tmp_proj file extension it uses.

@mungojam
Copy link

mungojam commented Feb 7, 2018

I tried using the extras package plus the workaround yesterday and got 2 errors:

  1. It couldn't find a main method in the WPF project
  2. It couldn't resolve the InitializeComponent method.

That was after using the https://github.com/hvanbakel/CsprojToVs2017 project converter

For now I've reverted to using the old style project. Let me know if you think it should work and I can try again and provide more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants