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 #49034

Merged
merged 7 commits into from
Jul 6, 2023
Merged

Update SDK #49034

merged 7 commits into from
Jul 6, 2023

Conversation

halter73
Copy link
Member

No description provided.

@halter73 halter73 requested review from a team and wtgodbe as code owners June 26, 2023 16:24
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jun 26, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jun 26, 2023
@ghost
Copy link

ghost commented Jun 26, 2023

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

@halter73
Copy link
Member Author

Error message
Assert.Equal() Failure
↓ (pos 0)
Expected: /images/red.png
Actual:   ~/images/red.png
↑ (pos 0)

Stack trace
   at Microsoft.AspNetCore.Mvc.FunctionalTests.HtmlGenerationTest.<HtmlGenerationWebSite_GeneratesExpectedResults_WithImageData>g__AssertImgElement|16_0(IElement imgElement, Dictionary`2 expectedAttributes) in /_/src/Mvc/test/Mvc.FunctionalTests/HtmlGenerationTest.cs:line 202
   at Microsoft.AspNetCore.Mvc.FunctionalTests.HtmlGenerationTest.HtmlGenerationWebSite_GeneratesExpectedResults_WithImageData() in /_/src/Mvc/test/Mvc.FunctionalTests/HtmlGenerationTest.cs:line 137
--- End of stack trace from previous location ---

https://dev.azure.com/dnceng-public/public/_build/results?buildId=321048&view=ms.vss-test-web.build-test-results-tab&runId=6585140&resultId=121888&paneView=debug

@dotnet/razor-compiler There seems to be an issue with the razor SDK somewhere between 8.0.100-preview.7.23321.23 and 8.0.100-preview.7.23325.3. I did a little investigation and found this corresponds to dotnet/razor@f7483d3...c3de764. f7483d386b01ec87d1a284817732c60711e242bf was only from last Wednesday, so that narrows it down a bit, but there are much older commits merged in there.

Looking at the decompilation from the above failing test, I noticed that when compiled with the new SDK, Views_HtmlGeneration_Home_Image no longer includes any reference to UrlResolutionTagHelper. Razor now just skipss straight to adding the ImageTagHelper.

<!-- Plain image tag -->
<img src="~/images/red.png" alt="Red block" title="&lt;the title>" id="1">

UrlResolutionTagHelper no longer referenced

[HtmlTargetElement("video", Attributes = "[poster^='~/']")]
public class UrlResolutionTagHelper : TagHelper

which makes me wonder if the razor compiler isn't picking up the [HtmlTargetElement("img", Attributes = "[src^='~/']", TagStructure = TagStructure.WithoutEndTag)] for some reason. That's the one I figure would apply here.

@jjonescz
Copy link
Member

jjonescz commented Jun 27, 2023

Thanks for the investigation @halter73, I can reproduce this. Seems it's caused by adding global:: to @addTagHelper directives in dotnet/razor#8857. One of the directives is for UrlResolutionTagHelper:

-@addTagHelper Microsoft.AspNetCore.Mvc.Razor.TagHelpers.UrlResolutionTagHelper, Microsoft.AspNetCore.Mvc.Razor
+@addTagHelper global::Microsoft.AspNetCore.Mvc.Razor.TagHelpers.UrlResolutionTagHelper, Microsoft.AspNetCore.Mvc.Razor

I would expect that to work. Will investigate and fix it or revert that part of the PR.

@halter73
Copy link
Member Author

We're now blocked on dotnet/installer#16864 before we can consume the razor fix in dotnet/razor#8871.

@Tratcher
Copy link
Member

Try again after #49113 (or its replacement) fixes e2e (now required).

@Tratcher
Copy link
Member

Tratcher commented Jul 5, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@javiercn
Copy link
Member

javiercn commented Jul 6, 2023

@dotnet/aspnet-build any thoughts about the issues here? There seem to be new warnings about globalization, but I'm not sure what action should we take.

@javiercn
Copy link
Member

javiercn commented Jul 6, 2023

@mavasani can you take a look? It's seems to be a bug that is blocking our update.

@mavasani
Copy link

mavasani commented Jul 6, 2023

@javiercn This update to CA1305 is coming from dotnet/roslyn-analyzers#6730, which fixed dotnet/roslyn-analyzers#6586. It seems the new CA1305 violations in the PR are indeed valid ones. You can attempt to fix the new violations, or suppress them in source with pragma suppressions/SuppressMessageAttributes.

@javiercn
Copy link
Member

javiercn commented Jul 6, 2023

@mavasani I'm seeing warnings like this:
image

- And update SDK again
@halter73 halter73 requested a review from JamesNK as a code owner July 6, 2023 17:45
@halter73 halter73 requested a review from mgravell as a code owner July 6, 2023 17:45
@halter73 halter73 enabled auto-merge (squash) July 6, 2023 18:58
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making the "easy" fix.

@halter73 halter73 merged commit a41c91c into main Jul 6, 2023
@halter73 halter73 deleted the halter73/sdk branch July 6, 2023 19:49
@ghost ghost added this to the 8.0-preview7 milestone Jul 6, 2023
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.

7 participants