search

dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
34.59k stars 9.79k forks source link

Update SDK #43028

Closed MackinnonBuck closed 1 year ago

ghost commented 1 year ago

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

MackinnonBuck commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

cc @dotnet/source-build-internal

dougbu commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 3 pipeline(s).
Tratcher commented 1 year ago

https://github.com/dotnet/arcade/issues/10311#issuecomment-1209629471

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 commented 1 year ago

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

TanayParikh commented 1 year ago

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

TanayParikh commented 1 year ago

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 commented 1 year ago

should now have everything we need

😞 unfortunately not. Source build still failing.

TanayParikh commented 1 year ago

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 commented 1 year ago

Looks like we have a new breaking change now:

I just discovered this in source-build and logged an issue for it - https://github.com/dotnet/aspnetcore/issues/43253

JamesNK commented 1 year ago

I've added a possible fix for the warning - https://github.com/dotnet/aspnetcore/pull/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 commented 1 year ago

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

JamesNK commented 1 year ago

@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 commented 1 year ago

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

sebastienros commented 1 year ago

@vitek-karas there is the suppression but we are still seeing errors. I did check that no suppression is overridden. Example: https://github.com/dotnet/aspnetcore/pull/43028/checks?check_run_id=7864889493

javiercn commented 1 year ago

@vitek-karas any update here?

javiercn commented 1 year ago

@sbomer there seems to be an ILinker issue that is triggering warnings despite suppresions, could you take a look at it as per https://github.com/dotnet/aspnetcore/pull/43028#issuecomment-1217090062?

sbomer commented 1 year ago

Would you be able to share a binlog or repro instructions of the case where it is warning even when NoWarn has IL2121?

sbomer commented 1 year ago

I do see a few places where the ILLink task is being called directly without passing in the NoWarn argument: https://github.com/dotnet/aspnetcore/blob/197c1693d3c830af52b587e8d88891bc9689be44/src/Tools/LinkabilityChecker/LinkabilityChecker.csproj#L47-L55 https://github.com/dotnet/aspnetcore/blob/197c1693d3c830af52b587e8d88891bc9689be44/src/Components/WebAssembly/testassets/WasmLinkerTest/WasmLinkerTest.csproj#L49-L57

These invocations should probably do what the SDK does, if they are intended to work with NoWarn: https://github.com/dotnet/linker/blob/696c2166078b1a70f12407840c3ab0f90d73211b/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets#L150

sebastienros commented 1 year ago

@sbomer thanks, I updated these files with NoWarn="$(NoWarn)", let's see if that helps.

tlakollo commented 1 year ago

I can see that there are no linker warnings being shown in the output, but the installer script is failing with a no-so-useful message

sebastienros commented 1 year ago

The ILLINK warnings are gone, thanks

sebastienros commented 1 year ago

@MichaelSimons I see you created https://github.com/dotnet/aspnetcore/issues/43253 for source build issues, do you know if there is something we can do in this PR to get it building? Wait/update to a new sdk?

MichaelSimons commented 1 year ago

@sebastienros, Regarding https://github.com/dotnet/aspnetcore/issues/43253, that was specific to the linker issue that I see was already addressed in this PR.

In regards to the failing source-build CI leg, the symptoms appear to be similar to the original issue reported in https://github.com/dotnet/arcade/issues/10311. Perhaps @MattGal can comment on the situation.

vitek-karas commented 1 year ago

@sebastienros regarding the reported unnecessary suppression - it's complaining about this suppression: https://github.com/dotnet/aspnetcore/blob/61e1572ad518c82f6fd487e65852e41bccde1bc1/src/Components/Components/src/Microsoft.AspNetCore.Components.WarningSuppressions.xml#L64

We've improved the linker to "blame" the XML file in a case like this, but the change probably didn't make it into this repo yet: https://github.com/dotnet/linker/commit/9332578bfd67d8b1e53a3cbf9e0f92d5af7186ac

dougbu commented 1 year ago

@vitek-karas should I remove the suppression in the meantime❔

vitek-karas commented 1 year ago

@dougbu if you want to, but with suppression of IL2121 there's nothing to do. Eventually we should go over all of the IL2121 warnings and clean it up. Basically aspnetcore equivalent of https://github.com/dotnet/runtime/pull/73238 in runtime.

The linker change will only produce the warning pointing to the XML file, but it will still produce the warning (just makes it easier to find).

/cc @jkurdek

MattGal commented 1 year ago

@sebastienros, Regarding dotnet/runtime#43253, that was specific to the linker issue that I see was already addressed in this PR.

In regards to the failing source-build CI leg, the symptoms appear to be similar to the original issue reported in dotnet/arcade#10311. Perhaps @MattGal can comment on the situation.

Yes, it would seem you need a newer SDK to move past that. The only other workaround would be to set DOTNET_CLI_DO_NOT_USE_MSBUILD_SERVER=true in the environment everywhere this build might need to work, which I think is undesirable.

dougbu commented 1 year ago

@JamesNK please see @vitek-karas's comments at https://github.com/dotnet/aspnetcore/pull/43028#issuecomment-1221610190 and later. I suspect you know how best to do something like dotnet/runtime#73238 in this repo. In any case, you're on build ops tomorrow…

JamesNK commented 1 year ago

It shouldn't be required because I already added a suppression for the warning.

    <!-- don't warn about unnecessary trim warning suppressions. can be removed with preview 6. -->
    <NoWarn>$(NoWarn);IL2121</NoWarn>

The NoWarn fixed all the failing builds except for aspnetcore-ci (Build Source-Build (Managed)). The error message from it doesn't give an indication about why it is failing.

Removing unnecessary suppressions is cleanup for the future. There are dozens. It will touch a lot of code and will take a bit of time.

dougbu commented 1 year ago

Since aspnetcore-components-e2e and the source-build job are still failing, we need something in this branch.

In any case, @JamesNK I thought the recent comments / issues were about redundant warning suppressions❔

JamesNK commented 1 year ago

Yes, there are dozens of redundant warning suppressions. They showed up in builds before I added the NoWarn.

aspnetcore-components-e2e is failing because the SDK download is getting a 404 - https://dev.azure.com/dnceng/public/_build/results?buildId=1960417&view=logs&j=fe94c0c9-bb8c-5d6f-3b51-887173cc2f5c&t=7877c2d9-a5b1-59a3-dc2b-0d7e9d9db055

dougbu commented 1 year ago

@JamesNK we've seen problems w/ Arcade retries lately. The initial 404s for the SDK itself should be ignored (looks like it was) and the actual failure seems later (when Arcade downloads runtime versions). I asked about this in the First Responders channel.

/cc @dotnet/dnceng in case you want to respond here instead of FR

dkurepa commented 1 year ago

As Matt mentioned above, it looks like it's a bug with the SDK, updating to a newer version should fix the dotnet-install issues. (based on the discussions in https://github.com/dotnet/arcade/issues/10311)

JamesNK commented 1 year ago

@dkurepa This PR is currently updating the SDK to 7.0.100-rc.2.22419.24. CI builds continue to show download errors when trying to acquire the SDK:

Attempting to install dotnet from public_location.
dotnet-install: Note that the intended use of this script is for Continuous Integration (CI) scenarios, where:
dotnet-install: - The SDK needs to be installed without user interaction and without admin rights.
dotnet-install: - The SDK installation doesn't need to persist across multiple CI runs.
dotnet-install: To set up a development environment or to run apps, use installers rather than this script. Visit https://dotnet.microsoft.com/download to get the installer.

dotnet-install: Attempting to download using primary link https://dotnetcli.azureedge.net/dotnet/Sdk/7.0.100-rc.2.22419.24/dotnet-sdk-7.0.100-rc.2.22419.24-linux-x64.tar.gz
curl: (22) The requested URL returned error: 404 
dotnet-install: The resource at primary link 'https://dotnetcli.azureedge.net/dotnet/Sdk/7.0.100-rc.2.22419.24/dotnet-sdk-7.0.100-rc.2.22419.24-linux-x64.tar.gz' is not available.
dotnet-install: Attempting to download using legacy link https://dotnetcli.azureedge.net/dotnet/Sdk/7.0.100-rc.2.22419.24/dotnet-dev-ubuntu.18.04-x64.7.0.100-rc.2.22419.24.tar.gz
curl: (22) The requested URL returned error: 404 
dotnet-install: The resource at legacy link 'https://dotnetcli.azureedge.net/dotnet/Sdk/7.0.100-rc.2.22419.24/dotnet-dev-ubuntu.18.04-x64.7.0.100-rc.2.22419.24.tar.gz' is not available.
dotnet-install: Attempting to download using primary link https://dotnetbuilds.azureedge.net/public/Sdk/7.0.100-rc.2.22419.24/dotnet-sdk-7.0.100-rc.2.22419.24-linux-x64.tar.gz
dotnet-install: Extracting zip from https://dotnetbuilds.azureedge.net/public/Sdk/7.0.100-rc.2.22419.24/dotnet-sdk-7.0.100-rc.2.22419.24-linux-x64.tar.gz
dotnet-install: Installed version is 7.0.100-rc.2.22419.24
dotnet-install: Adding to current process PATH: `/home/vsts/work/1/s/.dotnet`. Note: This change will be visible only when sourcing script.
dotnet-install: Note that the script does not resolve dependencies during installation.
dotnet-install: To check the list of dependencies, go to https://docs.microsoft.com/dotnet/core/install, select your operating system and check the "Dependencies" section.
dotnet-install: Installation finished successfully.
  Determining projects to restore...
  Restored /home/vsts/.nuget/packages/microsoft.dotnet.arcade.sdk/7.0.0-beta.22419.1/tools/Tools.proj (in 5.4 sec).
  Determining projects to restore...
  Restored /home/vsts/work/1/s/eng/tools/GenerateFiles/GenerateFiles.csproj (in 1.4 sec).
  GenerateFiles -> /home/vsts/work/1/s/artifacts/bin/GenerateFiles/Directory.Build.props
  GenerateFiles -> /home/vsts/work/1/s/artifacts/bin/GenerateFiles/Directory.Build.targets
  GenerateFiles -> /home/vsts/work/1/s/.config/dotnet-tools.json
/home/vsts/.nuget/packages/microsoft.dotnet.arcade.sdk/7.0.0-beta.22419.1/tools/InstallDotNetCore.targets(15,5): error : dotnet-install failed [/home/vsts/.nuget/packages/microsoft.dotnet.arcade.sdk/7.0.0-beta.22419.1/tools/Tools.proj]
Build failed with exit code 1. Check errors above.

What SDK version is this fixed in?

dougbu commented 1 year ago

Workaround may be to add DOTNET_CLI_DO_NOT_USE_MSBUILD_SERVER: true just below COMPlus_DbgMiniDumpName: ... in three places, starting at https://github.com/dotnet/aspnetcore/blob/f1353bd0de37a817f9f6acf22451ccd2d5c6a462/.azure/pipelines/jobs/default-build.yml#L226. This was mentioned recently but I didn't get to it.

dougbu commented 1 year ago

Added the hack I mentioned in latest commit.

@JamesNK please file an issue to remove these !temporary! lines when we can.

/fyi @rainersigwald @rokonec you'll need @MattGal's simple repro instead of building this entire repo to see the runtime download issue 😉

MattGal commented 1 year ago

/fyi @rainersigwald @rokonec you'll need @MattGal's simple repro instead of building this entire repo to see the runtime download issue 😉

It actually only takes about 10 minutes to do the "real" scenario repro too with docker setup. I'll make sure @rokonec has a usable one, one version or another.

JamesNK commented 1 year ago

The current state is all checks are passing except (Build Source-Build (Managed)). Who is handling fixing it?

MattGal commented 1 year ago

The current state is all checks are passing except (Build Source-Build (Managed)). Who is handling fixing it?

This is an actual bug in the copy of MSBuild included with the SDK update, so it's probably still not a desirable SDK update to merge to your repo. As of yesterday I think @rokonec is working on the fix.

dougbu commented 1 year ago

This is an actual bug in the copy of MSBuild included with the SDK update, so it's probably still not a desirable SDK update to merge to your repo

Not sure I agree because we're talking about the 'main' branch and some of our team are working on 8.0.0 features that need a newer SDK. Would rather expand the DOTNET_CLI_DO_NOT_USE_MSBUILD_SERVER: true thing short term. Could you try that @captainsafia❔

Basically, my https://github.com/dotnet/aspnetcore/pull/43028/commits/ac92df5a430f654555e499aedbbb8562e5cc1538 commit didn't affect source-build but should help everywhere else. Need a similar change in the source-build job in ci.yml.

MattGal commented 1 year ago

@dougbu there's a workaround in Arcade here: https://github.com/dotnet/arcade/pull/10585, that should allow you to use any of the SDKs with the breaking change without setting env vars.

captainsafia commented 1 year ago

@dougbu there's a workaround in Arcade here: https://github.com/dotnet/arcade/pull/10585, that should allow you to use any of the SDKs with the breaking change without setting env vars.

I noticed that you posted this comment (https://github.com/dotnet/arcade/pull/10585#issuecomment-1227742528) on the PR. I assume you'll be doing the validation for the fix on this branch?

dougbu commented 1 year ago

possible (I am not looking at the moment) the Arcade update in release/7.0 hasn't flown into main yet. also possible it didn't include your latest fix @MattGal

MattGal commented 1 year ago

possible (I am not looking at the moment) the Arcade update in release/7.0 hasn't flown into main yet. also possible it didn't include your latest fix @MattGal

Nothing is merged into Arcade's main branch yet. Locally https://github.com/dotnet/arcade/pull/10585 looks like the right fix, but I'm working through the other breaks in this PR now. What is clear is that it gets a little past the previous repro then is using some not-the-root nuget.config file that doesn't have the general-testing feed. I'm trying sticking this feed into every nuget.config I find now.

MattGal commented 1 year ago

I think the latest commit in https://github.com/dotnet/arcade/pull/10585 does the trick. I'm trying out just the SDK update from this PR and that arcade update in this draft PR: https://github.com/dotnet/aspnetcore/pull/43568

captainsafia commented 1 year ago

I think the latest commit in dotnet/arcade#10585 does the trick. I'm trying out just the SDK update from this PR and that arcade update in this draft PR: dotnet/runtime#43568

@MattGal Any success with the trial? I saw the draft PR was closed but wasn't sure of the outcome.