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

[AOT] Added publishAOT test app to ensure OpenTelemetry SDK is AOT safe. #4392

Merged
merged 32 commits into from Apr 21, 2023

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented Apr 15, 2023

Changes

Towards #3429
Alternative approach of #4370 to get the list of AOT warnings.

  • Added OpenTelemetry.AotCompatibility.TestApp project that references all the assemblies you want to analyze for AOT / trimming that targets net7.0. And set PublishAot=true in this project. Following approach in
    https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#show-all-warnings-with-sample-application
    to get the AOT warnings from the dependent packages. Specifying <TrimmerRootAssembly Include="YourLibraryName" /> in an tag ensures that every part of the library is analyzed. It tells the trimmer that this assembly is a "root" which means the trimmer will analyze the assembly as if everything will be used, and traverses all possible code paths that originate from that assembly.

  • Added a test OpenTelemetry.AotCompatibility.Tests that calls dotnet publish on the above Exe. EnsureAotCompatibility will be update to assert there are no warnings emitted when all the warnings are addressed.
    Current publish test logs:
    publish_testApp.log

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #4392 (794d92c) into main (678e9c4) will increase coverage by 0.05%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4392      +/-   ##
==========================================
+ Coverage   84.73%   84.79%   +0.05%     
==========================================
  Files         300      300              
  Lines       12064    12064              
==========================================
+ Hits        10223    10230       +7     
+ Misses       1841     1834       -7     

see 4 files with indirect coverage changes

@Yun-Ting Yun-Ting changed the title [Not for merging] Get AOT warnings [AOT] Added publishAOT test app to ensure OpenTelemetry SDK is AOT safe. Apr 17, 2023
@Yun-Ting Yun-Ting marked this pull request as ready for review April 17, 2023 21:08
@Yun-Ting Yun-Ting requested a review from a team as a code owner April 17, 2023 21:08
@Yun-Ting
Copy link
Contributor Author

Hi eerhardt, could you help in reviewing this PR? Thank you.

///
/// error IL2091: 'T' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors'.
/// </summary>
[Fact]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious - would it make sense to have the AOT test build as a separate CI job instead of using unit test here? (easier to turn on/off, can run in parallel with existing unit test pipelines, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking while we are still developing, i.e. the warnings are > 0, we keep this as a unit test so that it's easier to capture the expected number of warnings and run the test locally.
Once we fixed/suppressed all the warnings, we can bring it to be a CI to get the above mentioning benefits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that it's easier to capture the expected number of warnings and run the test locally.

You should be able to run the CI check locally as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍

Copy link
Contributor

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good way of getting all the warnings for the assemblies listed.


Assert.True(process.WaitForExit(milliseconds: 180_000), "dotnet publish command timed out after 180 seconds.");

Assert.True(process.ExitCode == 0, "Publishing the AotCompatibility app failed. See test output for more details.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to consider a way to baseline the existing warnings to start, and ensure new warnings aren't introduced as new features are added. I'm not sure the best way to do this, but maybe @vitek-karas @agocke or @sbomer knows of a good way?

In dotnet/sdk, we use the following method to verify the warnings (see the callers above this method):

https://github.com/dotnet/sdk/blob/7d23e9d3e4aad58a5b497d8d91a50ffdf148b238/src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs#L803-L834

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than baselining the output of the build I can't think of another way for NativeAOT right now.

ILLink supports suppressing warnings via XML files, which would be the best solution here, unfortunately that functionality isn't implemented in NativeAOT (it's on the list, but I don't know when it's going to happen).
NativeAOT does support suppressing warnings in code via assembly-level attributes, but those would have to go onto the assembly which produces the warnings, which is not useful to this case.

var process = new Process
{
// set '-nodereuse:false /p:UseSharedCompilation=false' so the MSBuild and Roslyn server processes don't hang around, which may hang the test in CI
StartInfo = new ProcessStartInfo("dotnet", $"publish {testAppProject} --self-contained -nodereuse:false /p:UseSharedCompilation=false")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these can be achieved in the CI tasks itself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the info, Cijo : )
Please check out this comment: #4392 (comment)

@utpilla utpilla merged commit ef0c921 into open-telemetry:main Apr 21, 2023
17 checks passed
@Yun-Ting Yun-Ting deleted the yunl/enableAOT_2 branch April 21, 2023 20:01
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

6 participants