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

[8/21 Infra rollout] update sdk to preview6 #56161

Merged
merged 3 commits into from
Aug 2, 2021

Conversation

mangod9
Copy link
Member

@mangod9 mangod9 commented Jul 22, 2021

Need to update the sdk to preview6, since it has a arm64 bug fix which is causing many intermittent failures on osx-arm64 legs.

Fixes #56161

to check whether an arm64 bug is the root cause of NRE on macos.
@mangod9 mangod9 added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-crossgen2-coreclr labels Jul 22, 2021
@mangod9 mangod9 closed this Jul 23, 2021
@mangod9 mangod9 reopened this Jul 23, 2021
@ViktorHofer
Copy link
Member

cc @eerhardt for the linker errors in the wasm legs.

@eerhardt
Copy link
Member

cc @steveisok @MihaZupan @EgorBo - can one of you check out the Http ILLink.Substitutions issue?

@MihaZupan
Copy link
Member

MihaZupan commented Jul 23, 2021

#55384 moved the IsNativeHandlerEnabled switch from HttpClient to HttpClientHandler.AnyMobile - since it's in *AnyMobile, the code doesn't exist for other builds anymore and ILLink can't find it.

@steveisok Seems like that method should be moved into the shared HttpClientHandler file (and the unused one in HttpClient removed)

@eerhardt
Copy link
Member

Seems like that method should be moved into the shared HttpClientHandler file

Alternatively, we can make ILLink.Substitution.xml files for specific platforms (ex ILLink.Substitution.mobile.xml) and only include the mobile specific substitutions during the mobile builds.. See CoreLib for how this is done.

@ghost
Copy link

ghost commented Jul 23, 2021

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

Issue Details

Need to update the sdk to preview6, since it has a arm64 bug which is causing many intermittent failures on osx-arm64 legs.

Author: mangod9
Assignees: -
Labels:

* NO MERGE *, area-Infrastructure

Milestone: -

akoeplinger added a commit that referenced this pull request Jul 26, 2021
It will be done in #56161
steveisok pushed a commit to steveisok/runtime that referenced this pull request Jul 26, 2021
It was discovered in dotnet#56161 that mobile specific HttpClientHandler substitutions were sticking around even for non mobile builds.  This change moves the substitution into ILLink.Substitutions.mobile.xml.
steveisok added a commit that referenced this pull request Jul 27, 2021
It was discovered in #56161 that mobile specific HttpClientHandler substitutions were sticking around even for non mobile builds. This change moves the substitution into ILLink.Substitutions.mobile.xml.
@mangod9
Copy link
Member Author

mangod9 commented Jul 27, 2021

Hi @MihaZupan /@steveisok , I see that #56306 has been merged. Does that resolve all the build issues or are more changes required? We hope to merge preview6 next week.

@mangod9 mangod9 closed this Jul 27, 2021
@mangod9 mangod9 reopened this Jul 27, 2021
@mangod9
Copy link
Member Author

mangod9 commented Jul 27, 2021

Appears that some mono build issues still persist.

lukas-lansky pushed a commit that referenced this pull request Jul 27, 2021
It was discovered in #56161 that mobile specific HttpClientHandler substitutions were sticking around even for non mobile builds. This change moves the substitution into ILLink.Substitutions.mobile.xml.
lukas-lansky pushed a commit that referenced this pull request Jul 27, 2021
It was discovered in #56161 that mobile specific HttpClientHandler substitutions were sticking around even for non mobile builds. This change moves the substitution into ILLink.Substitutions.mobile.xml.
@eerhardt
Copy link
Member

@radical - Can you look into these failures? I believe the issue is with the ILLink Descriptors you added for the aggressively trimming the unit tests for WASM (#51697).

/__w/1/s/eng/testing/ILLinkDescriptors/ILLink.Descriptors.Castle.xml(5,6): error IL2008: Could not resolve type 'Castle.DynamicProxy.Internal.AbstractInvocation' [/__w/1/s/src/libraries/Microsoft.Extensions.FileProviders.Composite/tests/Microsoft.Extensions.FileProviders.Composite.Tests.csproj]
##[error]eng/testing/ILLinkDescriptors/ILLink.Descriptors.Castle.xml(5,6): error IL2008: (NETCORE_ENGINEERING_TELEMETRY=Build) Could not resolve type 'Castle.DynamicProxy.Internal.AbstractInvocation'
  Optimizing assemblies for size, which may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
/__w/1/s/.dotnet/sdk/6.0.100-preview.6.21355.2/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.ILLink.targets(78,5): error NETSDK1144: Optimizing assemblies for size failed. Optimization can be disabled by setting the PublishTrimmed property to false. [/__w/1/s/src/libraries/Microsoft.Extensions.FileProviders.Composite/tests/Microsoft.Extensions.FileProviders.Composite.Tests.csproj]
##[error].dotnet/sdk/6.0.100-preview.6.21355.2/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.ILLink.targets(78,5): error NETSDK1144: (NETCORE_ENGINEERING_TELEMETRY=Build) Optimizing assemblies for size failed. Optimization can be disabled by setting the PublishTrimmed property to false.

@akoeplinger
Copy link
Member

Just opened a PR that should fix it: #56392

mmitche added a commit that referenced this pull request Jul 27, 2021
* Update dependencies from https://github.com/dotnet/arcade build 20210726.4

Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.VersionTools.Tasks , Microsoft.DotNet.Build.Tasks.Workloads , Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk , Microsoft.DotNet.Build.Tasks.Packaging , Microsoft.DotNet.Build.Tasks.Installers , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Build.Tasks.Archives , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.ApiCompat , Microsoft.DotNet.CodeAnalysis , Microsoft.DotNet.XUnitConsoleRunner , Microsoft.DotNet.GenFacades , Microsoft.DotNet.GenAPI , Microsoft.DotNet.RemoteExecutor , Microsoft.DotNet.PackageTesting , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SharedFramework.Sdk
 From Version 6.0.0-beta.21372.16 -> To Version 6.0.0-beta.21376.4

* Split mobile specific ILLink.Substitutions into its own file (#56306)

It was discovered in #56161 that mobile specific HttpClientHandler substitutions were sticking around even for non mobile builds. This change moves the substitution into ILLink.Substitutions.mobile.xml.

* Revert back to p4 SDK

* Revert "Split mobile specific ILLink.Substitutions into its own file (#56306)"

This reverts commit ae413d9.

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Steve Pfister <steveisok@users.noreply.github.com>
Co-authored-by: Matt Mitchell <mmitche@microsoft.com>
…LLink descriptor files

The type is actually in the `Castle.DynamicProxy` namespace, not the `.Internal` one.
This causes an error with newer linker versions.

The whole `Castle.DynamicProxy` namespace is already preserved so we can remove the entry for AbstractInvocation.
@mangod9
Copy link
Member Author

mangod9 commented Jul 27, 2021

Thanks for the fixes, only one leg in staging Build MacCatalyst x64 Release AllSubsets_Mono is still failing. Rerunning now in case its intermittent.

@steveisok
Copy link
Member

@mangod9 what was failing in catalyst?

@safern
Copy link
Member

safern commented Jul 27, 2021

I'm a little bit worried that every time we try to update the SDK to a new version we are hitting a lot of linker/trimming test failures. Are we missing coverage on the linker? Should we update the linker more often on dotnet/runtime?

@mangod9
Copy link
Member Author

mangod9 commented Jul 27, 2021

Below is the catalyst failure. But there appear to be other helix failures in staging too (they dont show up in the github checks probably since the job timed out):

  Running: xcodebuild ONLY_ACTIVE_ARCH=YES CODE_SIGN_IDENTITY="-" -scheme "System.Transactions.Local.Tests" -destination "generic/platform=macOS,name=Any Mac,variant=Mac Catalyst" -UseModernBuildSystem=YES -archivePath "/Users/runner/work/1/s/artifacts/bin/System.Transactions.Local.Tests/net6.0-Release/maccatalyst-x64/AppBundle/System.Transactions.Local.Tests" -derivedDataPath "/Users/runner/work/1/s/artifacts/bin/System.Transactions.Local.Tests/net6.0-Release/maccatalyst-x64/AppBundle/System.Transactions.Local.Tests" IPHONEOS_DEPLOYMENT_TARGET=13.5 -configuration Release
  Using working directory: /Users/runner/work/1/s/artifacts/bin/System.Transactions.Local.Tests/net6.0-Release/maccatalyst-x64/AppBundle/System.Transactions.Local.Tests
  Exit code: 139
/Users/runner/work/1/s/eng/testing/tests.mobile.targets(206,5): error MSB4018: The "AppleAppBuilderTask" task failed unexpectedly. [/Users/runner/work/1/s/src/libraries/System.Transactions.Local/tests/System.Transactions.Local.Tests.csproj]
/Users/runner/work/1/s/eng/testing/tests.mobile.targets(206,5): error MSB4018: System.Exception: Error: Process returned non-zero exit code:  [/Users/runner/work/1/s/src/libraries/System.Transactions.Local/tests/System.Transactions.Local.Tests.csproj]
/Users/runner/work/1/s/eng/testing/tests.mobile.targets(206,5): error MSB4018:    at Utils.RunProcess(TaskLoggingHelper logger, String path, String args, IDictionary`2 envVars, String workingDir, Boolean ignoreErrors, Boolean silent, MessageImportance debugMessageImportance) in /_/src/tasks/Common/Utils.cs:line 97 [/Users/runner/work/1/s/src/libraries/System.Transactions.Local/tests/System.Transactions.Local.Tests.csproj]
/Users/runner/work/1/s/eng/testing/tests.mobile.targets(206,5): error MSB4018:    at Xcode.BuildAppBundle(String xcodePrjPath, String architecture, Boolean optimized, String devTeamProvisioning) in /_/src/tasks/AppleAppBuilder/Xcode.cs:line 398 [/Users/runner/work/1/s/src/libraries/System.Transactions.Local/tests/System.Transactions.Local.Tests.csproj]
/Users/runner/work/1/s/eng/testing/tests.mobile.targets(206,5): error MSB4018:    at AppleAppBuilderTask.Execute() in /_/src/tasks/AppleAppBuilder/AppleAppBuilder.cs:line 240 [/Users/runner/work/1/s/src/libraries/System.Transactions.Local/tests/System.Transactions.Local.Tests.csproj]
/Users/runner/work/1/s/eng/testing/tests.mobile.targets(206,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() in Microsoft.Build.dll:token 0x60015fc+0x3e [/Users/runner/work/1/s/src/libraries/System.Transactions.Local/tests/System.Transactions.Local.Tests.csproj]
/Users/runner/work/1/s/eng/testing/tests.mobile.targets(206,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) in Microsoft.Build.dll:token 0x600147b+0x804 [/Users/runner/work/1/s/src/libraries/System.Transactions.Local/tests/System.Transactions.Local.Tests.csproj]
##[error]eng/testing/tests.mobile.targets(206,5): error MSB4018: (NETCORE_ENGINEERING_TELEMETRY=Build) The "AppleAppBuilderTask" task failed unexpectedly

@eerhardt
Copy link
Member

Are we missing coverage on the linker? Should we update the linker more often on dotnet/runtime?

The linker used in dotnet/runtime is updated very often outside of the update of the SDK.

runtime/eng/Versions.props

Lines 168 to 170 in b18ff29

<!-- ILLink -->
<MicrosoftNETILLinkTasksVersion>6.0.100-preview.6.21370.1</MicrosoftNETILLinkTasksVersion>
<MicrosoftNETILLinkAnalyzerPackageVersion>$(MicrosoftNETILLinkTasksVersion)</MicrosoftNETILLinkAnalyzerPackageVersion>

I believe both issues in this upgrade to preview6 were WASM specific errors. And the issue there is that the WASM tooling doesn't use the linker version that the rest of the repo uses. And instead, the WASM tooling uses the linker version that comes with the SDK.

In the update to preview5 (#55283), the failure wasn't a "new linker" issue, but instead was a "new SDK" issue. The new SDK defaulted MSBuild properties differently, which caused our tests to fail. We can't catch those kinds of errors without trying to update to the new SDK.

@safern
Copy link
Member

safern commented Jul 27, 2021

@eerhardt thanks for clarifying.

I believe both issues in this upgrade to preview6 were WASM specific errors. And the issue there is that the WASM tooling doesn't use the linker version that the rest of the repo uses. And instead, the WASM tooling uses the linker version that comes with the SDK.

Should we fix that to use the same version that other parts of the repo use?

@eerhardt
Copy link
Member

Should we fix that to use the same version that other parts of the repo use?

I think it would make sense

@safern
Copy link
Member

safern commented Jul 27, 2021

cc: @lewing @radical

@radical
Copy link
Member

radical commented Jul 27, 2021

I believe both issues in this upgrade to preview6 were WASM specific errors. And the issue there is that the WASM tooling doesn't use the linker version that the rest of the repo uses. And instead, the WASM tooling uses the linker version that comes with the SDK.

@eerhardt Could you point to what exactly is using the linker version from the SDK?

@eerhardt
Copy link
Member

Could you point to what exactly is using the linker version from the SDK?

The WASM tests that have been failing in this PR (i.e. the failures that prompted #56306 and #56392) are using the linker version from the SDK. That's why the tests are suddenly failing when we update the SDK from preview5 to preview6 in this PR - because they are now using the new linker, which has more rules in preview6 than in preview5.

If those tests were using the linker version that gets updated daily (see the PRs that change this line), they would have already been failing.

You can find out which linker version is being used by building the tests with /bl and inspecting the .binlog to see which version is being used.

Copy link
Member

@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.

Looks good. But we usually wait for an "infrastructure rollout" to update the SDK. Is this going to be part of one? Or are we going to do a "one off" here?

cc @ViktorHofer @Anipik

@mangod9
Copy link
Member Author

mangod9 commented Jul 27, 2021

Yeah it’s part of August rollout so will merge next week

@mangod9 mangod9 changed the title [WIP] update sdk to preview6 [8/21 Infra rollout] update sdk to preview6 Aug 2, 2021
@mangod9 mangod9 removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 2, 2021
@ViktorHofer ViktorHofer merged commit 7ed6714 into dotnet:main Aug 2, 2021
akoeplinger added a commit that referenced this pull request Aug 2, 2021
…-optimization dotnet/arcade dotnet/xharness dotnet/roslyn-analyzers dotnet/emsdk (#56211)

* Update dependencies from https://github.com/dotnet/xharness build 20210722.1

Microsoft.DotNet.XHarness.CLI , Microsoft.DotNet.XHarness.TestRunners.Xunit
 From Version 1.0.0-prerelease.21370.1 -> To Version 1.0.0-prerelease.21372.1

* Update dependencies from https://github.com/dotnet/arcade build 20210723.11

Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.VersionTools.Tasks , Microsoft.DotNet.Build.Tasks.Workloads , Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk , Microsoft.DotNet.Build.Tasks.Packaging , Microsoft.DotNet.Build.Tasks.Installers , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Build.Tasks.Archives , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.ApiCompat , Microsoft.DotNet.CodeAnalysis , Microsoft.DotNet.XUnitConsoleRunner , Microsoft.DotNet.GenFacades , Microsoft.DotNet.GenAPI , Microsoft.DotNet.RemoteExecutor , Microsoft.DotNet.PackageTesting , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SharedFramework.Sdk
 From Version 6.0.0-beta.21370.12 -> To Version 6.0.0-beta.21373.11

* Update dependencies from https://github.com/dotnet/xharness build 20210723.1

Microsoft.DotNet.XHarness.CLI , Microsoft.DotNet.XHarness.TestRunners.Xunit
 From Version 1.0.0-prerelease.21370.1 -> To Version 1.0.0-prerelease.21373.1

* Update dependencies from https://github.com/dotnet/roslyn-analyzers build 20210723.2

Microsoft.CodeAnalysis.NetAnalyzers
 From Version 6.0.0-rc1.21366.2 -> To Version 6.0.0-rc1.21373.2

* Update dependencies from https://dev.azure.com/dnceng/internal/_git/dotnet-optimization build 20210724.3

optimization.linux-x64.MIBC.Runtime , optimization.windows_nt-x64.MIBC.Runtime , optimization.windows_nt-x86.MIBC.Runtime , optimization.PGO.CoreCLR
 From Version 1.0.0-prerelease.21371.3 -> To Version 1.0.0-prerelease.21374.3

* Update dependencies from https://github.com/dotnet/roslyn-analyzers build 20210725.2

Microsoft.CodeAnalysis.NetAnalyzers
 From Version 6.0.0-rc1.21366.2 -> To Version 6.0.0-rc1.21375.2

* Revert SDK bump

It will be done in #56161

* Update dependencies from https://github.com/dotnet/runtime build 20210725.2

Microsoft.NETCore.DotNetHost , Microsoft.NETCore.DotNetHostPolicy , Microsoft.NETCore.ILAsm , runtime.native.System.IO.Ports , Microsoft.NET.Sdk.IL , System.Text.Json , System.Runtime.CompilerServices.Unsafe
 From Version 6.0.0-rc.1.21369.2 -> To Version 6.0.0-rc.1.21375.2

* Update dependencies from https://github.com/dotnet/arcade build 20210723.11

Microsoft.DotNet.XUnitExtensions , Microsoft.DotNet.VersionTools.Tasks , Microsoft.DotNet.Build.Tasks.Workloads , Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk , Microsoft.DotNet.Build.Tasks.Packaging , Microsoft.DotNet.Build.Tasks.Installers , Microsoft.DotNet.Build.Tasks.Feed , Microsoft.DotNet.Build.Tasks.Archives , Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.ApiCompat , Microsoft.DotNet.CodeAnalysis , Microsoft.DotNet.XUnitConsoleRunner , Microsoft.DotNet.GenFacades , Microsoft.DotNet.GenAPI , Microsoft.DotNet.RemoteExecutor , Microsoft.DotNet.PackageTesting , Microsoft.DotNet.Helix.Sdk , Microsoft.DotNet.SharedFramework.Sdk
 From Version 6.0.0-beta.21370.12 -> To Version 6.0.0-beta.21373.11

* Revert SDK bump

* Update dependencies from https://github.com/dotnet/emsdk build 20210726.1

Microsoft.NET.Workload.Emscripten.Manifest-6.0.100
 From Version 6.0.0-rc.1.21369.1 -> To Version 6.0.0-rc.1.21376.1

* Update dependencies from https://github.com/dotnet/emsdk build 20210726.4

Microsoft.NET.Workload.Emscripten.Manifest-6.0.100
 From Version 6.0.0-rc.1.21369.1 -> To Version 6.0.0-rc.1.21376.4

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@mangod9 mangod9 deleted the upgradeTopreview6 branch August 2, 2021 13:57
@radical
Copy link
Member

radical commented Aug 3, 2021

Could you point to what exactly is using the linker version from the SDK?

The WASM tests that have been failing in this PR (i.e. the failures that prompted #56306 and #56392) are using the linker version from the SDK. That's why the tests are suddenly failing when we update the SDK from preview5 to preview6 in this PR - because they are now using the new linker, which has more rules in preview6 than in preview5.

Which builds on the CI are we talking about? We have one that runs library tests, EAT, AOT, WasmBuildTests, runtime tests, in the runtime pipeline.

You can find out which linker version is being used by building the tests with /bl and inspecting the .binlog to see which version is being used.

Checking the path for the ILLink task should be enough, right? I checked for library tests, example, and that seemed to be using the same version, as one in linux/arm64.

@eerhardt
Copy link
Member

eerhardt commented Aug 3, 2021

Closing the loop here - the WASM errors that originally failed this PR:

ILLink.Descriptors.Castle.xml(5,6): error IL2008: (NETCORE_ENGINEERING_TELEMETRY=Build) Could not resolve type 'Castle.DynamicProxy.Internal.AbstractInvocation'

were previously suppressed in the SDK in preview5. With https://github.com/dotnet/sdk/pull/17676/files#diff-63c4b1605cd7cef177bfd11abd51ab525581c9076caf29a69bce547b8c72b72cR228, we changed the SDK to no longer suppress the IL2008 warning. So when we brought in the preview6 SDK here, the error started showing up.

So my comment above

I believe both issues in this upgrade to preview6 were WASM specific errors. And the issue there is that the WASM tooling doesn't use the linker version that the rest of the repo uses. And instead, the WASM tooling uses the linker version that comes with the SDK.

was incorrect. It uses the right linker version. But it was a change in the SDK (removing a suppression) that caused this to show up.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants