[Continuation] - Enable the fundamentals behind libraries tests compiled via Crossgen2 #80946

ivdiazsa · 2023-01-20T19:41:23Z

This PR is a transfer and continuation from PR #75230.

Original Description:

Document how to use the special modes of libraries compilation.
Add the TestCrossgen2 flag to libraries testing.
Unlike TestNativeAot and TestSingleFile, use a self-contained strategy instead of the usual single file to improve debuggability of the produced output binaries.
Tweak the SingleFileTestRunner.cs file to avoid the fork bomb problem introduced by RemoteTestExecutor in a self-contained, but not Single File scenario.
Add support for a set of flags to SingleFileTestRunner to respect the command-line options required when debugging and investigating why the tests were not working.

ghost · 2023-01-20T19:41:43Z

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR is a transfer and continuation from PR #75230.

Original Description:

Document how to use the special modes of libraries compilation.
Add the TestCrossgen2 flag to libraries testing.
Unlike TestNativeAot and TestSingleFile, use a self-contained strategy instead of the usual single file to improve debuggability of the produced output binaries.
Tweak the SingleFileTestRunner.cs file to avoid the fork bomb problem introduced by RemoteTestExecutor in a self-contained, but not Single File scenario.
Add support for a set of flags to SingleFileTestRunner to respect the command-line options required when debugging and investigating why the tests were not working.

Author:	ivdiazsa
Assignees:	ivdiazsa
Labels:	`area-Infrastructure-libraries`
Milestone:	8.0.0

eng/testing/tests.singlefile.targets

trylek

Overall looks good except for one bit in test.singlefile.targets that looks suspicious to me so I'd welcome a bit of clarification.

MichalStrehovsky · 2023-01-23T06:23:34Z

eng/testing/tests.singlefile.targets

-    and the output is `CppLinker`, because from `SetupOSSpecificProps` onwards, we only use `CppLinker`
-    (when alternative compiler, i.e. gcc, is not selected)
-  -->
-  <Target Name="LocateNativeCompiler"


Also undoing #77304? Why?

See my comment above.

MichalStrehovsky · 2023-01-23T06:25:04Z

eng/testing/tests.singlefile.targets

  <PropertyGroup Condition="'$(TestNativeAot)' == 'true'">
    <IlcToolsPath>$(CoreCLRILCompilerDir)</IlcToolsPath>
    <IlcToolsPath Condition="'$(TargetArchitecture)' != '$(BuildArchitecture)'">$(CoreCLRCrossILCompilerDir)</IlcToolsPath>
+    <CppCompilerAndLinker Condition="'$(TargetArchitecture)' != '$(BuildArchitecture)' and '$(HostOS)' == 'linux' and '$(RuntimeIdentifier)' == 'linux-musl-arm64'">clang-15</CppCompilerAndLinker>


This looks to be undoing #77304. Why?

That's odd, those changes shouldn't be modified here. Will restore them in the next iteration.

MichalStrehovsky · 2023-01-23T06:26:26Z

eng/testing/tests.singlefile.targets

+  <PropertyGroup Condition="">
+    <DefineConstants>$(DefineConstants);SINGLE_FILE_TEST_RUNNER</DefineConstants>
+  </PropertyGroup>


I don't understand this change. We had this line at the top of the file. Now it's moved here with an empty Condition="" - why?

Oops for some reason, that condition wasn't copied. It has to do with Single File and NativeAOT. Will add it on the next iteration. Thanks for catching this.

MichalStrehovsky · 2023-01-23T06:28:16Z

eng/testing/tests.singlefile.targets

+  <Target Name="__ReplaceCrossgen2ExecutableWithFreshlyBuiltOne"
+          BeforeTargets="_PrepareForReadyToRunCompilation">
+    <PropertyGroup>
+      <Crossgen2ArtifactPath>$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)', 'crossgen2'))crossgen2$(ExeSuffix)</Crossgen2ArtifactPath>


This should use $(CoreCLRCrossgen2Dir) instead of recomputing it.

MichalStrehovsky · 2023-01-23T06:31:45Z

src/libraries/Common/tests/SingleFileTestRunner/SingleFileTestRunner.cs

+        // The current RemoteExecutor implementation is not compatible with the SingleFileTestRunner.
+        Environment.SetEnvironmentVariable("DOTNET_REMOTEEXECUTOR_SUPPORTED", "0");


The reason why the existing users of the SingleFileTestRunner.cs didn't need this line is: dotnet/arcade#6553

It is now redundant. Could you submit a removal of that in Arcade?

By removal do you mean undoing dotnet/arcade#6553's changes, and then removing this line?

The arcade change covers the existing use cases of the single file runner - the single file deployments (where assembly.location is null/empty, like the arcade PR checks).

With this PR, we'll be using the single file runner for non-single file scenarios as well. The line you're adding will disable remote executor for any consumer - single file or not.

Therefore, I think we won't need the arcade part anymore because remote executor will be disabled by this.

Got it. Then, let me see if I understood correctly. The arcade change and this line help with the same issue for this PR's purposes, so they are redundant in this case. Removing the arcade change while keeping this line will remove that redundancy without breaking other scenarios. Is this the appropriate course of action?

Yep. Your change is making the arcade part redundant.

MichalStrehovsky · 2023-01-23T06:32:37Z

...sts/System.Diagnostics.FileVersionInfo.Tests/System.Diagnostics.FileVersionInfo.Tests.csproj

@@ -50,5 +50,8 @@
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </ProjectReference>
    <ProjectReference Include="..\System.Diagnostics.FileVersionInfo.TestAssembly\System.Diagnostics.FileVersionInfo.TestAssembly.csproj" Condition="'$(TargetOS)' == 'browser'" />
+    <!-- R2R testing does not tolerate the combination of a regular project reference and a content reference. -->
+    <!-- This is actually a bug in the SDK and ought to be fixed there. -->


Could you add a link to the tracking SDK bug?

I believe we don't have one. Will have to file it.

Did you file the bug? If so could you add a link here same as we have on line 46 (for a similar issue)?

ViktorHofer · 2023-01-23T16:05:39Z

docs/workflow/testing/libraries/testing.md

-Building the `libs` subset or any of individual library projects automatically copies product binaries into the testhost folder
-in the bin directory. This is where the tests will load the binaries from during the run. However System.Private.CorLib is an
-exception - the build does not automatically copy it to the testhost folder. If you [rebuild System.Private.CoreLib](https://github.com/dotnet/runtime/blob/main/docs/workflow/building/libraries/README.md#iterating-on-systemprivatecorelib-changes) you must also build the `libs.pretest` subset to ensure S.P.C is copied before running tests.
+Building the `libs` subset or any of individual library projects automatically copies product binaries into the testhost folder in the bin directory. This is where the tests will load the binaries from during the run. However System.Private.CorLib is an exception - the build does not automatically copy it to the testhost folder. If you [rebuild System.Private.CoreLib](https://github.com/dotnet/runtime/blob/main/docs/workflow/building/libraries/README.md#iterating-on-systemprivatecorelib-changes) you must also build the `libs.pretest` subset to ensure S.P.C is copied before running tests.


Please excuse me if I'm mixing things up as it's been a long time since we last talked about. Why do we mention the testhost folder or the libs.pretest subset? I think that we discussed that we want to exercise Crossgen2 testing in self-contained deployment only in the dev innerloop.

I regret to say I don't know the answer. This change was just reformatting what was already written there to match the format of most of the repo's Markdown files. @trylek do you have context for Viktor's question?

I think there are indeed two facets to this discussion - (a) using Crossgen2-compiled System.Private.CoreLib in library testing and (b) compiling the framework and / or the library tests themselves using Crossgen2. For (a), AFAIK that's always been the case, to my knowledge no testing involves the IL version of SPC. For (b), that's what Ivan is now creating foundations for unless I'm mistaken as at least weekly library lab testing involving Crossgen2 would be very beneficial for improving our test coverage; @davidwrighton also believes Crossgen2 library testing could be used for better regression monitoring of some of our features like the cross-module inlining. Having said that, I'm not sufficiently familiar with library testing to know off the top of my head whether the comment under discussion is actually accurate or not.

Right. I think we all agree on that we want to have crossgen2 coverage for our libraries testing. What I commented on is that I don't think we want to do framework dependent crossgen2 testing but self-contained deployment testing. We use self-contained deployment for AOT testing, linker testing, single file testing... cc @jkotas for opinions.

For (a), AFAIK that's always been the case, to my knowledge no testing involves the IL version of SPC.

We do use the IL version of SPC for Debug builds or when performing code coverage measurements:

runtime/src/libraries/externals.csproj

Line 3 in 0ca3647

<SwapNativeForIL Condition="'$(SwapNativeForIL)' == '' and ('$(Configuration)' == 'Debug' or '$(Coverage)' == 'true') and '$(RuntimeFlavor)' != 'Mono'">true</SwapNativeForIL>

Thanks Viktor for clarifying the usage of IL SPC. For the rest of your comment, so am I right to understand your point is about "Native-AOT compiled self-contained Crossgen2 app" vs. "raw framework-dependent Crossgen2 app produced by CoreCLR build"? I certainly agree with that, we should do our best to test the actual customer-facing product, not some intermediate artifact of our build system (not mentioning the fact that in the past it also identified bugs in NativeAOT). I apologize, I didn't get that from your original PR comment at the top of this thread, in fact it seems you're saying the opposite in

we want to exercise Crossgen2 testing in self-contained deployment only in the dev innerloop

I apologize if I misread or misunderstood one of your responses, in such case please have patience with me being slow on the uptake and clarify.

I don't think we want to do framework dependent crossgen2 testing but self-contained deployment testing.

crossgening the whole framework takes a while. Would self-contained deployment mean that the whole framework needs to be crossgened for each libraries test? It does not look very efficient.

In CoreCLR, crossgenning the framework takes something like 2~3 minutes. It should certainly be done only once, not in Helix for each test separately, that would be hugely wasteful.

crossgening the whole framework takes a while. Would self-contained deployment mean that the whole framework needs to be crossgened for each libraries test? It does not look very efficient.

No. It would mean that we could crossgen the runtime pack that we use for self-contained deployment once, instead of the testhost folder that we use for framework-dependent deployment. It boils down to which app-mode we want to add crossgen validation to. FDD, SCD or both?

MichalStrehovsky · 2023-01-24T04:41:12Z

eng/testing/tests.singlefile.targets

@@ -82,31 +89,33 @@
    </ItemGroup>
  </Target>

-  <!--


This is still deleting the comment. Could you make is so that this is not even part of the diff?

ivdiazsa · 2023-02-09T02:02:16Z

Hi @MichalStrehovsky! Is there anything else needed for this PR to be approved and merged?

MichalStrehovsky

Looks good modulo the unaddressed comment.

MichalStrehovsky · 2023-02-09T03:37:49Z

...sts/System.Diagnostics.FileVersionInfo.Tests/System.Diagnostics.FileVersionInfo.Tests.csproj

@@ -50,5 +50,8 @@
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </ProjectReference>
    <ProjectReference Include="..\System.Diagnostics.FileVersionInfo.TestAssembly\System.Diagnostics.FileVersionInfo.TestAssembly.csproj" Condition="'$(TargetOS)' == 'browser'" />
+    <!-- R2R testing does not tolerate the combination of a regular project reference and a content reference. -->
+    <!-- This is actually a bug in the SDK and ought to be fixed there. -->


Did you file the bug? If so could you add a link here same as we have on line 46 (for a similar issue)?

…t tracking SDK bug.

ivdiazsa added 2 commits January 17, 2023 17:05

Added the documentation regarding the new libraries test modes.

c336dbf

Enable the fundamentals behind libraries tests compiled via crossgen2.

7f71c59

ivdiazsa added the area-Infrastructure-libraries label Jan 20, 2023

ivdiazsa added this to the 8.0.0 milestone Jan 20, 2023

ivdiazsa requested review from davidwrighton, MichalStrehovsky and trylek January 20, 2023 19:41

ivdiazsa self-assigned this Jan 20, 2023

ivdiazsa mentioned this pull request Jan 20, 2023

Enable the fundamentals behind libraries tests compiled via crossgen2 #75230

Closed

ivdiazsa requested a review from mangod9 January 20, 2023 19:42

trylek reviewed Jan 20, 2023

View reviewed changes

eng/testing/tests.singlefile.targets Outdated Show resolved Hide resolved

trylek approved these changes Jan 20, 2023

View reviewed changes

Fixed a typo with SINGLE_FILE_TEST_RUNNER.

3f2de8f

MichalStrehovsky requested changes Jan 23, 2023

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 23, 2023

ViktorHofer reviewed Jan 23, 2023

View reviewed changes

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 23, 2023

Addressed review comments.

8b3df1d

MichalStrehovsky reviewed Jan 24, 2023

View reviewed changes

Restored an accidentally deleted comment.

7ac2212

runfoapp bot mentioned this pull request Jan 24, 2023

Infra improvements for Helix #68176

Closed

build-analysis bot mentioned this pull request Jan 24, 2023

tracing/eventpipe/eventsourceerror/eventsourceerror/eventsourceerror failure #80666

Closed

Merge branch 'main' into books-with-cg2

9add8fa

ivdiazsa mentioned this pull request Jan 27, 2023

Remove Special Casing of RemoteExecutor Detection for Single File dotnet/arcade#12348

Merged

MichalStrehovsky mentioned this pull request Feb 2, 2023

Assertion failed 'value != 0' in 'System.Tests.DoubleTests_GenericMath:GetExponentShortestBitLengthTest()' during 'Do value numbering' #81460

Closed

MichalStrehovsky approved these changes Feb 9, 2023

View reviewed changes

ivdiazsa mentioned this pull request Feb 18, 2023

Dotnet Build -t:Test with ReadyToRun currently not allowing project references and content references combined dotnet/sdk#30718

Open

Fixed wrong Crossgen2 path and added comment with link to an importan…

6ae3ed7

…t tracking SDK bug.

ivdiazsa merged commit bf0608f into dotnet:main Feb 20, 2023

ghost locked as resolved and limited conversation to collaborators Mar 23, 2023

ivdiazsa deleted the books-with-cg2 branch April 24, 2023 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Continuation] - Enable the fundamentals behind libraries tests compiled via Crossgen2 #80946

[Continuation] - Enable the fundamentals behind libraries tests compiled via Crossgen2 #80946

ivdiazsa commented Jan 20, 2023

ghost commented Jan 20, 2023

trylek left a comment

MichalStrehovsky Jan 23, 2023

ivdiazsa Jan 23, 2023

MichalStrehovsky Jan 23, 2023

ivdiazsa Jan 23, 2023

MichalStrehovsky Jan 23, 2023

ivdiazsa Jan 23, 2023

MichalStrehovsky Jan 23, 2023

MichalStrehovsky Jan 23, 2023

ivdiazsa Jan 23, 2023

MichalStrehovsky Jan 24, 2023

ivdiazsa Jan 24, 2023 •

edited

Loading

MichalStrehovsky Jan 25, 2023

MichalStrehovsky Jan 23, 2023

ivdiazsa Jan 23, 2023

MichalStrehovsky Feb 9, 2023

ViktorHofer Jan 23, 2023

ivdiazsa Jan 23, 2023

trylek Jan 23, 2023

ViktorHofer Jan 23, 2023

trylek Jan 23, 2023

jkotas Jan 23, 2023

trylek Jan 23, 2023

ViktorHofer Jan 24, 2023 •

edited

Loading

MichalStrehovsky Jan 24, 2023

ivdiazsa commented Feb 9, 2023

MichalStrehovsky left a comment

MichalStrehovsky Feb 9, 2023

		// The current RemoteExecutor implementation is not compatible with the SingleFileTestRunner.
		Environment.SetEnvironmentVariable("DOTNET_REMOTEEXECUTOR_SUPPORTED", "0");

[Continuation] - Enable the fundamentals behind libraries tests compiled via Crossgen2 #80946

[Continuation] - Enable the fundamentals behind libraries tests compiled via Crossgen2 #80946

Conversation

ivdiazsa commented Jan 20, 2023

ghost commented Jan 20, 2023

trylek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivdiazsa Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivdiazsa commented Feb 9, 2023

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivdiazsa Jan 24, 2023 •

edited

Loading

ViktorHofer Jan 24, 2023 •

edited

Loading