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

Use Microsoft.NET.Sdk.WindowsDesktop for XAML projects #40234

Merged
merged 3 commits into from
Feb 5, 2020

Conversation

tmat
Copy link
Member

@tmat tmat commented Dec 7, 2019

Switches projects that depend on WPF to Microsoft.NET.Sdk.WindowsDesktop SDK.

Enables building Roslyn.sln x-plat via dotnet build.

Note: dotnet pack will also work but not produce any VSIXes, as VSIXes can only be built using desktop msbuild/VS.

@tmat tmat force-pushed the WindowsDesktopSdk branch from 6ac6863 to eda0157 Compare December 7, 2019 22:09
@tmat
Copy link
Member Author

tmat commented Dec 7, 2019

@nguerrera Seems to work fine :)

@nguerrera
Copy link
Contributor

Nice. @vatsan-madhavan fyi

@tmat
Copy link
Member Author

tmat commented Dec 8, 2019

@vatsan-madhavan What is GeneratedInternalTypeHelper? Seems to be sometimes generated sometimes not.

@vatsan-madhavan
Copy link
Member

What is GeneratedInternalTypeHelper? Seems to be sometimes generated sometimes not.

GeneratedInternalTypeHelper.g.cs is generated by MarkupCompilePass1, and derives from InternalTypeHelper.

When compiling a XAML file, you can use public types, but you can also use internal types subject to the same limitations that exist on code access to internal types. InternalTypeHelper enables support of internal access level types for markup. This involves the compiler creating a generated class that derives from InternalTypeHelper and implements its members. The generated class exists in a security and access context such that only the same assembly or other assemblies specifically attributed for shared internal access can reference the generated class and thus the internal types.

/cc @rladuca, @ryalanms

@tmat
Copy link
Member Author

tmat commented Dec 9, 2019

@vatsan-madhavan It seems that this file is not always generated, which is a problem. We need the class to be either always generated or never. The reason is that the class is public and thus becomes part of public surface of the assembly. We have an analyzer that validates that public surface has not changed accidentally and it does so by checking against a baseline. If we include the type in public API baseline but the file is not always generated then we get build errors and vice versa.

@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented Dec 9, 2019

It seems that this file is not always generated

Depends on what one means by that.

(a) If a project generates GeneratedInternalTypeHelper.g.cs only sporadically without any corresponding changes in its sources, then that represents a likely bug in WPF's markup compiler.

(b) If GeneratedInternalTypeHelper.g.cs comes and goes as the project itself changes, that may very well be by design. XamlGeneratedNamespace.GeneratedInternalTypeHelper is generated only when WPF needs it. Typically, this happens when an internal type is referenced in the XAML, but the precise logic is a bit more involved and I can't give a full recounting of it ad lib.

(c) If GeneratedInternalTypeHelper.g.cs is present for some projects but absent for others, that's understandable, and also by design. This is similar to (b).

In general, I believe that it would be safe for public-API baseline analyzers to ignore XamlGeneratedNamespace namespace. This is because (b) and (c) are by-design and represent correct behavior; (a) represents a build-breaking bug - the build wouldn't normally reach the analyzer stage if GeneratedInterTypeHelper.g.cs were truly required but nevertheless absent due to a bug.

@tmat
Copy link
Member Author

tmat commented Dec 11, 2019

@vatsan-madhavan I think this is case (a). When I build the project locally and watch the obj directory this is what happens.
At some point GeneratedInternalTypeHelper.g.cs is created with a declaration of class GeneratedInternalTypeHelper and a few moments later it is replaced with an empty file. Seems like a bug. On CI the content of the file is not erased for some reason.

@tmat
Copy link
Member Author

tmat commented Dec 11, 2019

I can see .NET Framework targets imported:
image

Should they be? I thought by switching to WindowsDesktop SDK we remove the dependency on .NET Framework build targets.

@vatsan-madhavan
Copy link
Member

You might be seeing dotnet/msbuild#4948. Are you seeing this under any under conditions besides (a) build engine is msbuild/VS and (b) TFM is netfx?

@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented Dec 11, 2019

I thought by switching to WindowsDesktop SDK we remove the dependency on .NET Framework build targets.

That is indeed correct. Even when those get imported, WindowsDesktop SDK's targets will supersede _as long as the project is a Microsoft.NET.Sdk.WindowsDesktop project - but you'll need to be on 3.1.100+ SDK.

@tmat
Copy link
Member Author

tmat commented Dec 11, 2019

Are you seeing this under any under conditions besides (a) build engine is msbuild/VS and (b) TFM is netfx?

Yes, the same behavior (the file being generated and then its content erased) when building using msbuild and dotnet build (3.1.100 SDK).

The .NET Framework targets are only imported when using msbuild.

@vatsan-madhavan
Copy link
Member

Yes, the same behavior when building using msbuild and dotnet build (3.1.100 SDK).

We'd be interested in seeing the logs for the dotnet build case in the dotnet/msbuild#4948 discussion. I don't think I've seen it under dotnet build.

@tmat
Copy link
Member Author

tmat commented Dec 12, 2019

@vatsan-madhavan Sent the log via email.

@vatsan-madhavan
Copy link
Member

Yes, the same behavior (the file being generated and then its content erased) when building using msbuild and dotnet build (3.1.100 SDK).

Depending on the code/xaml being compiled, GeneratedInternalTypeHelper could be generated in MarkupCompilePass1 and subsequently removed in MarkupCompilePass2.

That doesn’t tell us why you might be seeing inconsistency across builds. For now my advice is to ignore XamlGeneratedNamespace from api-compat checks.

I’ll look into your binlog again in Jan when I’m back from vacation to see if it has any clues. If you have a small-ish project that can reproduce the problem consistently it would be helpful in debugging.

@tmat
Copy link
Member Author

tmat commented Dec 13, 2019

GeneratedInternalTypeHelper could be generated in MarkupCompilePass1 and subsequently removed in MarkupCompilePass2.

This is definitely not something I'd expect a build task to do - to update content of a file generated by other task.

If you have a small-ish project that can reproduce the problem consistently

I don't have a small project, but you can sync this PR branch and run

>C:\Roslyn\src\VisualStudio\Core\Def>dotnet build /bl:log.binlog /p:MSBuildTargetsVerbose=true

@tmat
Copy link
Member Author

tmat commented Jan 7, 2020

@vatsan-madhavan Ping.

@vatsan-madhavan
Copy link
Member

@tmat Would you mind opening an issue for this on https://github.com/dotnet/wpf so that we could look into this ? Unfortunately I haven't had the time to dig into this further since I was out sick for a few days I'm now trying to catch up on a bunch of work...

@tmat tmat force-pushed the WindowsDesktopSdk branch 2 times, most recently from 365477b to 2c56a28 Compare January 20, 2020 23:54
@tmat
Copy link
Member Author

tmat commented Jan 21, 2020

Actually, there is a difference in behavior between 3.0.100 and 3.1.100. Seems like if we update to 3.1.100 it works.

@vatsan-madhavan
Copy link
Member

Actually, there is a difference in behavior between 3.0.100 and 3.1.100. Seems like if we update to 3.1.100 it works.

Only thing I can think of that explains it is a combination of dotnet/wpf#2004 and dotnet/wpf#2075.

Glad to hear that this got better! 👍 😃 👍

@tmat tmat closed this Jan 27, 2020
@tmat tmat reopened this Jan 27, 2020
@tmat tmat closed this Jan 30, 2020
@tmat tmat reopened this Jan 30, 2020
@tmat tmat force-pushed the WindowsDesktopSdk branch 2 times, most recently from 4270e0e to b9b99eb Compare January 31, 2020 22:31
@tmat tmat marked this pull request as ready for review February 3, 2020 21:27
@tmat tmat requested review from a team as code owners February 3, 2020 21:27
@tmat
Copy link
Member Author

tmat commented Feb 3, 2020

@jaredpar PTAL

@tmat
Copy link
Member Author

tmat commented Feb 3, 2020

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Looks good, let's merge this after our 16.5 snap to avoid any surprises there unless we need it first.

@tmat tmat force-pushed the WindowsDesktopSdk branch from b9b99eb to 302456b Compare February 4, 2020 23:00
@tmat tmat merged commit 48b5e93 into dotnet:master Feb 5, 2020
@tmat tmat deleted the WindowsDesktopSdk branch February 5, 2020 17:32
333fred added a commit to 333fred/roslyn that referenced this pull request Feb 6, 2020
* dotnet/master: (918 commits)
  Pure null test on unconstrained type (dotnet#41384)
  Fix file headers, bootsrap build issues.
  Make `elementLocations` accept an array of nullable locations in the public api.
  Learn from non-null tests on as operator (dotnet#41419)
  Use Microsoft.NET.Sdk.WindowsDesktop for XAML projects (dotnet#40234)
  Address feedback.
  Introduce `GetRequiredBinder`.
  Add missing `NotNullWhen` annotations.
  Annotate more of the binder
  Add version of zlib library to deterministic build inputs (dotnet#41385)
  [master] Update dependencies from dotnet/arcade (dotnet#41354)
  Simplify
  Simplify
  Do not simplify interpolation expressions on implicit receivers.
  Fix local function crash + add tests
  Substituted symbol equality (dotnet#41123)
  Fix test failures
  Rename from IncludeNonNullableReferenceTypeModifier to IncludeNotNullableReferenceTypeModifier (dotnet#41332)
  Set TEMP environment variable to $TempDir on CI machines (dotnet#41361)
  Document placeholders
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants