-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[Android] Introduce NetTraceToMibcConverter task & streamline testing targets #72394
Conversation
Tagging subscribers to this area: @directhex Issue DetailsNetTraceToMibcConverter
Streamline Android testing targets
|
c4404b7
to
c4417a7
Compare
… targets NetTraceToMibcConverter - Used in profiled AOT scenarios where a .nettrace file is given as input and is converted to a .mibc file that can be fed into the AOT compiler. This previously was in the AotCompiler task, but for clarity purposes is now separated out. Streamline Android testing targets - The testing targets function the same, but are now structured similarly to iOS and Wasm. - Introduced new testing properties to support profiled AOT: RunProfileAOT - true/false to control the mode the aot compiler is set in. NetTracePath - The path to a .nettrace file that will be converted into a .mibc file and fed into the aot compiler RuntimeComponents - The list of native components to include in the test app build (diagnostics_tracing) DiagnosticsPorts - The ip address:port where the runtime will listen when running diagnostic tooling DiagnosticStartup - The mode the runtime will use at startup for diagnostic scenarios. Suspend will halt the app very early and wait, while nosuspend will wait for a connection, but not halt the runtime
c4417a7
to
3363776
Compare
<_PublishAssemblies Include="$(PublishDir)\**\*.dll" Exclude="$(PublishDir)\**\*.resources.dll" /> | ||
<_SatelliteAssemblies Include="$(PublishDir)\**\*.resources.dll" /> | ||
|
||
<AndroidAssembliesToBundle Include="@(_PublishAssemblies)"> |
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.
Nit since publish assesmblies naming implis all assemblies, and it doesnt seem like its being used elsewhere, so is there a benefit to having it in a separate itemgroup rather than explicitly defining AndroidAssembliesToBundle
?
<_PublishAssemblies Include="$(PublishDir)\**\*.dll" Exclude="$(PublishDir)\**\*.resources.dll" /> | |
<_SatelliteAssemblies Include="$(PublishDir)\**\*.resources.dll" /> | |
<AndroidAssembliesToBundle Include="@(_PublishAssemblies)"> | |
<_SatelliteAssemblies Include="$(PublishDir)\**\*.resources.dll" /> | |
<AndroidAssembliesToBundle Include="$(PublishDir)\**\*.dll" Exclude="@(_SatelliteAssemblies)"> |
Is there a particular reason we are keeping track of SatelliteAssemblies in an itemgroup?
If its not going to be used, maybe just
<AndroidAssembliesToBundle Include="$(PublishDir)\**\*.dll" Exclude="$(PublishDir)\**\*.resources.dll" />
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 could be wrong, but I think the paths need expanded in an item first before you can condition them like we do in AndroidAssembliesToBundle
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.
Ah do you mean the metadata getting set properly?
I think it still works properly before getting expanded. With a console project with a.txt
b.txt
c.txt
in the same directory
<ItemGroup>
<TestItemGroup Include="$(MSBuildThisFileDirector)*.txt">
<_InternalForceInterpret Condition="'%(FileName)%(Extension)' != 'b.txt'">true</_InternalForceInterpret>
</TestItemGroup>
</ItemGroup>
So it looks like the metadata is still set properly
/// NetTrace file to use when invoking dotnet-pgo for | ||
/// </summary> | ||
[Required] | ||
public string NetTracePath { get; set; } = ""; |
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.
Should we extend NetTracePath to be multiple NetTrace files?
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.
Does dotnet-pgo support this?
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 I dont think so, but it does support merging multiple .mibc files. I'm not sure if its a big desire to input multiple .nettrace files, but if it is a use case we could generate multiple .mibc and merge it, or perhaps dotnet-pgo could be extended to convert multiple .nettrace files.
src/tasks/NetTraceToMibcConverterTask/NetTraceToMibcConverter.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Mitchell Hwang <[email protected]>
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
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 it looks good if the build order is good https://github.com/dotnet/runtime/pull/72394/files#r924949694, my mistake, I got the BeforeTargets and AfterTargets mixed up again, updating the comments.
https://github.com/dotnet/runtime/pull/72394/files#r924955076
#72394 (comment)
Can the HelloWorld desktop sample be updated as well? https://github.com/dotnet/runtime/blob/main/src/mono/sample/HelloWorld/HelloWorld.csproj
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Can we update HelloWorld.csproj to use the MonoAOTCompiler.cs and NetTraceToMibcConverter.cs changes to ensure things are working (in terms of correct parameter name etc).
If everything is functioning as expected, it looks good to me!
Yes, I'd rather do it in a follow up. |
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.
some comments but looks good overall
|
||
protected override string GenerateFullPathToTool() | ||
{ | ||
return ToolPath; |
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.
@steveisok ok but we could at least make ToolExe default to dotnet-pgo
so you don't need to explicitly pass that in. That reminds me, do we need to append .exe
on Windows?
eng/testing/tests.mobile.targets
Outdated
<PublishTestAsSelfContainedDependsOn>Publish</PublishTestAsSelfContainedDependsOn> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup> | ||
<RunAOTCompilation Condition="'$(TargetOS)' == 'iOS' or '$(TargetOS)' == 'tvOS'">true</RunAOTCompilation> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup> | ||
<DotnetPgoToolDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', 'coreclr', '$(TargetOS).$(TargetArchitecture).$(Configuration)', 'dotnet-pgo'))</DotnetPgoToolDir> |
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.
nit: this could be unified with ILAsmToolPath
which references the same path in Directory.Build.props
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.
Since ILAsmToolPath
is only active when it's source build or s390x, I made a CoreCLRToolPath
property and used that instead.
src/tasks/MonoTargetsTasks/NetTraceToMibcConverterTask/NetTraceToMibcConverter.cs
Outdated
Show resolved
Hide resolved
The wasm failures are known and unrelated. |
.. builds. dotnet#72394 disabled the cache always, even when it was intended to be used. This reverses that change. Fixes dotnet#73419
Hi @steveisok, it looks like this broke native build on s390x:
The problem seems to be that the new
This in turn pulls in code in So the question is, what is the best way to fix this? Of course, we could try to rewrite Are the tools in |
@akoeplinger @directhex can you help Uli with this? |
NetTraceToMibcConverter
Streamline Android testing targets
The testing targets function the same, but are now structured similarly to iOS and Wasm.
Introduced new testing properties to support profiled AOT:
NetTraceFilePath - The path to a .nettrace file that will be converted into a .mibc file and fed into the aot compiler
RuntimeComponents - The list of native components to include in the test app build (diagnostics_tracing)
DiagnosticsPorts - The ip address:port where the runtime will listen when running diagnostic tooling
DiagnosticStartupMode - The mode the runtime will use at startup for diagnostic scenarios. Suspend will halt the app very early and wait, while nosuspend will wait for a connection, but not halt the runtime