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

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

Closed
marknn3 opened this issue Oct 21, 2020 · 27 comments · Fixed by #4149
Closed
Assignees
Labels
area-Theming 💥 regression-preview Regression from a preview release

Comments

@marknn3
Copy link

marknn3 commented Oct 21, 2020

  • .NET Core Version:
    5.0.100-rc.2.20479.15

  • Have you experienced this same bug with .NET Framework?:
    No

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:

  • Target Framework: net5.0-windows
  • Deployment mode: Self-contained
  • Target Runtime: win-x64
  • File publish options: Produce single file
@JeremyKuhne
Copy link
Member

Here is the relevant docs entry:

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

Some of the related issues.

dotnet/runtime#40087
dotnet/runtime#38405

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

@JeremyKuhne JeremyKuhne added this to the 6.0 milestone Oct 21, 2020
@JeremyKuhne JeremyKuhne added the 💥 regression-preview Regression from a preview release label Oct 21, 2020
@JeremyKuhne
Copy link
Member

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

@JeremyKuhne
Copy link
Member

Code in question:

string assemblyLoc = typeof(Application).Assembly.Location;

@merriemcgaw merriemcgaw modified the milestones: 6.0, 6.0 Preview1 Oct 21, 2020
@marknn3
Copy link
Author

marknn3 commented Oct 21, 2020

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor

paul1956 commented Oct 22, 2020

@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
Copy link
Author

marknn3 commented Oct 22, 2020

@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
Copy link
Member

agocke commented Oct 22, 2020

@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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Member

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

  • We want WinForms apps to enable the common controls themes even if the app's manifest doesn't have this in it
  • The Windows API can only do this from a Win32 resource accessible via HMODULE (and Win32 resource management APIs) - it can't do this from a piece of memory

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:

  • Modify the SDK to recognize this situation and copy the native resource from the WinForms assembly to the .exe. - we already have code to copy native resources from the main app.dll to the .exe, so this is a relatively simple change.
  • Ideally we would also rename the resource to make its name unique to avoid potential collisions.
  • WinForms would detect single-file and access the resource on the .exe instead

@agocke
Copy link
Member

agocke commented Oct 22, 2020

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

agocke commented Oct 22, 2020

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

weltkante commented Oct 22, 2020

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
Copy link
Contributor

@JeremyKuhne the difference I am using ClickOnce

@vitek-karas
Copy link
Member

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.

@RussKie RussKie modified the milestones: 6.0 Preview1, 5.0.1 Oct 23, 2020
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Oct 26, 2020
@ghost ghost added the 🚧 work in progress Work that is current in progress label Oct 30, 2020
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Nov 10, 2020
@RussKie RussKie removed this from the 5.0.1 milestone Nov 10, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Theming 💥 regression-preview Regression from a preview release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants