Update SDK #47540

surayya-MS · 2023-04-03T15:09:17Z

…ew instance

surayya-MS · 2023-04-03T15:21:47Z

In the closed PR I changed only global.js file. There were a lot of exceptions saying "Use 'ArgumentOutOfRangeException.ThrowIfEqual' instead of explicitly throwing a new exception instance":
.https://github.com/dotnet/aspnetcore/pull/47537/files

I updated SDK and used ArgumentOutOfRangeException.ThrowIfEqual in this PR. Now checks are failing saying "'ArgumentOutOfRangeException' does not contain a definition for 'ThrowIfEqual'"

@BrennanConroy, @wtgodbe could you please help me?

BrennanConroy · 2023-04-03T15:53:12Z

Well ThrowIfEqual doesn't even exist yet...
dotnet/runtime#83853

We should ignore the rule in our .editorconfig file and file an issue to fix it when the api exists.

JamesNK · 2023-04-04T04:17:20Z

Well ThrowIfEqual doesn't even exist yet... dotnet/runtime#83853

We should ignore the rule in our .editorconfig file and file an issue to fix it when the api exists.

It looks like just one warning. IMO, suppress the warning in *.cs file with a pragma. That isolate suppression to just places that have a bad warning. Add a follow-up issue to re-enable once ThrowIfEqual is available. Link to issue from cs file.

BrennanConroy · 2023-04-04T15:29:55Z

Oh good, now Helix is using the broken version of runtime D:
dotnet/runtime#84009

surayya-MS · 2023-04-04T15:58:02Z

Oh good, now Helix is using the broken version of runtime D: dotnet/runtime#84009

Thanks a lot! I've been trying to figure out what the problem was.

BrennanConroy · 2023-04-04T18:06:07Z

...sting/Server.IntegrationTesting/src/Deployers/RemoteWindowsDeployer/RemoteWindowsDeployer.cs

@@ -123,7 +123,9 @@ public override void Dispose()
            }
            catch (Exception ex)
            {
+#pragma warning disable CA2017 // Parameter count mismatch
                Logger.LogWarning(0, "Failed to stop the server.", ex);


Why not fix it instead?
I assume it's supposed to be LogWarning(0, ex, "...")

I just tried out LogWarning(0, ex, "...") and there is no warning any more.

This is weird. Why doesn't Logger.LogWarning(0, "..", ex) work on this line but works on line 136? @BrennanConroy what do you think?

Because the warning is for {blah} arguments in the log string not matching the passed in parameters. On line 136 there is 1 argument expected in the log, and 1 argument passed in. Although, it's also wrong because the string is using $"". So we should fix it also. And looks like even more of the logs in this file are wrong.

Thanks! I will change it to LogWarning(0, ex, "...") then.

Could you please file an issue for fixing $""? You would describe it better than me since you have more context.

It's the same change as what you're going to do for this line, move ex to be first.
Unless the analyzer breaks in that case, and then we'll probably file an issue for the analyzer.

surayya-MS · 2023-04-04T18:08:22Z

After updating SDK to a newer version 8.0.100-preview.4.23204.1 the CA1512 suppression is no longer needed. It was the one causing warning in src/SignalR/common/Http.Connections/src/HttpConnectionDispatcherOptions.cs.

Now there is a new warning CA2017 "Number of parameters supplied in the logging message template do
not match the number of named placeholders." in \src\Hosting\Server.IntegrationTesting\src\Deployers\RemoteWindowsDep loyer\RemoteWindowsDeployer.cs. Fixed Logger.LogWarning by placing Exception before string message.

BrennanConroy · 2023-04-10T17:39:45Z

src/SignalR/common/Http.Connections/src/HttpConnectionDispatcherOptions.cs

@@ -108,10 +108,12 @@ public TimeSpan TransportSendTimeout
        get => _transportSendTimeout;
        set
        {
+#pragma warning disable CA1512 // Use ArgumentOutOfRangeException throw helper


Shouldn't this be fixable now since the APIs exist?

I tried this but it looks like they still haven't been added?

Oh that's annoying, you probably need an updated runtime as well. The APIs have definitely been added. (added 5 days ago)

Something's up with maestro, we haven't had a dependency PR opened here in 4 days. I'll post in FR (CC @dotnet/dnceng)

They're investigating (it's a timeout in darc), in the meantime I manually triggered the subscriptions.

maraf · 2023-04-27T17:13:26Z

@eerhardt since this appears to be 6.0 specific I think it is fine to quarantine it while we work on it

It's >= 6.0, the targets are shared.

amcasey · 2023-04-27T17:15:36Z

@eerhardt since this appears to be 6.0 specific I think it is fine to quarantine it while we work on it

It's >= 6.0, the targets are shared.

I'm picking up from @eerhardt on the aspnetcore side and I'm trying to build up some context. I think @maraf just answered my question but @lewing how did we conclude this was 6.0-specific?

amcasey · 2023-04-27T17:20:33Z

Does the publish run in parallel? I see 6 failures with the same path within 2s. Are the binlogs from tests published? I can't find them.

I think the template tests are run in parallel. At least, they muck with the port numbers to avoid conflicts, which you wouldn't expect to be necessary otherwise.

wtgodbe · 2023-04-27T17:24:37Z

Are the binlogs from tests published? I can't find them.

Unfortunately we don't get access to these, the template tests run from a temp dir and clean themselves up after execution

amcasey · 2023-04-27T17:25:41Z

Should we disable these tests (like what was done in #47920) to unblock the SDK update?

I can't tell from this thread that we think the failures are understood/acceptable enough that we'd want to sweep them under the rug by skipping several tests. Am I missing something? Is something important blocked on this PR or is this just a priority because of how far behind it is?

lewing · 2023-04-27T17:52:48Z

@eerhardt since this appears to be 6.0 specific I think it is fine to quarantine it while we work on it

It's >= 6.0, the targets are shared.

I'm picking up from @eerhardt on the aspnetcore side and I'm trying to build up some context. I think @maraf just answered my question but @lewing how did we conclude this was 6.0-specific?

There is a reason I asked other people to take a look, I just did a quick pass and clearly misread the targets. As far as disabling vs blocking do what you think is best, the right people are aware of the issue and looking into it.

maraf · 2023-04-27T17:53:11Z

I think the template tests are run in parallel. At least, they muck with the port numbers to avoid conflicts, which you wouldn't expect to be necessary otherwise.

This particular obj path is shown in 6 errors within 2s /datadisks/disk1/work/B95509FE/w/998E08AE/e/Templates/BaseFolder/AspNet.w0wr52oqxukq/Client/obj/Release/net8.0/blazor.publish.boot-extension.json. Does it mean the single test was run 6 times in parallel?

radical · 2023-04-27T18:02:03Z

/datadisks/disk1/work/B95509FE/w/998E08AE/e/Templates/BaseFolder/AspNet.w0wr52oqxukq/Client/obj/Release/net8.0/blazor.publish.boot-extension.json

What's the root of the test project here? Is the project called AspNet.*? And is that suffix generated per test run? If so, then that should isolate the various runs.

wtgodbe · 2023-04-27T18:06:15Z

@MackinnonBuck may be able to help with the template test failures

radical · 2023-04-27T18:22:55Z

I think the template tests are run in parallel. At least, they muck with the port numbers to avoid conflicts, which you wouldn't expect to be necessary otherwise.

This particular obj path is shown in 6 errors within 2s /datadisks/disk1/work/B95509FE/w/998E08AE/e/Templates/BaseFolder/AspNet.w0wr52oqxukq/Client/obj/Release/net8.0/blazor.publish.boot-extension.json. Does it mean the single test was run 6 times in parallel?

The different tests with the same failures have different project names AspNet.* (suffix differs)
- so they shouldn't be overlapping, even if they run in parallel
Looking at a single test, it complains about the issue only once. IIUC, this is happening during the nested publish for the Client project. Maybe the task was run during the regular publish too and did not close the file. But looking at https://github.com/dotnet/sdk/blob/880798d13942d41e61201e4281604560eade31e9/src/BlazorWasmSdk/Tasks/GenerateBlazorBootExtensionJson.cs#L25-L27 it doesn't seem to be the case. Unless I missed something.

maraf · 2023-04-27T18:32:43Z

https://github.com/dotnet/sdk/blob/880798d13942d41e61201e4281604560eade31e9/src/BlazorWasmSdk/Tasks/GenerateBlazorBootExtensionJson.cs#L25-L27 it doesn't seem to be the case. Unless I missed something.

The target that triggers it is referenced in two places

The first one has a guard for nested publish, but the second one added recently with compression doesn't have it.

radical · 2023-04-27T18:42:55Z

This file is also opened by GenerateWasmBootJson but is not disposed off:

https://github.com/dotnet/runtime/blob/0f06edefe1588d27de716af5805a20ca918c5f46/src/tasks/Microsoft.NET.Sdk.WebAssembly.Pack.Tasks/GenerateWasmBootJson.cs#L293-L295

var config = (Dictionary<string, object>)configSerializer.ReadObject(File.OpenRead(configExtension.ItemSpec));

maraf · 2023-04-27T18:52:38Z

Good catch!

radical · 2023-04-27T19:12:06Z

Preparing a fix in dotnet/runtime#85480 .

eerhardt · 2023-04-27T19:15:37Z

Since the fix is needed in runtime, we will have to wait for that to flow all the way to installer, and update the SDK (and take any new changes that came in the meantime). Would it make sense to disable the tests for now, until the next SDK update?

We have been trying to merge a new SDK in for a while. And need it to enable some AOT template tests.

radical · 2023-04-27T19:18:14Z

A related part of this would be to not run the target twice, which would be a good thing in general. And that should fix the failure here too, if our hypothesis is indeed correct.

radical · 2023-04-27T19:22:24Z

A related part of this would be to not run the target twice, which would be a good thing in general. And that should fix the failure here too, if our hypothesis is indeed correct.

Ignore me! That file is in dotnet/runtime now, so it would need the same flow!
I think we can disable the tests for now.

wtgodbe · 2023-04-27T19:23:52Z

Would it make sense to disable the tests for now, until the next SDK update?

I'm supportive, but we should file an issue & link it in the build-ops handoff issue

For dotnet#47933 to unblock dotnet#47540

lewing · 2023-04-27T23:34:51Z

Is there someone that can reproduce this locally to manually verify the fix?

ghost · 2023-04-27T23:34:56Z

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

Update SDK; use ArgumentOutOfRangeException.ThrowIfEqual instead of n…

23a34b6

…ew instance

surayya-MS requested review from BrennanConroy, halter73, a team and wtgodbe as code owners April 3, 2023 15:09

dotnet-issue-labeler bot added the area-signalr Includes: SignalR clients and servers label Apr 3, 2023

surayya-MS added 2 commits April 4, 2023 08:23

suppress CA1512

7a28ea2

suppress CA1512 with pragma

5c7771b

surayya-MS mentioned this pull request Apr 4, 2023

Enable CA1512 in HttpConnectionDispatcherOptions.cs #47553

Closed

Update global.json

9407844

surayya-MS and others added 2 commits April 4, 2023 19:49

Merge branch 'dotnet:main' into main

f7efa2e

remove suppression of CA1512; suppress CA2017

2aa4b1a

surayya-MS requested a review from Tratcher as a code owner April 4, 2023 18:03

BrennanConroy reviewed Apr 4, 2023

View reviewed changes

removed CA2017 suppression; fixed Logger.LogWarning

84dfd5e

surayya-MS requested review from JamesNK and mgravell as code owners April 4, 2023 18:44

surayya-MS changed the title ~~Update SDK; use ArgumentOutOfRangeException.ThrowIfEqual instead of n…~~ Update SDK Apr 4, 2023

JamesNK approved these changes Apr 5, 2023

View reviewed changes

MackinnonBuck added 2 commits April 10, 2023 09:55

Update SDK to 8.0.100-preview.4.23210.1

eae6ddd

Suppress CA1512 (again)

6219d14

BrennanConroy reviewed Apr 10, 2023

View reviewed changes

MackinnonBuck requested a review from a team as a code owner April 10, 2023 21:51

MackinnonBuck force-pushed the main branch from 49c082c to 6219d14 Compare April 10, 2023 21:53

radical mentioned this pull request Apr 27, 2023

[wasm] GenerateWasmBootJson - close file handles dotnet/runtime#85480

Merged

amcasey mentioned this pull request Apr 27, 2023

Quarantine BlazorWasmHostedTemplate_AzureActiveDirectoryTemplate_* #47933

Closed

amcasey added a commit to amcasey/aspnetcore that referenced this pull request Apr 27, 2023

Quarantine BlazorWasmHostedTemplate_AzureActiveDirectoryTemplate_*

6bbd048

For dotnet#47933 to unblock dotnet#47540

amcasey mentioned this pull request Apr 27, 2023

Quarantine BlazorWasmHostedTemplate_AzureActiveDirectoryTemplate_* #47934

Closed

Quarantine BlazorWasmHostedTemplate_AzureActiveDirectoryTemplate_*

01b5bf6

For dotnet#47933 to unblock dotnet#47540

eerhardt merged commit 1b78513 into dotnet:main Apr 27, 2023

ghost added this to the 8.0-preview5 milestone Apr 27, 2023

lewing mentioned this pull request Apr 27, 2023

[release/8.0-preview4] [wasm] GenerateWasmBootJson - close file handles dotnet/runtime#85482

Merged

eerhardt mentioned this pull request May 1, 2023

IDE0005 error when building with latest SDK #47932

Closed

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Update SDK #47540

Update SDK #47540

Conversation

surayya-MS commented Apr 3, 2023

surayya-MS commented Apr 3, 2023 • edited Loading

BrennanConroy commented Apr 3, 2023

JamesNK commented Apr 4, 2023

BrennanConroy commented Apr 4, 2023

surayya-MS commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surayya-MS commented Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maraf commented Apr 27, 2023

amcasey commented Apr 27, 2023

amcasey commented Apr 27, 2023

wtgodbe commented Apr 27, 2023

amcasey commented Apr 27, 2023

lewing commented Apr 27, 2023

maraf commented Apr 27, 2023

radical commented Apr 27, 2023

wtgodbe commented Apr 27, 2023

radical commented Apr 27, 2023

maraf commented Apr 27, 2023

radical commented Apr 27, 2023

maraf commented Apr 27, 2023

radical commented Apr 27, 2023

eerhardt commented Apr 27, 2023 • edited Loading

radical commented Apr 27, 2023

radical commented Apr 27, 2023

wtgodbe commented Apr 27, 2023

lewing commented Apr 27, 2023

ghost commented Apr 27, 2023

surayya-MS commented Apr 3, 2023 •

edited

Loading

surayya-MS commented Apr 4, 2023 •

edited

Loading

eerhardt commented Apr 27, 2023 •

edited

Loading