[Continuation] - Enable the fundamentals behind libraries tests compiled via Crossgen2 #80946
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,6 @@ | |
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
|
|
||
| <BundleDir>$([MSBuild]::NormalizeDirectory('$(OutDir)', 'publish'))</BundleDir> | ||
| <RunScriptOutputPath>$([MSBuild]::NormalizePath('$(BundleDir)', '$(RunScriptOutputName)'))</RunScriptOutputPath> | ||
| <RuntimeIdentifier>$(OutputRid)</RuntimeIdentifier> | ||
|
|
@@ -18,9 +16,16 @@ | |
| <SelfContained>true</SelfContained> | ||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition="'$(TestReadyToRun)' == 'true'"> | ||
| <PublishReadyToRun>true</PublishReadyToRun> | ||
| <PublishSingleFile>false</PublishSingleFile> | ||
| </PropertyGroup> | ||
|
|
||
| <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> | ||
|
There was a problem hiding this comment. This looks to be undoing #77304. Why? There was a problem hiding this comment. That's odd, those changes shouldn't be modified here. Will restore them in the next iteration. |
||
| <CppCompilerAndLinker Condition="'$(TargetArchitecture)' != '$(BuildArchitecture)' and '$(HostOS)' == 'linux' and '$(RuntimeIdentifier)' != 'linux-musl-arm64'">clang-9</CppCompilerAndLinker> | ||
| <SysRoot Condition="'$(TargetArchitecture)' != '$(BuildArchitecture)' and '$(HostOS)' != 'windows'">$(ROOTFS_DIR)</SysRoot> | ||
| <IlcBuildTasksPath>$(CoreCLRILCompilerDir)netstandard/ILCompiler.Build.Tasks.dll</IlcBuildTasksPath> | ||
| <IlcSdkPath>$(CoreCLRAotSdkDir)</IlcSdkPath> | ||
|
|
@@ -34,6 +39,10 @@ | |
| <SelfContained>true</SelfContained> | ||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition=""> | ||
| <DefineConstants>$(DefineConstants);SINGLE_FILE_TEST_RUN</DefineConstants> | ||
ivdiazsa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </PropertyGroup> | ||
|
|
||
| <Import Project="$(CoreCLRBuildIntegrationDir)Microsoft.DotNet.ILCompiler.SingleEntry.targets" Condition="'$(TestNativeAot)' == 'true'" /> | ||
|
|
||
| <ItemGroup Condition="'$(TestNativeAot)' == 'true'"> | ||
|
|
@@ -82,26 +91,17 @@ | |
| </ItemGroup> | ||
| </Target> | ||
|
|
||
|
There was a problem hiding this comment. This is still deleting the comment. Could you make is so that this is not even part of the diff? |
||
|
There was a problem hiding this comment. Also undoing #77304? Why? |
||
| <Target Name="__ReplaceCrossgen2ExecutableWithFreshlyBuiltOne" | ||
| BeforeTargets="_PrepareForReadyToRunCompilation"> | ||
| <PropertyGroup> | ||
| <Crossgen2ArtifactPath>$([MSBuild]::NormalizeDirectory('$(CoreCLRArtifactsPath)', 'crossgen2'))crossgen2$(ExeSuffix)</Crossgen2ArtifactPath> | ||
|
There was a problem hiding this comment. This should use |
||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <Crossgen2CurrentTool Include="@(Crossgen2Tool->'$(Crossgen2ArtifactPath)')" /> | ||
| <Crossgen2Tool Remove="@(Crossgen2Tool)" /> | ||
| <Crossgen2Tool Include="@(Crossgen2CurrentTool)" /> | ||
| </ItemGroup> | ||
| </Target> | ||
|
|
||
| <Target Name="PublishTestAsSingleFile" | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,9 @@ public static int Main(string[] args) | |
| var asm = typeof(SingleFileTestRunner).Assembly; | ||
| Console.WriteLine("Running assembly:" + asm.FullName); | ||
|
|
||
| // The current RemoteExecutor implementation is not compatible with the SingleFileTestRunner. | ||
| Environment.SetEnvironmentVariable("DOTNET_REMOTEEXECUTOR_SUPPORTED", "0"); | ||
|
Comment on lines
+30
to
+31
There was a problem hiding this comment. 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? There was a problem hiding this comment. By removal do you mean undoing dotnet/arcade#6553's changes, and then removing this line? There was a problem hiding this comment. 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. There was a problem hiding this comment. 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? There was a problem hiding this comment. Yep. Your change is making the arcade part redundant. |
||
|
|
||
| var diagnosticSink = new ConsoleDiagnosticMessageSink(); | ||
| var testsFinished = new TaskCompletionSource(); | ||
| var testSink = new TestMessageSink(); | ||
|
|
@@ -68,16 +71,77 @@ public static int Main(string[] args) | |
| { | ||
| if (args[i].Equals("-notrait", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| var traitKeyValue = args[i + 1].Split("=", StringSplitOptions.TrimEntries); | ||
|
|
||
| if (!noTraits.TryGetValue(traitKeyValue[0], out List<string> values)) | ||
| { | ||
| noTraits.Add(traitKeyValue[0], values = new List<string>()); | ||
| } | ||
|
|
||
| values.Add(traitKeyValue[1]); | ||
| i++; | ||
| } | ||
|
|
||
| if (args[i].Equals("-xml", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| xmlResultFileName = args[i + 1].Trim(); | ||
| i++; | ||
| } | ||
|
|
||
| if (args[i].Equals("-class", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| filters.IncludedClasses.Add(args[i + 1].Trim()); | ||
| i++; | ||
| } | ||
|
|
||
| if (args[i].Equals("-noclass", StringComparison.OrdinalIgnoreCase) || | ||
| args[i].Equals("-class-", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| filters.ExcludedClasses.Add(args[i + 1].Trim()); | ||
| i++; | ||
| } | ||
|
|
||
| if (args[i].Equals("-method", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| filters.IncludedMethods.Add(args[i + 1].Trim()); | ||
| i++; | ||
| } | ||
|
|
||
| if (args[i].Equals("-nomethod", StringComparison.OrdinalIgnoreCase) || | ||
| args[i].Equals("-method-", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| filters.ExcludedMethods.Add(args[i + 1].Trim()); | ||
| i++; | ||
| } | ||
|
|
||
| if (args[i].Equals("-namespace", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| filters.IncludedNamespaces.Add(args[i + 1].Trim()); | ||
| i++; | ||
| } | ||
|
|
||
| if (args[i].Equals("-nonamespace", StringComparison.OrdinalIgnoreCase) || | ||
| args[i].Equals("-namespace-", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| filters.ExcludedNamespaces.Add(args[i + 1].Trim()); | ||
| i++; | ||
| } | ||
|
|
||
| if (args[i].Equals("-parallel", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| string parallelismArg = args[i + 1].Trim().ToLower(); | ||
| var (parallelizeAssemblies, parallelizeTestCollections) = parallelismArg switch | ||
| { | ||
| "all" => (true, true), | ||
| "assemblies" => (true, false), | ||
| "collections" => (false, true), | ||
| "none" => (false, false), | ||
| _ => throw new ArgumentException($"Unknown parallelism option '{parallelismArg}'.") | ||
| }; | ||
|
|
||
| assemblyConfig.ParallelizeAssembly = parallelizeAssemblies; | ||
| assemblyConfig.ParallelizeTestCollections = parallelizeTestCollections; | ||
| i++; | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?