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 off-by-default analyzers to account for rename #31460

Merged
merged 13 commits into from
Mar 29, 2023

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Mar 27, 2023

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member labels Mar 27, 2023
@ghost
Copy link

ghost commented Mar 27, 2023

Thanks for your PR, @captainsafia.
To learn about the PR process and branching schedule of this repo, please take a look at the SDK PR Guide.

@javiercn
Copy link
Member

@ilonatommy is this something that was recently added? _framework\dotnet.timezones.blat

The PR is updating the SDK from preview1 to preview4 and we are trying to determine if there is a bug.

From my understanding, it should be ok to update the baselines as this is just an extra file. https://github.com/dotnet/sdk/blob/main/src/RazorSdk/update-test-baselines.ps1

@captainsafia
Copy link
Member Author

I updated the baselines in a31a53e but happy to revert if that ends up not being necessary.

Locally, I did observe that I was still seeing test failures related to _framework\dotnet.timezones.blat after updating the baselines but we'll see if they repro in CI.

@javiercn
Copy link
Member

javiercn commented Mar 27, 2023

@captainsafia I think looking at your commit made things clearer.

I believe @pavelsavara mentioned that this change was coming, but I'll let him confirm.

I somewhat understood the change as there was an extra file, not one less file. I believe the timezone info is now directly embedded into the wasm payload

@javiercn
Copy link
Member

@captainsafia the "additionally" failing tests might be tests that are not using baselines, it should be fine to just "remove" the assertion on those.

@captainsafia
Copy link
Member Author

@captainsafia the "additionally" failing tests might be tests that are not using baselines, it should be fine to just "remove" the assertion on those.

Quite right! Just pushed b0a0ea4 with this.

@ilonatommy
Copy link
Member

ilonatommy commented Mar 28, 2023

@ilonatommy is this something that was recently added? _framework\dotnet.timezones.blat

The PR is updating the SDK from preview1 to preview4 and we are trying to determine if there is a bug.

From my understanding, it should be ok to update the baselines as this is just an extra file. https://github.com/dotnet/sdk/blob/main/src/RazorSdk/update-test-baselines.ps1

On the contrary, dotnet.timezones.blat was there since a while already. Then, this PR: dotnet/runtime#82250 removed it and moved the data to be embedded into .wasm file.

Edit:
Pavel was planning further work on this and the status is in progress, see: dotnet/runtime#82250 (comment).

@maraf
Copy link
Member

maraf commented Mar 28, 2023

We were waiting till the change flows into the SDK

@javiercn
Copy link
Member

@ilonatommy @maraf just for the sake of completeness, it's correct that the file is being removed by the update, correct?

@maraf
Copy link
Member

maraf commented Mar 28, 2023

@ilonatommy @maraf just for the sake of completeness, it's correct that the file is being removed by the update, correct?

Yes, as Ilona mentioned, it is bundled in the wasm file. And we need to fix BlazorEnableTimeZoneSupport

@maraf
Copy link
Member

maraf commented Mar 28, 2023

@radekdoulik Do you have any idea why it picks emscripten 3.1.34?

https://dev.azure.com/dnceng-public/public/_build/results?buildId=218974&view=ms.vss-test-web.build-test-results-tab&runId=4098548&resultId=100001&paneView=attachments

EDIT I don't think there is or will be a working preview4 SDK build until dotnet/runtime#83998

@radekdoulik
Copy link
Member

radekdoulik commented Mar 28, 2023

@radekdoulik Do you have any idea why it picks emscripten 3.1.34?

The CI is using docker images with 3.1.34. dotnet/runtime#84014 should help

@maraf
Copy link
Member

maraf commented Mar 28, 2023

The CI is using docker images with 3.1.34. dotnet/runtime#84014 should help

The workload manifests for 3.1.34 are packed in the SDK, althou the runtime pack expects 3.1.30

@maraf
Copy link
Member

maraf commented Mar 28, 2023

@captainsafia Is it possible to use preview3?

@lewing
Copy link
Member

lewing commented Mar 28, 2023

@dsplaisted here we're picking up emsdk packs that are ahead of what is in runtime. I think we need the rollback file sooner rather than later.

@lewing
Copy link
Member

lewing commented Mar 28, 2023

@radekdoulik unless we expect to have the emsdk bump merged quickly we should probably roll back the version on emsdk main to unblock things

cc @marcpopMSFT

@captainsafia
Copy link
Member Author

@captainsafia Is it possible to use preview3?

Unfortunately not, this is in reaction to a change that was made to preview4.

@lewing
Copy link
Member

lewing commented Mar 28, 2023

I think the best move for the moment is to disable the AOT tests while we sort out how to make them work in any sane way here. This started breaking now because previously we were using much to old versions of the manifests for these tests and now the tests will pick up the very latest build of the manifest even if they haven't been integrated into their dependency which is also not correct.

@lewing
Copy link
Member

lewing commented Mar 28, 2023

@captainsafia the current aot errors will go away once dotnet/emsdk#325 lands and publishes to the dotnet8 feed

@captainsafia
Copy link
Member Author

@captainsafia the current aot errors will go away once dotnet/emsdk#325 lands and publishes to the dotnet8 feed

OK, I can skip the tests in this PR and open a follow-up issue to revert once that change has flowed through.

@captainsafia captainsafia marked this pull request as ready for review March 28, 2023 18:27
@captainsafia captainsafia requested review from a team as code owners March 28, 2023 18:27
@captainsafia captainsafia requested review from maraf, javiercn, ilonatommy and lewing and removed request for a team March 28, 2023 18:27
@captainsafia captainsafia enabled auto-merge (squash) March 28, 2023 18:28
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runtime related changes looks good to me 👍

The .blat file could also be removed from this test (but the test is not asserting anything...) https://github.com/dotnet/sdk/blob/712dfa7c9b21acda4cd4f55b02c8cb9c5200ce0e/src/Tests/Microsoft.NET.Sdk.BlazorWebAssembly.Tests/ComputeBlazorPublishAssetsTest.cs#L60-L75

We will update this test once we have a working solution

public void Build_WithBlazorEnableTimeZoneSupportDisabled_DoesNotCopyTimeZoneInfo()

@captainsafia captainsafia merged commit 381f5ba into main Mar 29, 2023
@captainsafia captainsafia deleted the safia/update-rdg-main branch March 29, 2023 09:16
@maraf
Copy link
Member

maraf commented Mar 29, 2023

I'm sorry, I didn't realize the PR will merged when I add approval..
TIL about this feature

@captainsafia
Copy link
Member Author

I'm sorry, I didn't realize the PR will merged when I add approval..

Ah yes, I did this intentionally as I was keen to get this change (it breaks the off-by-default support for the RequestDelegateGenerator).

Should I make a new issue to track this follow-up?

@maraf
Copy link
Member

maraf commented Mar 30, 2023

Should I make a new issue to track this follow-up?

@captainsafia it's ok, we'll do it once we have the solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React to RDG name change in SDK for preview4
6 participants