murrayju / CreateProcessAsUser

Creates a process in a different Windows session
MIT License
369 stars 114 forks source link

Integrate CsWin32 for Safer and More Maintainable P/Invoke Interop #38

Open vitkuz573 opened 10 months ago

vitkuz573 commented 10 months ago

Changes

In this Pull Request, I have refactored the existing manual P/Invoke definitions to use the CsWin32 generated APIs. This transition introduces a modern approach that simplifies code maintenance and reduces the risk of errors associated with interop calls.

Why CsWin32?

CsWin32 is a tool provided by Microsoft that allows for safe and efficient Win32 API calls from C#. It automatically generates precise and type-safe P/Invoke signatures, enabling developers to avoid the manual writing of interop code and the potential mistakes that come with it.

Benefits

Impact of Changes

I believe these changes align well with the project's ongoing efforts to adopt modern .NET practices and will contribute to its overall robustness. I look forward to your feedback and any further improvements you may suggest.

Thank you for considering this contribution.

AndrewSav commented 10 months ago

It would be good if we did not have to lock ourselves out of everything that is not C#12. There is value in this being usable for a wider range of .net framework versions.

vitkuz573 commented 10 months ago

From C# 12, I think only the primary constructor was here. I rolled back the commit

murrayju commented 10 months ago

Looks good to me at a glance. Happy to see the manual p/invoke declarations go away, and looks like you threw in some other type improvements too.

I'm not really in a great position to review/test this properly myself. As @AndrewSav pointed out, my only concern would be any backwards compatibility problems. Can someone help test/validate this with older framework/windows versions?

@MV10 your feedback would be welcome here as well.

vitkuz573 commented 10 months ago

I tested on Windows 11 and the DemoModernService (.NET 8) example. In this combination everything works correctly

MV10 commented 10 months ago

My only concern is taking a dependency on beta/preview packages...

  <ItemGroup>
    <PackageReference Include="Microsoft.Windows.CsWin32" Version="0.3.51-beta">
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>
    <PackageReference Include="Microsoft.Windows.SDK.Win32Metadata" Version="56.0.13-preview" />
  </ItemGroup>
vitkuz573 commented 10 months ago

Yeah. I understand the concern. I'm really looking forward to these packages becoming officially stable myself. But according to my observations and extensive and prolonged use in my own remote control application project they are stable enough