Skip to content

Update sdk to v7.0.100-preview.3.22159.27#40353

Closed
pranavkm wants to merge 4 commits into
mainfrom
pranavkm-patch-1
Closed

Update sdk to v7.0.100-preview.3.22159.27#40353
pranavkm wants to merge 4 commits into
mainfrom
pranavkm-patch-1

Conversation

@pranavkm

Copy link
Copy Markdown
Contributor

No description provided.

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 22, 2022
@pranavkm
pranavkm marked this pull request as ready for review February 22, 2022 17:12
@pranavkm
pranavkm requested a review from a team as a code owner February 22, 2022 17:12
@pranavkm

Copy link
Copy Markdown
Contributor Author

Looks like some of the middleware tests are failing. I couldn't reproduce the failure locally, so I'm guessing it's something do with how we deploy tests to Helix.

@HaoK could you help here? Do we publish tests projects and then pack them using the SDK?

@HaoK

HaoK commented Feb 22, 2022

Copy link
Copy Markdown
Member

You can get the publish directory from this zip, maybe you can reproduce it from that: https://helixde8s23ayyeko0k025g8.blob.core.windows.net/helix-job-015dd8b5-a2e8-4d54-a2ec-5bd753ba4c4caa245e00b2c4f6e8a/087bdf6a-071b-4f07-9da7-1dab11da7fd4.zip

@HaoK

HaoK commented Feb 22, 2022

Copy link
Copy Markdown
Member

cc @dougbu in case some of his recent helix work affected anything potentially

@dougbu

dougbu commented Feb 22, 2022

Copy link
Copy Markdown
Contributor

I'm guessing it's something do with how we deploy tests to Helix.

cc @dougbu in case some of his recent helix work affected anything potentially

Is a file actually missing on the Helix agents❔ I ask because the aspnetcore-components-e2e failure looks quite real and I could imagine the StaticFiles test failures are related to whatever caused that -- likely a merge ordering issue.

More generally, @HaoK provided a useful link. You can also test using eng/scripts/RunHelix.ps1 to grab a binary log of the Helix submission and examine the publish/ folder as necessary. You may want to comment out the following to avoid an actual submission because that can spin for a long while then fail (lots to upload).

$env:BUILD_REASON="PullRequest"
$env:BUILD_SOURCEBRANCH="local"
$env:BUILD_REPOSITORY_NAME="aspnetcore"
$env:SYSTEM_TEAMPROJECT="aspnetcore"

@pranavkm

Copy link
Copy Markdown
Contributor Author

Looks like a runtime regression. Filed dotnet/runtime#65750 to track resolving it.

@halter73

halter73 commented Mar 4, 2022

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@halter73

halter73 commented Mar 5, 2022

Copy link
Copy Markdown
Member

aspnetcore-components-e2e is still failing with the following:

/home/vsts/work/1/s/src/Components/WebAssembly/testassets/Wasm.Authentication.Client/Pages/Authentication.razor(8,48): error CS1503: Argument 2: cannot convert from 'method group' to 'EventCallback' [/home/vsts/work/1/s/src/Components/WebAssembly/testassets/Wasm.Authentication.Client/Wasm.Authentication.Client.csproj]

@javiercn Do you have any idea what could be happening here?

@captainsafia

Copy link
Copy Markdown
Contributor

It looks like this happens because the Razor compiler is now generating a type that is globally qualified twice.

#nullable disable
            ));
            __builder.AddAttribute(2, "OnLogInSucceeded", global::Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers.TypeCheck<**global::global::**Microsoft.AspNetCore.Components.EventCallback<Wasm.Authentication.Client.RemoteAppState>>(global::Microsoft.AspNetCore.Components.EventCallback.Factory.Create(this, 
#nullable restore

@pranavkm
pranavkm requested a review from dougbu as a code owner March 7, 2022 22:41
@javiercn

javiercn commented Mar 8, 2022

Copy link
Copy Markdown
Member

FYI, I'm working on the compiler fix, it's turning out to be a bit tricky.

@TanayParikh

Copy link
Copy Markdown
Contributor

working on the compiler fix

Just cross-linking Javier's compiler fix: dotnet/razor-compiler#88

@javiercn

javiercn commented Mar 9, 2022

Copy link
Copy Markdown
Member

Fix is merged on the razor-compiler side, so it'll have to trickle up to here

@TanayParikh

TanayParikh commented Mar 9, 2022

Copy link
Copy Markdown
Contributor

Fix is merged on the razor-compiler side, so it'll have to trickle up to here

Thanks Javier, I've triggered the subscription from razor-compiler -> sdk. Will monitor and update this PR with the updated SDK version once that dep update PR is merged.

Edit: Actually will have to wait till the official build passes before I can kick off the dep update: https://dev.azure.com/dnceng/internal/_build/results?buildId=1654348&view=results

Edit 2: dotnet/sdk#24300 consumes dotnet/razor-compiler#88 on SDK

Edit 3: razor-compiler->sdk PR merged, official main branch build is here: https://dev.azure.com/dnceng/internal/_build/results?buildId=1655041&view=results

@TanayParikh TanayParikh changed the title Update sdk to v7.0.100-preview.3.22122.1 Update sdk to v7.0.100-preview.3.22159.14 Mar 9, 2022
@TanayParikh

Copy link
Copy Markdown
Contributor

Updated SDK to reflect artifacts produced by https://dev.azure.com/dnceng/internal/_build/results?buildId=1655041&view=results. There's a chance this still fails due to the artifacts not having fully propagated yet.

@TanayParikh
TanayParikh enabled auto-merge (squash) March 10, 2022 00:00
@wtgodbe

wtgodbe commented Mar 10, 2022

Copy link
Copy Markdown
Member

@TanayParikh that's a build of dotnet/sdk, you need to wait for it to flow through to dotnet/installer then use the SDK produced by that build

@TanayParikh

Copy link
Copy Markdown
Contributor

@TanayParikh that's a build of dotnet/sdk, you need to wait for it to flow through to dotnet/installer then use the SDK produced by that build

Ah okay, thanks for the clarification. In that case, it'll be dotnet/installer#13375.

@TanayParikh TanayParikh changed the title Update sdk to v7.0.100-preview.3.22159.14 Update sdk to v7.0.100-preview.3.22159.27 Mar 10, 2022
@TanayParikh

TanayParikh commented Mar 10, 2022

Copy link
Copy Markdown
Contributor

Middleware tests are still failing unfortunately, I believe the fix for that was dotnet/runtime#65886. cc/ @jozkee @carlossanlop any ideas for what may be going on here?

The components-e2e pipeline now passes so that should be indicative of us consuming Javier's compiler fix via v7.0.100-preview.3.22159.27.

@jozkee

jozkee commented Mar 10, 2022

Copy link
Copy Markdown
Member

I believe the fix for that was dotnet/runtime#65886. cc/ @jozkee @carlossanlop any ideas for what may be going on here?

That fix was for external zip readers that don't use utf-8 by default to decode unicode filenames. .NET's ZipArchive uses utf-8 by default, so https://github.com/dotnet/runtime/pull/ 65886 doesn't even have a functional impact on how zips are read.

It is weird that the tests are failing becuase the filename is missing and I don't see how that can be ZipArchive's fault.

[xUnit.net 00:00:02.20]     Microsoft.AspNetCore.StaticFiles.DefaultFilesMiddlewareTests.FoundDirectoryWithDefaultFile_PathModified_All(baseUrl: "", baseDir: "./SubFolder", requestUrl: "/你好", appendTrailingSlash: False) [FAIL]
[xUnit.net 00:00:02.21]       Assert.Equal() Failure
[xUnit.net 00:00:02.21]                    ↓ (pos 3)
[xUnit.net 00:00:02.22]       Expected: /你好/default.html
[xUnit.net 00:00:02.22]       Actual:   /你好
[xUnit.net 00:00:02.23]                    ↑ (pos 3)
[xUnit.net 00:00:02.26]       Stack Trace:
[xUnit.net 00:00:02.26]         /_/src/Middleware/StaticFiles/test/UnitTests/DefaultFilesMiddlewareTests.cs(181,0): at Microsoft.AspNetCore.StaticFiles.DefaultFilesMiddlewareTests.FoundDirectoryWithDefaultFile_PathModified(String baseUrl, String baseDir, String requestUrl, Boolean appendTrailingSlash)
[xUnit.net 00:00:02.27]         /_/src/Middleware/StaticFiles/test/UnitTests/DefaultFilesMiddlewareTests.cs(138,0): at Microsoft.AspNetCore.StaticFiles.DefaultFilesMiddlewareTests.FoundDirectoryWithDefaultFile_PathModified_All(String baseUrl, String baseDir, String requestUrl, Boolean appendTrailingSlash)
[xUnit.net 00:00:02.27]         --- End of stack trace from previous location ---

@TanayParikh

Copy link
Copy Markdown
Contributor

@HaoK, @dougbu, @wtgodbe does anyone have context in this area? The components-e2e failures are now resolved by the middleware static files issues persist. dotnet/runtime#65750 has also been marked resolved via dotnet/runtime#65886.

@HaoK

HaoK commented Mar 10, 2022

Copy link
Copy Markdown
Member

I don't have context other than that's the only change in the zip area, and pranav was able to reproduce the issue, helix clearly behaves as expected prior to their change, so unless they have some other explaination for why the helix payload is mangled now, its clearly a regression

@HaoK

HaoK commented Mar 10, 2022

Copy link
Copy Markdown
Member

dotnet/runtime#65750 has a good summary and all the context

@wtgodbe

wtgodbe commented Mar 10, 2022

Copy link
Copy Markdown
Member

Something must have changed in runtime to cause the new behavior - whoever owns those tests should take a look and see if it's a bug, or a real behavioral change that will necessitate a reaction on our part (test update or src update)

@HaoK

HaoK commented Mar 10, 2022

Copy link
Copy Markdown
Member

I would reopen the issue since the helix issue isn't fixed, they might have fixed some comment issue, but clearly the e2e doesn't work on helix still

@HaoK

HaoK commented Mar 10, 2022

Copy link
Copy Markdown
Member

But we can Quarantine the test so we don't lose track of this regression and unblock the sdk update if that's the last thing holding it up

@HaoK

HaoK commented Mar 10, 2022

Copy link
Copy Markdown
Member

@jozkee did you try the repro in https://github.com/pranavkm/zip-file with the fix to verify that the helix usage works now?

@carlossanlop

Copy link
Copy Markdown
Contributor

@wtgodbe has the runtime fix been ingested by the aspnet repo?

@wtgodbe

wtgodbe commented Mar 10, 2022

Copy link
Copy Markdown
Member

Ah, that'll do it - we should combine this PR with #40507

@jozkee

jozkee commented Mar 10, 2022

Copy link
Copy Markdown
Member

@HaoK, yes, I used 7zip and winzip to verify the changes. Also added a unit test in dotnet/runtime.

@wtgodbe

wtgodbe commented Mar 10, 2022

Copy link
Copy Markdown
Member

Updated the SDK in #40507, if that works we can close this PR

@TanayParikh

Copy link
Copy Markdown
Contributor

Looks like we're making progress in #40507. Closing this out.

auto-merge was automatically disabled March 10, 2022 19:54

Pull request was closed

@TanayParikh
TanayParikh deleted the pranavkm-patch-1 branch March 10, 2022 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.