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

Update SDK #43028

Merged
merged 26 commits into from
Aug 30, 2022
Merged

Update SDK #43028

merged 26 commits into from
Aug 30, 2022

Conversation

MackinnonBuck
Copy link
Member

No description provided.

@MackinnonBuck MackinnonBuck added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 1, 2022
@MackinnonBuck MackinnonBuck requested review from a team, dougbu and wtgodbe as code owners August 1, 2022 18:01
@ghost
Copy link

ghost commented Aug 1, 2022

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

@MackinnonBuck
Copy link
Member Author

The aspnetcore-components-e2e and aspnetcore-ci (Build Source-Build (Managed)) checks have failed twice now because dotnet-install failed:

/home/vsts/.nuget/packages/microsoft.dotnet.arcade.sdk/7.0.0-beta.22379.10/tools/InstallDotNetCore.targets(15,5): error : dotnet-install failed [/home/vsts/.nuget/packages/microsoft.dotnet.arcade.sdk/7.0.0-beta.22379.10/tools/Tools.proj]

Not totally sure what the cause is at the moment. See https://dev.azure.com/dnceng/public/_build/results?buildId=1916161&view=logs&j=2f0d093c-1064-5c86-fc5b-b7b1eca8e66a&t=d2b639a6-cb19-5f1f-66fd-8047f66b3026&l=126

@wtgodbe
Copy link
Member

wtgodbe commented Aug 1, 2022

I'd recommend checking out the binlog to see if there's more info (here). Looks like it's only in the source-build leg, so maybe there was a relevant change there in dotnet/sdk or dotnet/installer recently. @MichaelSimons might know more

@MichaelSimons
Copy link
Member

cc @dotnet/source-build-internal

@dougbu
Copy link
Member

dougbu commented Aug 3, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Tratcher
Copy link
Member

Tratcher commented Aug 9, 2022

dotnet/arcade#10311 (comment)

As it's now clear this is an MSBuild bug in this SDK (and thus not one users want to adopt) I am closing this issue.

I'll try a later SDK until this works.

@dougbu
Copy link
Member

dougbu commented Aug 9, 2022

@Tratcher this SDK didn't work. Suggest watching https://maestro-prod.westus2.cloudapp.azure.com/2237/https:%2F%2Fgithub.com%2Fdotnet%2Finstaller/latest/graph for a build that includes dotnet/msbuild@f2d13c6. Latest SDK (which, confusingly, we get from the dotnet/installer repo) includes msbuild commits from 6 days ago but we want a commit from 4 days ago.

@Tratcher Tratcher added the blocked The work on this issue is blocked due to some dependency label Aug 9, 2022
@Tratcher Tratcher self-assigned this Aug 9, 2022
@TanayParikh
Copy link
Contributor

TanayParikh commented Aug 11, 2022

Updated to 7.0.100-rc.1.22410.15 to match latest SDK from installer.

@TanayParikh
Copy link
Contributor

Suggest watching https://maestro-prod.westus2.cloudapp.azure.com/2237/https:%2F%2Fgithub.com%2Fdotnet%2Finstaller/latest/graph for a build that includes dotnet/msbuild@f2d13c6. Latest SDK (which, confusingly, we get from the dotnet/installer repo) includes msbuild commits from 6 days ago but we want a commit from 4 days ago.

Just did a trace and I believe 7.0.100-rc.1.22411.3 should now have everything we need. 🤞

@TanayParikh
Copy link
Contributor

should now have everything we need

😞 unfortunately not. Source build still failing.

@Tratcher Tratcher removed their assignment Aug 12, 2022
@TanayParikh
Copy link
Contributor

TanayParikh commented Aug 12, 2022

Looks like we have a new breaking change now:

src/DataProtection/DataProtection/src/AuthenticatedEncryption/ManagedAuthenticatedEncryptorFactory.cs(116,61): error IL2070: (NETCORE_ENGINEERING_TELEMETRY=Build) 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in call to 'System.Type.MakeGenericType(params Type[])'. The parameter 'implementation' of method 'Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption.ManagedAuthenticatedEncryptorFactory.AlgorithmActivator.CreateFactory(Type)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

@JamesNK @BrennanConroy would you happen to have context on this?

@MichaelSimons
Copy link
Member

Looks like we have a new breaking change now:

I just discovered this in source-build and logged an issue for it - #43253

@JamesNK
Copy link
Member

JamesNK commented Aug 13, 2022

I've added a possible fix for the warning - #43259

It might not fix the warning - ASP.NET Core isn't using the same SDK as source build so I can't test it - but if it's still a problem, then the follow-up change should be a minor fix.

@dougbu
Copy link
Member

dougbu commented Aug 13, 2022

Might be easier to test your commit in this PR's branch @JamesNK

@JamesNK
Copy link
Member

JamesNK commented Aug 13, 2022

@vitek-karas @sbomer What's going on with IL2121? A lot of them have shown up in the aspnetcore build. I tried to remove some of the suppressions, but they were correctly suppressing warnings.

I see you disabled this warning in dotnet/runtime@e65381b (#73410). Is IL2121 broken?

Should the incorrect suppression warning be suppressed because it's incorrect? 😆

@vitek-karas
Copy link
Member

vitek-karas commented Aug 13, 2022

IL2121 is off by default, but that "off by default" part is in SDK - which runtime or asp.net don't use to run the linker 😢
Please suppress them for now.

That said I would be interested to see the cases where the suppression is necessary but linker produces IL2121 still. The only place where we've seen that happening was either around feature switches, or in code which uses ifdefs.

/cc @jkurdek

@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

@captainsafia you're right and I think the latest build hit an outage or two.
@wtgodbe please do not update the SDK version again. It's doubtful we'll get a fix for the source-build problem from there. Instead, we need to figure out why YAML variables aren't becoming environment variables and affecting source-build. I'm looking…

@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

Build w/ @wtgodbe's latest SDK update had already failed w/ usual source-build problem when I pushed my newest fixup. Earlier placement of the job variables and even-earlier attempt to control the source-build job timeout wasn't quite right.

@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

And … source-build finally worked 🎉

@wtgodbe
Copy link
Member

wtgodbe commented Aug 30, 2022

Templates.Blazor.Test.BlazorWasmTemplateAuthTest.BlazorWasmHostedTemplate_IndividualAuth_NoHttps_Works_WithOutLocalDB failed, looks like it may be related to the SDK:

Project failed to publish. Exit code 1.
/private/tmp/helix/working/BD1F09DF/p/dotnet-cli/dotnet publish --no-restore -c Release /bl
StdErr:
StdOut: MSBuild version 17.4.0-preview-22416-02+5d102ae37 for .NET
/private/tmp/helix/working/BD1F09DF/p/dotnet-cli/sdk/7.0.100-rc.2.22426.5/MSBuild.dll -maxcpucount -property:_IsPublishing=true -property:Configuration=Release -target:Publish -verbosity:m /bl ./AspNet.dvhepurhcgwo.Server.csproj
/private/tmp/helix/working/BD1F09DF/p/dotnet-cli/sdk/7.0.100-rc.2.22426.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.RuntimeIdentifierInference.targets(219,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [/private/tmp/helix/working/BD1F09DF/w/A2D0090F/e/Templates/BaseFolder/AspNet.dvhepurhcgwo/Server/AspNet.dvhepurhcgwo.Server.csproj]
/private/tmp/helix/working/BD1F09DF/p/dotnet-cli/sdk/7.0.100-rc.2.22426.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.RuntimeIdentifierInference.targets(219,5): message NETSDK1057: You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy [/private/tmp/helix/working/BD1F09DF/w/A2D0090F/e/Templates/BaseFolder/AspNet.dvhepurhcgwo/Shared/AspNet.dvhepurhcgwo.Shared.csproj]
AspNet.dvhepurhcgwo.Shared -> /private/tmp/helix/working/BD1F09DF/w/A2D0090F/e/Templates/BaseFolder/AspNet.dvhepurhcgwo/Shared/bin/Release/net7.0/AspNet.dvhepurhcgwo.Shared.dll
AspNet.dvhepurhcgwo.Client -> /private/tmp/helix/working/BD1F09DF/w/A2D0090F/e/Templates/BaseFolder/AspNet.dvhepurhcgwo/Client/bin/Release/net7.0/AspNet.dvhepurhcgwo.Client.dll
AspNet.dvhepurhcgwo.Client (Blazor output) -> /private/tmp/helix/working/BD1F09DF/w/A2D0090F/e/Templates/BaseFolder/AspNet.dvhepurhcgwo/Client/bin/Release/net7.0/wwwroot
AspNet.dvhepurhcgwo.Server -> /private/tmp/helix/working/BD1F09DF/w/A2D0090F/e/Templates/BaseFolder/AspNet.dvhepurhcgwo/Server/bin/Release/net7.0/AspNet.dvhepurhcgwo.Server.dll
Optimizing assemblies for size may change the behavior of the app. Be sure to test after publishing. See: https://aka.ms/dotnet-illink
Compressing Blazor WebAssembly publish artifacts. This may take a while...
AspNet.dvhepurhcgwo.Server -> /private/tmp/helix/working/BD1F09DF/w/A2D0090F/e/Templates/BaseFolder/AspNet.dvhepurhcgwo/Server/bin/Release/net7.0/publish/

Expected: True
Actual: False

@javiercn @TanayParikh @MackinnonBuck any ideas? rerunning for now.

@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

As @captainsafia commented earlier in response to my "Who has a good understanding of these issues or the specific tests❔" comment, it appears our old dotnet seg fault problems have returned. But the scenarios might be narrower this time around. What is consistent between the latest failure and the ones I mentioned❔

@dsplaisted @marcpopMSFT who are the best contacts for dotnet seg faults❔ If our tests are doing something unexpected, please let us know.

@dougbu dougbu merged commit bcf7319 into main Aug 30, 2022
@dougbu dougbu deleted the mbuck/update-sdk-20220801 branch August 30, 2022 21:21
@ghost ghost added this to the 8.0-alpha1 milestone Aug 30, 2022
@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

Note we need to track fixes for the "exit code 139" and msbuild problems. We have a new SDK but builds of main will likely be less reliable until those problems are corrected.

@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

Put another way, #43581 tracks one problem we're going to see sporadically in main until something makes updates msbuild in the .NET SDK.

Separately, dotnet/runtime#68393 (or some of the issues it references) has regressed again or we've hit a new variant of the problem. @jkotas @rainersigwald @radical @lewing @bartonjs @vcsjones @karelz you were all involved in some of those dotnet seg fault / exit code 139 issues. Details mostly in the first attempt of the CI for this PR and comments above e.g.

We may need a new issue to track addressing this.

Thoughts❔

@jkotas
Copy link
Member

jkotas commented Aug 30, 2022

dotnet seg fault / exit code 139 issues

This is common failure mode with many different root causes. We need either crash dump or repro steps to make progress on it.

@ghost
Copy link

ghost commented Aug 30, 2022

Hi @jkotas. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

@jkotas there's a dump at https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-aspnetcore-refs-pull-43028-merge-9a8cf838a9654554a1/Templates.Mvc.Tests--net7.0/1/core.1000.19678?helixlogtype=result and likely one in the artifacts for the test(s) that failed during the first attempt at the last build of this PR.

@jkotas
Copy link
Member

jkotas commented Aug 30, 2022

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-aspnetcore-refs-pull-43028-merge-9a8cf838a9654554a1/Templates.Mvc.Tests--net7.0/1/core.1000.19678?helixlogtype=result

The stacktrace of the crash is:

libcoreclr!sigsegv_handler+0x260 [/__w/1/s/src/coreclr/pal/src/exception/signal.cpp @ 15732480] 
libpthread_2_27!_restore_rt
libpthread_2_27!_GI___pthread_rwlock_wrlock+0x12
libcrypto_so_1!CRYPTO_THREAD_write_lock+0x9
libcrypto_so_1!RAND_get_rand_method+0x33
libcrypto_so_1!RAND_bytes+0x10
libcrypto_so_1!EVP_MD_CTX_ctrl+0x132f
libcrypto_so_1!EVP_CIPHER_CTX_ctrl+0x17
libssl_so_1!SSL_in_before+0x13bd4
libssl_so_1!SSL_in_before+0x59ea
libssl_so_1!SSL_in_before+0x3d6
libssl_so_1!SSL_do_handshake+0x54
libSystem_Security_Cryptography_Native_OpenSsl!CryptoNative_SslDoHandshake+0x25 [/__w/1/s/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c @ 555] 

This is dup of dotnet/runtime#68393. It is bug in Ubuntu https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1983100 that is in the process of being fixed.

@ghost
Copy link

ghost commented Aug 30, 2022

Hi @jkotas. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

Will the problem automatically go away @jkotas if we migrate away from Ubuntu 18.04 build / test agents❔ I doubt that's viable in general (yet) but we at least plan to stop using that version for our container builds and where we use AzDO-hosted build agents.

@bartonjs
Copy link
Member

The only persistent SIGSEGV that I personally know about is unique to Ubuntu 18.04, and is caused by tools having background TLS connections still running when the process exits. Ubuntu has a bug on it (https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1983100), but it sadly hasn't hit an acceptable final release yet.

So... yes, moving off of Ubuntu 18.04 (or their fix finally rolling out) would probably take the segfaults away.

@ghost
Copy link

ghost commented Aug 30, 2022

Hi @bartonjs. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

In a separate bit of follow-up, I'm looking at including MSBUILDLOGALLENVIRONMENTVARIABLES in almost every job we submit. Trying to decide whether to target release/7.0 or main w/ that change…

@rainersigwald @Forgind and @baronfel, environment variables are still logged in full in recent release/7.0 builds. Is that likely to change as new msbuild bits get into the RC2 or RTM 7.0.0 SDKs or newer Visual Studio releases❔ If yes, I'll target release/7.0 proactively; otherwise, just add the workaround in main.

@Forgind
Copy link
Member

Forgind commented Aug 30, 2022

Yes; the change should flow with the new MSBuild, so if it's causing trouble for you, you should target release/7.0 with that.

We're also discussing the best way to communicate clearly if we make certain environment variables always logged. If you're interested, I'd be happy to add you to that conversation.

@ghost
Copy link

ghost commented Aug 30, 2022

Hi @Forgind. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

I'd be happy to add you to that conversation.

I probably don't have much more to add beyond my "regression" thoughts above but sure…

dougbu added a commit to dougbu/aspnetcore that referenced this pull request Aug 30, 2022
- should affect most of our pipelines though not a few post-build jobs
- as we saw in dotnet#43028, binary logs no longer include all environment variables by default
  - this change captures the environment variables not visibly referenced in our projects etc.
  - fix avoids a need to add the override later to debug an issue, at the cost of larger .binlog files
@dougbu
Copy link
Member

dougbu commented Aug 30, 2022

#43644 opened, targeting release/7.0. I'll flow automatically (modulo an expected merge conflict) into main.

dougbu added a commit that referenced this pull request Aug 31, 2022
…#43644)

- should affect most of our pipelines though not a few post-build jobs
- as we saw in #43028, binary logs no longer include all environment variables by default
  - this change captures the environment variables not visibly referenced in our projects etc.
  - fix avoids a need to add the override later to debug an issue, at the cost of larger .binlog files
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework blocked The work on this issue is blocked due to some dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.