SDK targets rework

[andrew-boyarshin]

• Merge SharpPatch into SharpGenTools.Sdk
• Merge SharpGen tasks into a single one (SharpGenTask)
• Rework MSBuild file invalidation to be well specified
• Rework test infra to support large test matrices

Extensibility and documentation parts are expected in Episode 6

[jkoritzinsky]

Would it be possible to add the function pointers work to this PR and remove SharpPatch completely? If possible I’d like to go down that route since IL patching is a pain and makes strong naming annoying.

[andrew-boyarshin]

I can do that, of course, but I'd prefer to leave the old method in case someone can't upgrade Roslyn version (some enterprise backwaters, for instance) and use C# 9.0. In fact, I have also added Marshal delegates method in my dev branch (which has a known issue, but it works) in case both C# 9.0 and assembly patching can't be used.

[amerkoleci]

Would be possible to add map directly to nuint and nint? And generation of void* instead of IntPtr?
Those are two request I would have.
Thanks

[jkoritzinsky]

As these PRs are for SharpGenTools 2.0 and we’re already accepting breaking changes, I honestly don’t care if people can’t upgrade to an SDK that supports function pointers. 1.x doesn’t use them, so people can stay on that version. We have a servicing branch for 1.x that people can make changes to if need be. And if people are stuck on an LTS SDK for some enterprisey reason, that will be remedied when .NET 6.0 comes out in November.

I’m also opposed to using Marshal delegates instead of function pointers. It was an explicit design decision for perf reasons and limiting allocations to use calli and not delegates.

And if we bump the minimum version to C#9, we can probably update default mappings to use nint and nuint as requested. We could also convert the last step of the code generator to be a C# Source Generator and get out of the business of writing source files to disk.

[amerkoleci]

Sorry for bumping the topic, I agree with @jkoritzinsky , lets focus on future stuff.
I would also to bring up the topic of documentation, atm my bindings has no documentation and would love to add them.

[andrew-boyarshin]

@amerkoleci it's not related to this PR, but I can answer it here:

1. NativeLong/NativeULong are backed by nint/nuint and provide the same API surface while being distinct from IntPtr (in that respect they are better than nint/nuint, which are terrifying Roslyn shenanigans for the sake of backwards compat).
2. But theoretically - yeah for both.

[amerkoleci]

Any change we can mute all those warnings about mapping already exists? see https://github.com/amerkoleci/Vortice.Windows/runs/1642426719?check_suite_focus=true

I would bring also attention on parameters name, most function has the Ref suffix "dstRef" or when returning type the "p" prefix is omitted, like D3D12CreateDevice you'll have deviceOut.

[andrew-boyarshin]

@jkoritzinsky okay, that's reasonable. I'm still not sure about doing it in this PR. I don't want to make it larger. SharpPatch is already heavily trimmed in this PR. Function pointers require extending IMarshaller API surface and implementing a new method on all marshallers. That's too big a change to add in this PR. But I'm actually happy that I won't have to finish delegates impl.

@amerkoleci documentation & extensibility is in Episode 6.

[jkoritzinsky]

NativeLong and NativeULong are only identical at the interop boundary to nint and nuint on non Windows. On Windows they’re supposed to map to int and uint.

Function pointers should only require updating the InvocationGenerator and the mapping phase. We already have the knowledge of the interop method we’re calling, so we should be able to construct the full signature. We have to emit it in other places today for SharpPatch to patch up.

[andrew-boyarshin]

@jkoritzinsky yeah, it turned out to be not so simple. TypeSyntax we have is sometimes not the one we can use to construct fnptr signature (non-convertible types). I am currently updating my dev branch based on this one, but dropping assembly patching and delegates as discussed. After that, we can discuss if IMarshaller still needs changes to API surface, since I can't really give an example right now.

[jkoritzinsky]

Ok. I’ll try to review this PR this week. Let’s get function pointers in soon after this one though. Really looking forward to removing SharpPatch and some of the complexity in the code base along with it.

[andrew-boyarshin]

@jkoritzinsky reminder

[jkoritzinsky]

What's the reasoning behind merging all of the different tasks together? I liked having the separation so I could actually debug the inputs and outputs of each step individually.

Also, one of my goals for the future is to add a new format for the mapping files that allows true incremental builds so we only re-parse C++ when the includes or defines change. Merging all of the tasks makes that much more difficult.

[andrew-boyarshin]

Merging it fixes all issues I've discovered with SharpGen invalidations: mostly it's overly aggressive bindings regenerations (most frequently seen when using IDEs: I remember at least once with VS and many times with Rider, when SharpGen started regenerating everything when there was no reason to do so). This also enhances current handling of files that are not visible to MSBuild (previously: no invalidations were done when changed, now: works as expected), or when properties are changed (due to whatever reason). Btw, it doesn't just try to duplicate what MSBuild does, it works together with MSBuild (by populating MSBuild target Inputs for the SharpGen target).

A new format for mapping files is cool (I also wanted that, I planned on using existing MSBuild infra, so that we'd have proper conditionals and FS invalidations). But I'm not sure this is the 2.0 goal, since it's pretty massive.

I see your point about incremental builds (it is something I was keeping in mind when working on this), but the current state is that incremental builds are broken, this PR fixes everything by dropping the malfunctioning incremental parts. If needed in the future, this can be partially brought back, but in a better way (I'm thinking actual MSBuild partial incremental builds for headers, by leveraging MSBuild item transforms), and with the TransformCppToCSharp/MapCppToCSharp and GenerateCSharp combined as they are in this PR (since they can't be properly incremental due to the many-to-1 file relationship in TransformCppToCSharp).

Also, debugging SharpGen is possible, I even made some improvements in this area in this PR (related to $(SharpGenWaitForDebuggerAttach), usage sample in SdkTests). Attaching a debugger is also easier now (since SharpGen now prints its MSBuild host process IDs).

It is also model serializability that I want to get rid of, at least for C# (this PR gets rid of both, for obvious reasons, but it doesn't remove the serialization cruft from the model). I can understand keeping it for C++ model (given the interest in incremental builds), but reading & writing huge XML files multiple times on each regeneration is a big red flag (at least for me).

[jkoritzinsky]

That's a reasonable argument. We can revisit incrementability in the future.