dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.35k stars 962 forks source link

Visual Styles not enabled when app published as 'Self-contained' + 'Produce Single File' #4145

Closed marknn3 closed 3 years ago

marknn3 commented 3 years ago

Problem description:

Application.EnableVisualStyles() does not enable Visual Styles when the application is published as 'Self-contained' + 'Produce Single File'.

The probable root cause is that typeof(Application).Assembly.Location returns an empty string for this scenario on .NET 5. Note that on .NET Core 3.1 it returns a temporary file location.

Minimal repro: Create simplest WinForms .NET 5 App. In Form1 ctor add following sample code to show the result: Text = "UseVisualStyles=" + Application.UseVisualStyles.ToString() + "; " + typeof(Application).Assembly.Location;

Publish app:

JeremyKuhne commented 3 years ago

Here is the relevant docs entry:

https://docs.microsoft.com/en-us/dotnet/core/deploying/single-file#api-incompatibility

Some of the related issues.

https://github.com/dotnet/runtime/issues/40087 https://github.com/dotnet/runtime/issues/38405

Need to look for any of these problematic APIs in the code base.

JeremyKuhne commented 3 years ago

Presumably using the manifest to enable styles (comctl6) should work in single file, should validate that this is the case.

JeremyKuhne commented 3 years ago

Code in question: https://github.com/dotnet/winforms/blob/02089e92406b3f508c05060ee0ee7857a8ba3c11/src/System.Windows.Forms/src/System/Windows/Forms/Application.cs#L823

marknn3 commented 3 years ago

The only way to create the required ActivationContext to enable Visual Styles is via System.Windows.Forms.ThemingScope.CreateActivationContext which is on an internal class. And since reflection is not possible on single file, I see no workaround.

JeremyKuhne commented 3 years ago

@marknn3 The only practical workaround is to add a manifest file to your project and enable the 6.0 common controls section. Uncomment this section after adding a manifest item to your project:

  <!-- Enable themes for Windows common controls and dialogs (Windows XP and later) -->
  <dependency>
    <dependentAssembly>
      <assemblyIdentity
          type="win32"
          name="Microsoft.Windows.Common-Controls"
          version="6.0.0.0"
          processorArchitecture="*"
          publicKeyToken="6595b64144ccf1df"
          language="*"
        />
    </dependentAssembly>
  </dependency>

I'm investigating a possible fix.

paul1956 commented 3 years ago

When I publish 'Self-contained' + 'Produce Single File' the manifest is included and the code above is already there and not commented out. The publish still fails with

Could not find file 'Microsoft.Windows.Common-Controls, Version=6.0.0.0, Culture=*, PublicKeyToken=6595b64144ccf1df, ProcessorArchitecture=*, Type=win32'.

I am publishing a 64 bit app

JeremyKuhne commented 3 years ago

@paul1956 I don't see that error. I created a new 5.0 WinForms application, added a button to the form to see the styles, and added an "app.manifest" and uncommented as above. Publishing as follows:

image

JeremyKuhne commented 3 years ago

@agocke I've run into a bit of a wall here. I can't seem to get the HMODULE for the embedded .NET assemblies. Listing everything in the current process using EnumProcessModules doesn't show any .NET assemblies. Any ideas?

Note that CreateActCtxW does work with an HMODULE ref instead of a path outside of the single file scenario.

paul1956 commented 3 years ago

@JeremyKuhne not sure what the difference is, except mine is a real project. I get the error above and my project already had a manifest from framework and the XML above was already uncommented. image The file pointed to has no useful information except to look at the Build Log where the error shows up. Also this worked until recently.

marknn3 commented 3 years ago

@marknn3 The only practical workaround is to add a manifest file to your project and enable the 6.0 common controls section. Uncomment this section after adding a manifest item to your project: <!-- Enable themes for Windows common controls and dialogs (Windows XP and later) --> ...

This does indeed re-enable Visual Styles! And for me this workaround is completely acceptable.

But since this is a regression it must indeed be fixed. Might not be trivial.

Thanks for the quick response!

agocke commented 3 years ago

@JeremyKuhne Have you already loaded the assembly? Either by Assembly.Load or running code inside that assembly? I'm not sure what you mean by "can't get an HMODULE. Do you at least have a valid Assembly object for the target assembly?

cc @vitek-karas. Context is trying to get an HMODULE handle from an embedded assembly and pass that to a helper, to avoid dependency on Assembly.Location

JeremyKuhne commented 3 years ago

@paul1956 Your issue should be tracked separately. Can you open a new issue with detailed repro steps? Note that I am currently:

  1. Using VS 16.8.0 Preview 5.0
  2. Using 5.0.100-rc.2.20479.15 for .NET (use dotnet --list-sdks to see what you have installed)
  3. Publishing to a folder, not Click Once

As much detail as you can put in the new issue would be great. Please try to minimize the repro as well. This will help us reproduce the error.

JeremyKuhne commented 3 years ago

@agocke Yes, the assembly is loaded already. It shows in the assemblies debugger window and the code that is trying to get the HMODULE is actually running from the assembly I'm trying to get.

CreateActCtx is the API we have to use to "inject" the manifest. It requires a manifest file or a native resource from a PE image. The fix I'm attempting is to use hModule instead of lpSource.

I'll try and dump the manifest file to the temp folder to see if that works, but that is something I'd rather not do if possible.

vitek-karas commented 3 years ago

I've looking into this couple of weeks ago and this will need a different solution. My understanding of the problem:

The core issue here is that in .NET 5 Single-File none of the assemblies loaded directly from the bundle have an HMODULE - we map them into memory basically like a random piece of data - we don't go through LoadLibrary (can't, Windows can't load library from an offset in a file).

Since there's really only one true file (and thus also only one true HMODULE) - the bundle itself, the app.exe - we need to work with that. One possible solution would be to:

agocke commented 3 years ago

Ah, I didn't know that there was only one HMODULE for embedded assemblies. We should consider documenting this at https://docs.microsoft.com/en-us/dotnet/core/deploying/single-file

JeremyKuhne commented 3 years ago

@vitek-karas Your solution seems plausible and likely would end up useful in other scenarios too. What is the the prescribed way to detect that we're in single file mode?

vitek-karas commented 3 years ago

It's a windows limitation (as far as I know) - HMODULE == something loaded via LoadLibrary (or similar).

Detect single-file - for this case checking typeof(Application).Assembly.Location == string.Empty should do - the definition is that Location is empty string if the assembly was loaded from memory (which is more or less what single-file does), but would also happen for things like loading assembly from a byte[] and so on.

We intentionally don't have a specific API to detect single-file - we kind of hope that people will be able to write code which works in either case. Unfortunately native Win32 resources are... weird.

vitek-karas commented 3 years ago

We could/should take this a step further and probably have a more general solution for Win32 resources - SDK could pretty easily go over all assemblies which will be bundled and extract native resources and add them to the .exe. The problem will be collisions - but maybe something along those lines. Or have an item metadata which would tell SDK to do this. Just like we have one to exclude it from the bundle, we could have CopyWin32ResourcesToBundle (better name obviously).

JeremyKuhne commented 3 years ago

Or have an item metadata which would tell SDK to do this. Just like we have one to exclude it from the bundle, we could have CopyWin32ResourcesToBundle (better name obviously).

I think that may be the better choice, we would need to know what the identifier would be in code and I don't know how we would do that if there was auto collision renaming.

vitek-karas commented 3 years ago

I wasn't thinking about auto-rename - that's just asking for trouble. Either it would work, or fail. But I agree it's better to have this opt-in. The managed code will have to react to this anyway, since the way to get the HMODULE/FileName is different for single-file. So the code owner must be aware of this and thus it's better to make it opt-in.

JeremyKuhne commented 3 years ago

@vitek-karas I'm going to try the temp folder solution to see if that works so we can know what our options are. This is something we're going to want to service as default WinForms projects don't work properly with single file.

agocke commented 3 years ago

Reminder that we have a back-compat flag for extraction IncludeAllContentForSelfExtract=true that may be a good alternative if this is too risky as a servicing fix.

JeremyKuhne commented 3 years ago

Creating a temporary manifest in the Temp directory works, I've created a PR #4149 for master (6.0) that does this. For servicing 5.0 this may be what we want to do as I presume adding infra for native resources is out of scope for a servicing fix.

I'll get our testers to specifically look at the single file publish scenario and scour the code for usage of other impacted APIs. https://docs.microsoft.com/en-us/dotnet/core/deploying/single-file#api-incompatibility

vitek-karas commented 3 years ago

I think it's worth considering what @agocke mentions above. Using the IncludeAllContentForSelfExtract=true the single-file will do basically that - it writes the app into a temp and runs it from there. Having just the manifest written out is obviously much faster, but in terms of risk, we already have a workaround, so a necessity of a fix in servicing might be somewhat lower.

weltkante commented 3 years ago

Note that for native resource manifests the WinForms manifest is not the same as the manifest the main application may carry by itself. You may very well have a scenario where the main application has a manifest not declaring v6 support and relying on using the WinForms API call to enable v6 support. Propagating the manifest from the WinForms assembly into the single file application will conflict the native resource ID in a way not resolvable unless WinForms understands that scenario and looks at a different resource index for single file scenarios.

We could/should take this a step further and probably have a more general solution for Win32 resources - SDK could pretty easily go over all assemblies which will be bundled and extract native resources and add them to the .exe

Aren't native resources exclusively accessed via native APIs? That means the calling code will fail anyways, just like WinForms failed. Not much point remapping native resources if you can't access them without being specially coded for it (and at that point you probably should use something else than native resources).

I'd propagate the native resources only of the main application and emit warnings for any embedded libraries that have custom native resources, because its very likely those libraries won't work if they try to access their resources.

paul1956 commented 3 years ago

@JeremyKuhne the difference I am using ClickOnce

vitek-karas commented 3 years ago

WinForms code can use any resource ID, so the idea is to put the WinForms manifest into the .exe under some unique name/ID definitely not under "1". The original app's manifest should still be the "1".

I do agree that code wanting to consume native resource will have to be aware of single-file and as noted in such case it would be ideal if it changed to not use native resource. But given this example it might not always be possible. So having an opt-in mechanism seems reasonable.