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

RemoteExecutor: support single file and NativeAOT #11460

Closed
wants to merge 7 commits into from

Conversation

AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Oct 31, 2022

Problem

Currently the remote executor is used as both a library and an executable. When used as a library, the remote executor will start itself as an executable to remotely execute a method. For CoreCLR's single-file mode and NativeAOT, there is no separate file on disk for the remote executor executable, so it is not possible to start in this way.

Solution

This PR exposes an entry point to the main method of the remote executor so that single files hosts like dotnet/runtime's
SingleFileTestRunner
can also act as a remote remote executor host. A special argument is added to the command line so that the single file host knows whether it should execute as normal or execute as the remote executor. A copy of the SingleFileTestRunner is checked in, demonstrating this approach.

Note the reason this works at all with NativeAOT is the SingleFileTestRunner is build using TrimMode=partial. This trim mode is required for Xunit to function. I also experimented with making a source generator version of this library that is trim compatible, but that would require users of this library to update how they define remotely-callable methods.

Open Questions

  • Currently when building as single file and NativeAOT, two executables are created: one for the SingleFileTestRunner and one for Microsoft.DotNet.RemoteExecutor. I'm not sure if this will cause problems downstream.

Testing

  • The existing remote executor tests continue to pass.
  • Integrate tests for single file and NativeAOT into CI?

Related bugs: #5129 dotnet/runtime#77472

Downstream

If this is merged, dotnet/runtime#78144 will have to be cherry-picked into the commit that update the remote executor Nuget package.

@AustinWise AustinWise marked this pull request as ready for review October 31, 2022 22:05
@AustinWise
Copy link
Contributor Author

The build analysis says "This failure is expected and is not the reason why your PR failed". Can that be trusted, or should I look into the CI failures? I don't think the Apple Simulator supports the remote executor and failing tests (System.Numerics.Vectors, which I assume is from dotnet/runtime?) don't use remote executor.

@AustinWise
Copy link
Contributor Author

Updated to prevent infinite recursion. Also switched to using an environmental variable instead of a command line argument to trigger the remote executor.

@akoeplinger
Copy link
Member

The build analysis says "This failure is expected and is not the reason why your PR failed". Can that be trusted, or should I look into the CI failures?

Yes that is expected and won't actually fail the build (it just tests that failed tests show up in AzDO Test Results). The other failures were unrelated.

@akoeplinger
Copy link
Member

It looks good to me but I wonder if we really need the SingleFileTestRunner here. We'll notice if something breaks pretty quickly in dotnet/runtime so the code duplication doesn't seem that valuable to me. If we want to have it here we could put the SingleFileTestRunner into a package similar to Microsoft.DotNet.XUnitConsoleRunner and remove it from dotnet/runtime.

AustinWise and others added 6 commits November 7, 2022 17:46
Now that the single file test runner is not included in this repo, these are not needed to successfully publish the Nuget package.
@AustinWise
Copy link
Contributor Author

It looks good to me but I wonder if we really need the SingleFileTestRunner here. We'll notice if something breaks pretty quickly in dotnet/runtime so the code duplication doesn't seem that valuable to me. If we want to have it here we could put the SingleFileTestRunner into a package similar to Microsoft.DotNet.XUnitConsoleRunner and remove it from dotnet/runtime.

Sounds good, I've removed the SingleFileTestRunner, revered the warning suppressions required to make the SingleFileTestRunner build, and adopted your other suggestions.

@agocke
Copy link
Member

agocke commented Nov 8, 2022

I'll try to make some time to review this. Overall, I think the long term direction we should go in with Native AOT is to use the XUnitWrapper source generator from runtime -- but that needs to be changed to xunit compatible. I have something very simple that could probably be folded in, but haven't had the time: https://github.com/agocke/xunitgen.

I'm not as familiar with the remote executor so I'll look at that code in particular.

@AustinWise
Copy link
Contributor Author

I'll try to make some time to review this. Overall, I think the long term direction we should go in with Native AOT is to use the XUnitWrapper source generator from runtime -- but that needs to be changed to xunit compatible. I have something very simple that could probably be folded in, but haven't had the time: https://github.com/agocke/xunitgen.

I'm not as familiar with the remote executor so I'll look at that code in particular.

Looking at the XUnitWrapper source generator, it generates the application entry point:

https://github.com/AustinWise/runtime/blob/cff9fd93a561f195a5a57d7e3935fbb0f253b6c8/src/tests/Common/XUnitWrapperGenerator/XUnitWrapperGenerator.cs#L207-L213

So it should be compatible with the approach in this PR, as it could generate code to vector into the remote executor.

For reference, here is what the changes to the SingleFileTestRunner look like to support the remote executor:

dotnet/runtime#78144

@MichalStrehovsky
Copy link
Member

For comparison, this is the RemoteExecutor implementation we used in .NET Native timeframe: #4142 (this is where it got ripped out)

(For .NET Native we didn't have a SingleFileRunner because we just used the runner from here, but that's orthogonal - we should be using a source generated runner instead at some point.)

@AustinWise AustinWise closed this Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Infrastructure-libraries Area maintained by .NET libraries team: APICompat, AsmDiff, GenAPI, GenFacades, PkgProj, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants