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

Start using CsWin32 #7445

Closed
elachlan opened this issue Jul 19, 2022 · 21 comments
Closed

Start using CsWin32 #7445

elachlan opened this issue Jul 19, 2022 · 21 comments
Assignees
Milestone

Comments

@elachlan
Copy link
Contributor

elachlan commented Jul 19, 2022

Is your feature request related to a problem? Please describe

This issue is to be used to help centralize information around the change to implement CsWin32.

Blockers:
microsoft/CsWin32#639
microsoft/CsWin32#605
microsoft/CsWin32#600
microsoft/CsWin32#644
microsoft/CsWin32#624
microsoft/CsWin32#625
microsoft/CsWin32#324
dotnet/runtime#59013

Performance:
microsoft/CsWin32#595

Useful:
microsoft/CsWin32#596
microsoft/CsWin32#606
microsoft/CsWin32#602
microsoft/CsWin32#610
microsoft/CsWin32#122
microsoft/CsWin32#626

Associated Issues/Discussions:
#4751
#7392
#7444
#7446
#7468

@elachlan elachlan added api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation untriaged The team needs to look at this issue in the next triage labels Jul 19, 2022
@elachlan
Copy link
Contributor Author

@JeremyKuhne I have created an issue here to help centralize issues/information around CSWin32.

Before anything gets too deep, the Winforms team should decide on how they want to split up the NativeMethods.txt file.

@JeremyKuhne
Copy link
Member

Before anything gets too deep, the Winforms team should decide on how they want to split up the NativeMethods.txt file.

Not sure how we'd want to split this up in ways that make sense given everything going to one class. Any ideas are certainly welcome.

Note that this is currently a prototype effort targeting .NET 8. We have an intern working on this (@fernandanavamoya) with support from myself and our returning @lonitra.

@elachlan
Copy link
Contributor Author

I have asked the CsWin32 team about the NativeMethods.txt in microsoft/CsWin32#606.

AFAIK, each NativeMethods.txt file needs to be in its own folder and added to additionalfiles. So in the context of winforms, we can place a NativeMethods.txt in each library sub folder. That way each library has its own NativeMethods.txt.

Another way is have a single file, but add a comment "header" for each separate library.

I like the idea of separate files as the management is per library.

Its even more simple if winforms is using every method in a library, we can add them via a single wildcard line. Although, I am unsure if Trimming could then remove the used methods afterwards.

@elachlan
Copy link
Contributor Author

@JeremyKuhne do you want all the generated code in one file? If not, you can use the following in NativeMethods.json

{
  "$schema": "https://aka.ms/CsWin32.schema.json",
  "emitSingleFile": false
}

@RussKie
Copy link
Member

RussKie commented Jul 19, 2022

Personal opinion: I like how we have separation per containing dll (i.e., Interop.User32, Interop.Gdi32, etc). It'd be great to retain the separation if possible, e.g., /User32/NativeMethods.txt, /Gdi32/NativeMethods.txt, /Kernel32/NativeMethods.txt, etc.

@RussKie
Copy link
Member

RussKie commented Jul 19, 2022

AFAIK, each NativeMethods.txt file needs to be in its own folder and added to additionalfiles. So in the context of winforms, we can place a NativeMethods.txt in each library sub folder. That way each library has its own NativeMethods.txt.

👍

@RussKie RussKie removed the untriaged The team needs to look at this issue in the next triage label Jul 19, 2022
@RussKie RussKie added this to the Future milestone Jul 19, 2022
@elachlan
Copy link
Contributor Author

Personal opinion: I like how we have separation per containing dll (i.e., Interop.User32, Interop.Gdi32, etc). It'd be great to retain the separation if possible, e.g., /User32/NativeMethods.txt, /Gdi32/NativeMethods.txt, /Kernel32/NativeMethods.txt, etc.

I'll proceed on this basis. It seems easier to deal with.

This was referenced Jul 19, 2022
@elachlan
Copy link
Contributor Author

So @AArnott suggests we keep a centralized NativeMethods.txt and not split it up. The splitting up was designed for shared project scenarios. Which makes sense.

I will adjust my PR to move it back to the main NativeMethods.txt as this will allow better management.

@elachlan
Copy link
Contributor Author

elachlan commented Jul 19, 2022

I compiled the project with the whole list of methods I put together, without any code changes. I get 4 of these warnings:
warning PInvoke005: This API is only available when targeting a specific CPU architecture. AnyCPU cannot generate this API.

The Associated APIs:

  • GetClassLongPtrW
  • GetWindowLongPtrW
  • SetClassLongPtrW
  • SetWindowLongPtrW

I believe in these cases, we have just checked either IntPtr.Size = 4 or !Environment.Is64BitProcess, then call the associated function.

public static IntPtr SetWindowLong(IntPtr hWnd, GWL nIndex, nint dwNewLong)
{
  if (!Environment.Is64BitProcess)
  {
	  return SetWindowLongW(hWnd, nIndex, dwNewLong);
  }

  return SetWindowLongPtrW(hWnd, nIndex, dwNewLong);
}

@AArnott do you have a suggestion here? Is there an escape hatch available?

Edit: I guess we just remove them from generation by CSWin32 and manage our own PInvoke? which isn't ideal.

@JeremyKuhne
Copy link
Member

@elachlan I appreciate that you want to help here and do want your help, but I'd ask that you hold off for a week or so while we get things spun up on our end. I'll ping you when we're ready if that is ok.

@elachlan
Copy link
Contributor Author

Not a problem. I have been mostly probing for issues at this stage. So hopefully that helps in establishing everything. I will hold off on any additional work for now.

@elachlan
Copy link
Contributor Author

Here is a link to a discussion around the the 32bit/64bit based APIs.
microsoft/CsWin32#592
microsoft/CsWin32#528

@RussKie
Copy link
Member

RussKie commented Jul 20, 2022

@AArnott if I read it correctly, the CsWin32 guideline is to commit to a specific bitness, which may work for a end-user app but doesn't work for for an SDK (our case). Windows Forms and WPF binaries are compiled as AnyCPU and flow through to Windows Desktop, where they get cross-compiled for the target bitness.

We also have situations where struct packing is different between bitness, e.g.:

// Any change in PRINTDLG_32, should also be in PRINTDLG and PRINTDLG_64
// x86 requires EXPLICIT packing of 1.

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode, Pack = 1)]
public unsafe struct PRINTDLGW_32 : PRINTDLGW

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
public unsafe struct PRINTDLGW_64 : PRINTDLGW

@AArnott
Copy link
Contributor

AArnott commented Jul 20, 2022

do you want all the generated code in one file? If not, you can use the following in NativeMethods.json

@elachlan The default is not one file, so you don't need this setting to turn it off. Only to opt in to single file generation.

warning PInvoke005: This API is only available when targeting a specific CPU architecture. AnyCPU cannot generate this API.

@elachlan This has come up a few times. I would recommend that WinForms actually target specific CPU architectures, at least for its runtime assemblies. That will give it access to the broadest Win32 API surface area. While sometimes you can do quick workarounds with a runtime pointer-size check, that isn't reliable generally, since structs can sometimes be declared very differently across architectures.
For ref assemblies, AnyCPU assemblies is really important so customers can continue to build AnyCPU WinForms apps.

doesn't work for for an SDK (our case). Windows Forms and WPF binaries are compiled as AnyCPU and flow through to Windows Desktop, where they get cross-compiled for the target bitness.

@RussKie as you point out, structs themselves vary by bitness. It's not always as simple as a Pack parameter. And while you can sometimes get away with declaring two structs, and doing your own runtime check to know which to use, sometimes the struct in question is deep in a type hierarchy where you'd have to define several more (otherwise AnyCPU) structs twice in order to enable the runtime check. This process is error prone and tedious to code around. It's much easier to just target multiple architectures. As I mention above, you can still compile AnyCPU ref assemblies for your SDK. And as you mention, eventually your runtime crossgen's the runtime assemblies into being arch-specific anyway, so it's not really that you're AnyCPU.

@RussKie
Copy link
Member

RussKie commented Jul 21, 2022

We don't package or release directly, and our binaries follow an established flow through several layers (dotnet/winforms->dotnet/wpf->dotnet/windowsdesktop->...). So we can't target specific bitness, as it would require us to completely reengineer all our build, test, packaging and release pipelines. This is simply unattainable.

@AArnott
Copy link
Contributor

AArnott commented Jul 21, 2022

Ok. Then you'll have to keep the arch-specific APIs hand-authored.

@merriemcgaw merriemcgaw modified the milestones: Future, .NET 8.0 Aug 10, 2022
ghost pushed a commit that referenced this issue Aug 26, 2022
Remove unused enums. Found them in my manual search through User32.

Related: #7445
RussKie pushed a commit that referenced this issue Sep 8, 2022
Remove unused enums. Found them in my manual search through User32.

Related: #7445
lonitra added a commit to lonitra/winforms that referenced this issue Sep 23, 2022
Remove unused enums. Found them in my manual search through User32.

Related: dotnet#7445
RussKie pushed a commit that referenced this issue Sep 23, 2022
Remove unused enums. Found them in my manual search through User32.

Related: #7445
@merriemcgaw
Copy link
Member

I'm officially going to tag this for .NET 9, for whatever bits remain after .NET 8. I think we're past where we want to do more in this release, but let's keep going! 😄

@willibrandon
Copy link
Contributor

Is the plan to convert the remaining Ole32 functions and Shell32 interfaces to CsWin32?

@elachlan
Copy link
Contributor Author

elachlan commented Dec 6, 2023

@willibrandon it's been slowly happening. I think it's a lot of com wrapper stuff left.

@willibrandon
Copy link
Contributor

willibrandon commented Dec 7, 2023

@elachlan - I'm curious what that will look like, and will keep an eye out, thanks.

@merriemcgaw merriemcgaw removed the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Jan 10, 2024
@JeremyKuhne
Copy link
Member

Everything has been moved now.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants