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

Draft-only PR #161

Closed
wants to merge 1 commit into from
Closed

Conversation

andrew-boyarshin
Copy link
Contributor

@andrew-boyarshin andrew-boyarshin commented Mar 25, 2020

  • Migrate to net472;netcoreapp2.1 (MSBuild requires net472;netcoreapp2.0, .NET Core 2.0 is EOL, 2.1 is the closest LTS)
  • Update dependencies, minor code cleanups
  • Fix bug in SharpGen MSBuild targets (it was impossible to override SharpGenSdkAssembly path)
  • Clean up SharpGen MSBuild props and targets for best-practices.
  • Support multiple relations (see, cDimension there is a length for both initialValue and initialVelocity), e.g. length(initialValue),length(initialVelocity). Proper parsing for relations using Superpower toolkit.
  • Prevent crash when relation specifies parameter name, which is absent on the method. Print helpful message instead.
  • Workaround MSBuild Core not loading Microsoft.Win32.Registry implementation dll (I suspect bug in MSBuild assembly load context override) by building RuntimeIdentifier-specific assemblies (win;unix).
  • Workaround MSBuildSdkExtras bug with the previous point; upd: I've submitted PR to MSBuildSdkExtras to make this better.
  • Fix returning Result. Result is forcibly mapped to int, so the shadow callback returns int. But the actual C# interface returns Result, and Result->int cast is explicit, not implicit. Generate explicit cast.
  • A lot of newer changes, not specified above.

This PR is not meant to be merged (never ever), instead, it is the basis from which chunks can be torn out and upstreamed.

I'd like to have a feedback on these changes, if they are in line with SharpGenTools goals, if they are worthy of upstreaming, if there is a better way to accomplish their aims.

The only thing left now, preventing the correct binding generation in my project, is marshalling primitive arrays (double[]) in callbacks Vtbl (memory provided by caller, the callback must fill an array and return Result). SharpGen generates new array allocation instead of using the pointer received (that pointer is being cast to the correct pointer type and then forgotten). fast fixes the Native part, but not the Vtbl. I'll fill the issue if that is not a known issue.

This PR is being separated into multiple and upstreamed.

@andrew-boyarshin
Copy link
Contributor Author

@jkoritzinsky it would be great to have an initial feedback on the PR. Actual review is not needed, but I am interested in upstreaming these changes and I need confirmation they're worth upstreaming.

@jkoritzinsky
Copy link
Member

Everything here seems like a good idea to me.

@andrew-boyarshin
Copy link
Contributor Author

There are some issues with CodeCov token. Doesn't matter for this PR, but in general.
I'm preparing individual feature PRs now.

@andrew-boyarshin
Copy link
Contributor Author

@jkoritzinsky the only issue left is everything coverage-related, reproducible only on AzDO:

  • coverlet not finding modules, thus 0 coverage output
  • Wrong codecov token

Running build.ps1 and test.ps1 locally works fully (coverage reported correctly). The whole coverage situation in .NET world seems extremely fragile at the moment. No proper report merging, known deficiencies in AzDO publish coverage task, coverlet team still working on v2 tool spec. I'd say it's not worth the trouble.

I'll split this PR tomorrow into:

  1. Infra + TFM/RID-agnostic Runtime PR
  2. Relations PR
  3. The rest

@jkoritzinsky
Copy link
Member

I'd be fine dropping code-coverage checking for a while. It was primarily important when I was doing full rewrites of the C# code generator to make sure I got all the special cases. If it's working locally though, I'd prefer to move the code coverage handling under a switch so that I can still use it locally when changing marshalling. It's fine if we turn it off in CI. I'll go disable the check.

Those 3 PRs sound like a great way to split this. Thanks for your contributions! I really appreciate it!

@andrew-boyarshin
Copy link
Contributor Author

Once #165 is merged, I'll rebase this and split into multiple PRs.

@andrew-boyarshin
Copy link
Contributor Author

andrew-boyarshin commented May 14, 2020

I've rebased everything I had on the latest master, and committed everything I have on the refactored SharpGen pipeline. This is still WIP:

  1. New MicrosoftDocs extension tests don't work, contain excessive code
  2. Typo in Sdk.targets
  3. New non-MIT code not taken care of
  4. There are many improvements that could be done to MicrosoftDocs results (cleaning up formatting, autolinking), some were present in MSDN provider, some are more QoL features to have
  5. Clean up this PR (I even pushed binlogs here by accident)

Of course upstreaming this will take a long time (splitting this into chunks — nightmare).

There is a new code here taken from 3rd party projects:

  1. ObservableHashSet<T> from Entity Framework Core, Apache 2.0 with plans to move to MIT, but no actual effort for now. Used in DocItem and DocSubItem in the SDK.
  2. Bits of HTML renderer for Markdig, BSD 2-Clause. Used in MicrosoftDocs extension.

Both have MIT-compatible non-copyleft licenses, but this still needs to be taken care of:

  1. Add proper attribution in this project's COPYING file
  2. Strip Markdig code a bit more

I'm thinking my 3rd PR (RotS) will contain a lot of refactorings, except for the main one (merging tasks into one). I'll try to put it together tomorrow.

@andrew-boyarshin
Copy link
Contributor Author

Some progress (not pushed to remote):

  1. Cleaned up this branch
  2. MicrosoftDocs tests compile and produce satisfactory results
  3. Dropped Apache2-licensed ObservableHashSet<T>, by replacing it with ObservableCollection-derived class with HashSet to keep uniqueness. As a bonus — guaranteed to preserve insertion order.
  4. Extended SharpGen.Runtime.PlatformDetection to provide OSVersion and IsInAppContainer (.NET Standard 1.1 was the tricky part)
  5. Partially moved ComUtilities from SharpGen.Runtime.COM to SharpGen.Runtime, renamed to ComActivationHelpers, merged changes made by @amerkoleci (runtime detection instead of #ifs)
  6. Well-documented ComContext enum, synchronized with latest Windows SDK IDL files and Microsoft Docs reference

To-do before I can call it a win:

  1. Do some actual testing in MicrosoftDocs SDK test
  2. Skip MicrosoftDocs SDK test on Unix platforms (Windows Animation Manager is obviously Windows-only)

ComActivationHelpers are more low-level than ComUtilities in the sense that they return Result, instead of bool or CheckError+void. They also provide means to invoke specific code path (AppContainer or not), if heuristic is incorrect/insufficient.

Sadly, I can't drop BSD-licensed code, I'm afraid it will have to be taken into account when merged into SharpGen v2 master branch.

I still haven't prepared the next PR. :(

@andrew-boyarshin
Copy link
Contributor Author

  • Brought back the ability to use SharpGen SDK as PackageReference, not as a MSBuild SDK. It blocks implicit SharpGen.Runtime PackageReference and SharpGenExtensionReference shortcut for PackageReference with PrivateAssets=all, but allows workaround for MSBuild SDKs version property is not expanded dotnet/msbuild#5349 (btw, my MSBuild PR still hasn't even been looked at).
  • Added simple Windows Animation Manager test in MicrosoftDocs SDK test (tests binding, not the docs, since those would require more work)
  • Windows Animation Manager test is skipped on non-Windows platforms

Done for now. I still haven't prepared the next PR, and I won't be able do that until the mid-July. :(

@andrew-boyarshin
Copy link
Contributor Author

andrew-boyarshin commented Dec 4, 2020

In my local branch I have a partially complete implementation of C# 9.0 function pointers codegen.

The updated Microsoft Docs doc provider was previously in this repo, but I've moved it back into its own repo, private for now until we get the extensibility working.

@andrew-boyarshin
Copy link
Contributor Author

Published IMarshaller refactoring (moved many methods to ICallableMarshaller), merged SharpPatch into SDK (+removed useless CLI), implemented calli via function pointers (C# 9.0+) and a fallback method to enable disallowing assembly patching and function pointers (via Marshal.GetDelegateForFunctionPointer, slow and has a known issue). Also greatly refactored tests, now they test both x64 and x86 (previously - only x64), fixed multiple issues around x86 support (master branch is really broken with x86), one known issue remains (BoolArrayMember).

@andrew-boyarshin
Copy link
Contributor Author

Per discussion in #173, dropped delegates & assembly patching impl. Only fnptrs impl remains. I'll investigate if major IMarshaller refactoring is still needed.

@andrew-boyarshin
Copy link
Contributor Author

Okay, I've successfully reverted a lot of changes (including splitting IMarshaller and adding new method to it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants