Fix native AoT test scripts #1540

martincostello · 2024-01-20T12:46:06Z

Changes

Support the AoT compatibility script being run locally on Windows.
Fix AoT test script not failing for IL compiler errors, only warnings.
Fix AoT test script not failing if dotnet publish fails.
.NET 8 supports native AoT publishing on macOS, so remove condition.

Found while looking into an app of mine is getting trim warnings from OpenTelemetry.Instrumentation.AWS to see if they were easy to fix.

codecov · 2024-01-20T12:47:36Z

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (71655ce) 73.91% compared to head (b95b8da) 73.37%.
Report is 136 commits behind head on main.

❗ Current head b95b8da differs from pull request most recent head acc16e4. Consider uploading reports for the commit acc16e4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1540      +/-   ##
==========================================
- Coverage   73.91%   73.37%   -0.55%     
==========================================
  Files         267      256      -11     
  Lines        9615     9457     -158     
==========================================
- Hits         7107     6939     -168     
- Misses       2508     2518      +10

Flag	Coverage Δ
unittests-Exporter.Geneva	`58.03% <49.46%> (?)`
unittests-Exporter.OneCollector	`89.72% <100.00%> (?)`
unittests-Extensions	`82.75% <100.00%> (?)`
unittests-Instrumentation.AspNet	`75.66% <ø> (?)`
unittests-Instrumentation.EventCounters	`75.92% <ø> (?)`
unittests-Instrumentation.Owin	`85.71% <ø> (?)`
unittests-Instrumentation.Process	`100.00% <ø> (?)`
unittests-Instrumentation.Runtime	`100.00% <ø> (?)`
unittests-Instrumentation.StackExchangeRedis	`71.00% <ø> (?)`
unittests-Instrumentation.Wcf	`78.47% <ø> (?)`
unittests-PersistentStorage	`58.80% <ø> (?)`
unittests-ResourceDetectors.Azure	`80.90% <ø> (?)`
unittests-ResourceDetectors.Host	`40.00% <ø> (?)`
unittests-ResourceDetectors.Process	`100.00% <ø> (?)`
unittests-ResourceDetectors.ProcessRuntime	`76.08% <ø> (?)`
unittests-Solution	`80.66% <88.57%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
....Exporter.Geneva/GenevaExporterHelperExtensions.cs	`100.00% <ø> (+31.81%)`	⬆️
...Telemetry.Exporter.Geneva/GenevaExporterOptions.cs	`90.00% <ø> (ø)`
...OpenTelemetry.Exporter.Geneva/GenevaLogExporter.cs	`85.00% <ø> (+7.50%)`	⬆️
...lemetry.Exporter.Geneva/GenevaLoggingExtensions.cs	`100.00% <ø> (+14.28%)`	⬆️
...enTelemetry.Exporter.Geneva/GenevaTraceExporter.cs	`82.50% <ø> (+7.50%)`	⬆️
...xporter.Geneva/Internal/ConnectionStringBuilder.cs	`92.94% <100.00%> (ø)`
...ry.Exporter.Geneva/Internal/ExporterEventSource.cs	`14.28% <ø> (+9.52%)`	⬆️
...eneva/Internal/ReentrantActivityExportProcessor.cs	`100.00% <ø> (ø)`
...porter.Geneva/Internal/ReentrantExportProcessor.cs	`100.00% <ø> (ø)`
...ry.Exporter.Geneva/Internal/TableNameSerializer.cs	`98.93% <ø> (ø)`
... and 91 more

... and 181 files with indirect coverage changes

cijothomas · 2024-01-22T16:31:11Z

@Yun-Ting Could you also review?

eerhardt · 2024-01-30T20:00:02Z

build/test-aot-compatibility.ps1

 $rootDirectory = Split-Path $PSScriptRoot -Parent
-$publishOutput = dotnet publish $rootDirectory/test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj -nodeReuse:false /p:UseSharedCompilation=false
+$publishOutput = dotnet publish $rootDirectory/test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj --runtime $runtime -nodeReuse:false /p:UseSharedCompilation=false


You shouldn't need to pass --runtime in. It will be inferred on the platform this code is executing on.

I was passing it explicitly so it married-up obviously with the output path, as the binary is found later down to run what was compiled, and I thought that made it easier to follow for the casual observer.

it married-up obviously with the output path

Following that same logic you would be passing -f $targetNetFramework to publish, because we are using it in the output path. (not that I'm suggesting we actually do that as well).

I'm just of the opinion to not clutter up the command if it doesn't add value. It just adds more things for people to try to understand what this line is doing.

eerhardt · 2024-01-30T20:01:02Z

build/test-aot-compatibility.ps1


 $actualWarningCount = 0

 foreach ($line in $($publishOutput -split "`r`n"))
 {
-    if ($line -like "*analysis warning IL*") 
+    if (($line -like "*analysis warning IL*") -or ($line -like "*analysis error IL*"))


Under what cases does it emit analysis warning IL vs analysis error IL? Do you need to set "TreatWarningsAsErrors" for this to happen?

To be honest, I'm not sure. I was working locally on a project I knew had AoT issues, but the script wasn't flagging any issues. When I manually ran the commands this script was doing, I found the errors.

I can get dotnet publish for PublishAot=true to produce "analysis error IL" only on a 9.0 SDK and if TreatWarningsAsErrors=true. So this seems like the a valid change to make (especially if people are using the script as a starting point).

Yun-Ting · 2024-01-31T18:21:09Z

If the goal of the PR is to support running verifyaotcompat as one of the CI flow on Windows, the below part will need to be updated, too.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/8ca81d4ce29b3550195f55547870c2e5165fcd92/.github/workflows/verifyaotcompat.yml#L14C1-L15C1

That being said, I don't think we'll need to commit the change to the shell file if we are just trying to run the script locally : ).

martincostello · 2024-01-31T18:22:57Z

No, the goal was just to make the script re-usable locally on a Windows computer.

I found that I couldn't use it while working on several PRs recently to add AoT support for some of the AWS libraries.

People should be able to easily use it to lower the barrier to contribution.

I can also update the test matrix to validate it on Windows as well if that's desired.

Yun-Ting · 2024-01-31T18:46:18Z

"I can also update the test matrix to validate it on Windows as well if that's desired." -> yes, please.
LGTM once the test matrix got updated. Thanks!

eerhardt

LGTM. Thanks!

Support the AoT compatibility script being run locally on Windows.

.NET 8 supports native AoT publishing on macOS.

- Script did not fail for errors, only warnings. - Script did not fail if publishing failed.

If publish fails, show why.

Use `Push-Location` and `Pop-Location` to resolve PowerShell lint warnings in Visual Studio Code.

- Remove explicit `--runtime` flag. - Cater for macOS explicitly.

Add Windows to the matrix for verifying AoT support.

martincostello requested a review from a team January 20, 2024 12:46

martincostello had a problem deploying to external January 20, 2024 12:46 — with GitHub Actions Failure

Kielek added the infra Infra work - CI/CD, code coverage, linters label Jan 22, 2024

martincostello mentioned this pull request Jan 22, 2024

Extend AoT support #1541

Merged

1 task

Kielek approved these changes Jan 22, 2024

View reviewed changes

cijothomas had a problem deploying to external January 22, 2024 16:30 — with GitHub Actions Failure

martincostello force-pushed the fix-aot-scripts branch from c96a787 to ce57646 Compare January 22, 2024 20:39

martincostello had a problem deploying to external January 22, 2024 20:39 — with GitHub Actions Failure

martincostello force-pushed the fix-aot-scripts branch from ce57646 to 36ad158 Compare January 23, 2024 00:11

martincostello had a problem deploying to external January 23, 2024 00:11 — with GitHub Actions Failure

martincostello had a problem deploying to external January 23, 2024 10:04 — with GitHub Actions Failure

martincostello mentioned this pull request Jan 23, 2024

[Instrumentation.AWSLambda] Fix AoT warnings #1544

Merged

1 task

martincostello force-pushed the fix-aot-scripts branch from 5fae692 to cac099d Compare January 24, 2024 17:40

martincostello had a problem deploying to external January 24, 2024 17:40 — with GitHub Actions Failure

martincostello force-pushed the fix-aot-scripts branch from cac099d to 452a25a Compare January 26, 2024 07:44

martincostello had a problem deploying to external January 26, 2024 07:44 — with GitHub Actions Failure

martincostello force-pushed the fix-aot-scripts branch from 452a25a to 10f3ddc Compare January 26, 2024 13:33

martincostello had a problem deploying to external January 26, 2024 13:33 — with GitHub Actions Failure

martincostello force-pushed the fix-aot-scripts branch from 10f3ddc to 128b59d Compare January 26, 2024 19:46

martincostello had a problem deploying to external January 26, 2024 19:46 — with GitHub Actions Failure

martincostello force-pushed the fix-aot-scripts branch from 128b59d to f45b7a2 Compare January 30, 2024 17:08

martincostello had a problem deploying to external January 30, 2024 17:08 — with GitHub Actions Failure

eerhardt reviewed Jan 30, 2024

View reviewed changes

martincostello had a problem deploying to external January 31, 2024 17:35 — with GitHub Actions Failure

martincostello force-pushed the fix-aot-scripts branch from ec33eb3 to f60a08b Compare January 31, 2024 18:03

martincostello had a problem deploying to external January 31, 2024 18:03 — with GitHub Actions Failure

Yun-Ting approved these changes Jan 31, 2024

View reviewed changes

eerhardt approved these changes Jan 31, 2024

View reviewed changes

martincostello had a problem deploying to external February 1, 2024 10:11 — with GitHub Actions Failure

martincostello force-pushed the fix-aot-scripts branch from a9652e7 to b95b8da Compare February 1, 2024 10:16

martincostello had a problem deploying to external February 1, 2024 10:16 — with GitHub Actions Failure

martincostello force-pushed the fix-aot-scripts branch from b95b8da to 6747803 Compare February 2, 2024 18:46

martincostello had a problem deploying to external February 2, 2024 18:46 — with GitHub Actions Failure

martincostello added 7 commits February 4, 2024 23:44

Support script on Windows

ace5667

Support the AoT compatibility script being run locally on Windows.

Validate AoT on macOS

34926f4

.NET 8 supports native AoT publishing on macOS.

Fix AoT test script false negatives

a411d77

- Script did not fail for errors, only warnings. - Script did not fail if publishing failed.

Show publish output

e6e5bd9

If publish fails, show why.

Fix lint warnings

853516b

Use `Push-Location` and `Pop-Location` to resolve PowerShell lint warnings in Visual Studio Code.

Address feedback

05eb22b

- Remove explicit `--runtime` flag. - Cater for macOS explicitly.

Verify native AoT on Windows

acc16e4

Add Windows to the matrix for verifying AoT support.

martincostello force-pushed the fix-aot-scripts branch from 6747803 to acc16e4 Compare February 4, 2024 23:44

martincostello had a problem deploying to external February 4, 2024 23:44 — with GitHub Actions Failure

Kielek merged commit 410ddc6 into open-telemetry:main Feb 5, 2024
112 of 113 checks passed

martincostello deleted the fix-aot-scripts branch February 5, 2024 07:26

martincostello mentioned this pull request Apr 8, 2024

Improve AoT validation and fix OpenTelemetryProtocol AoT warnings open-telemetry/opentelemetry-dotnet#5520

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix native AoT test scripts #1540

Fix native AoT test scripts #1540

martincostello commented Jan 20, 2024

codecov bot commented Jan 20, 2024 •

edited

Loading

cijothomas commented Jan 22, 2024

eerhardt Jan 30, 2024

martincostello Jan 30, 2024

eerhardt Jan 30, 2024

eerhardt Jan 30, 2024

martincostello Jan 30, 2024

eerhardt Jan 30, 2024

Yun-Ting commented Jan 31, 2024

martincostello commented Jan 31, 2024 •

edited

Loading

Yun-Ting commented Jan 31, 2024

eerhardt left a comment

Fix native AoT test scripts #1540

Fix native AoT test scripts #1540

Conversation

martincostello commented Jan 20, 2024

Changes

codecov bot commented Jan 20, 2024 • edited Loading

Codecov Report

cijothomas commented Jan 22, 2024

eerhardt Jan 30, 2024

Choose a reason for hiding this comment

martincostello Jan 30, 2024

Choose a reason for hiding this comment

eerhardt Jan 30, 2024

Choose a reason for hiding this comment

eerhardt Jan 30, 2024

Choose a reason for hiding this comment

martincostello Jan 30, 2024

Choose a reason for hiding this comment

eerhardt Jan 30, 2024

Choose a reason for hiding this comment

Yun-Ting commented Jan 31, 2024

martincostello commented Jan 31, 2024 • edited Loading

Yun-Ting commented Jan 31, 2024

eerhardt left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 20, 2024 •

edited

Loading

martincostello commented Jan 31, 2024 •

edited

Loading