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 branding to 7.0.1 #44768

Merged

Conversation

vseanreesermsft
Copy link
Contributor

No description provided.

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 27, 2022
@wtgodbe
Copy link
Member

wtgodbe commented Oct 27, 2022

Is it time for this already @vseanreesermsft?

- change label to "servicing"
- check in 7.0.0 PackageOverrides.txt and PlatformManifest.txt files
@dougbu
Copy link
Member

dougbu commented Nov 1, 2022

Looks like we're missing a System.Text.Json.Serialization fix in this branch e.g. from local debugging of StatusCodeSampleTest.StatusCodePage_ProducesProblemDetails():

System.MissingMethodException: Method not found: 'Void System.Text.Json.Serialization.Metadata.JsonTypeInfo.MakeReadOnly()'.
   at Microsoft.AspNetCore.Http.DefaultProblemDetailsWriter.ProblemDetailsJsonContext.Create_ProblemDetails(JsonSerializerOptions options, Boolean makeReadOnly)
   at Microsoft.AspNetCore.Http.DefaultProblemDetailsWriter.ProblemDetailsJsonContext.get_ProblemDetails() in C:\\dd\\dnx\\aspnetcore\\src\\Http\\Http.Extensions\\src\\System.Text.Json.SourceGeneration\\System.Text.Json.SourceGeneration.JsonSourceGenerator\\ProblemDetailsJsonContext.ProblemDetails.g.cs:line 23
   at Microsoft.AspNetCore.Http.DefaultProblemDetailsWriter.WriteAsync(ProblemDetailsContext context) in C:\\dd\\dnx\\aspnetcore\\src\\Http\\Http.Extensions\\src\\DefaultProblemDetailsWriter.cs:line 57
   at Microsoft.AspNetCore.Http.ProblemDetailsService.WriteAsync(ProblemDetailsContext context) in C:\\dd\\dnx\\aspnetcore\\src\\Http\\Http.Extensions\\src\\ProblemDetailsService.cs:line 37
   at Microsoft.AspNetCore.Builder.StatusCodePagesOptions.<>c.<<-ctor>b__0_0>d.MoveNext() in C:\\dd\\dnx\\aspnetcore\\src\\Middleware\\Diagnostics\\src\\StatusCodePage\\StatusCodePagesOptions.cs:line 29
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context) in C:\\dd\\dnx\\aspnetcore\\src\\Middleware\\Diagnostics\\src\\StatusCodePage\\StatusCodePagesMiddleware.cs:line 71
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context) in C:\\dd\\dnx\\aspnetcore\\src\\Middleware\\Diagnostics\\src\\DeveloperExceptionPage\\DeveloperExceptionPageMiddlewareImpl.cs:line 98","headers":{"Accept":["application/json"],"Host":["localhost"]},"path":"/","endpoint":null,"routeValues":null}]

The missing method isn't mentioned at all in this repo and DefaultProblemDetailsWriter.ProblemDetailsJsonContext inherits from JsonSerializerContext w/ only [JsonSerializable(typeof(ProblemDetails))] added.

@dotnet/area-system-text-json any suggestions on a workaround or fix❔

@dougbu
Copy link
Member

dougbu commented Nov 1, 2022

Also seeing a lot of the following in slightly lower-level tests:

System.MissingMethodException : Method not found: 'Void System.Text.Json.Serialization.Metadata.JsonTypeInfo.MakeReadOnly()'

@wtgodbe
Copy link
Member

wtgodbe commented Nov 1, 2022

Looks like that was added here: dotnet/runtime#76899. I suspect this might not work until we get the dependency updates included in the internal -> public merge

@dougbu
Copy link
Member

dougbu commented Nov 2, 2022

I suspect this might not work until we get the dependency updates included in the internal -> public merge

Why❔ Aren't we doing dependency flow in public now that we have a 7.0.0 build❔ Needing to wait for the internal ➡️ public merge before even branding can merge will likely cause a mad rush on GA day ☹️

Can we at least manually trigger the public runtime ➡️ aspnetcore flow to get the fix for the missing methods here❔ I could also darc update-dependencies in this branch if that's appropriate (once I find or learn the right BAR id of course).

/cc @vseanreesermsft @dkurepa @mmitche

@wtgodbe
Copy link
Member

wtgodbe commented Nov 2, 2022

Why❔ Aren't we doing dependency flow in public now that we have a 7.0.0 build❔

We do every servicing release from the internal repos - worst case scenario, if we do have to wait until GA day, we get an extra week before branches close for 7.0.1 (GA 11/8, branches close 11/14 EOD), so we could make that work. I'll leave it up to @mmitche on whether it's appropriate to do a pre-emptive update-dependencies here

@dougbu
Copy link
Member

dougbu commented Nov 2, 2022

If it matters, dotnet-runtime had a few successful builds after dotnet/runtime#76899 merged:

#20221018.4 failed but is the closest match for the chosen GA build. We could rerun failed jobs in that one to create a public build w/ most of the 7.0.0 content.

@wtgodbe
Copy link
Member

wtgodbe commented Nov 2, 2022

I can't think of a good reason not to do an update-dependencies here for one of the builds you linked. I don't think "closest match" matters, I'd just go with 153194. It does mean we'll have to resolve a merge conflict in the internal->public merge, and therefore do it manually, but the tradeoff seems worth it to me.

@dougbu
Copy link
Member

dougbu commented Nov 2, 2022

but the tradeoff seems worth it to me.

👍 will do (after lunch)

@dougbu
Copy link
Member

dougbu commented Nov 2, 2022

Something is very wrong here. We're already at #20221013.12, BAR id 153194 in this branch and @eiriktsarpalis's dotnet/runtime#76899 change was included in that build (see https://github.com/dotnet/runtime/commits/cd2d83798383716204eb580eb5c89ef5b73b8ec2). Are we doing something wrong here @jeffhandley @carlossanlop @eiriktsarpalis

Again, the problems are Method not found: 'Void System.Text.Json.Serialization.Metadata.JsonTypeInfo.MakeReadOnly()' all over the place though we make no direct use of JsonTypeInfo and definitely don't call MakeReadOnly().

@dougbu
Copy link
Member

dougbu commented Nov 2, 2022

Another, relatively easy stack trace to use

   at Microsoft.AspNetCore.StaticWebAssets.ManifestStaticWebAssetFileProvider.SourceGenerationContext.Create_StaticWebAssetManifest(JsonSerializerOptions options, Boolean makeReadOnly)
   at Microsoft.AspNetCore.StaticWebAssets.ManifestStaticWebAssetFileProvider.SourceGenerationContext.get_StaticWebAssetManifest() in /_/src/Hosting/Hosting/src/System.Text.Json.SourceGeneration/System.Text.Json.SourceGeneration.JsonSourceGenerator/SourceGenerationContext.StaticWebAssetManifest.g.cs:line 23
   at Microsoft.AspNetCore.StaticWebAssets.ManifestStaticWebAssetFileProvider.StaticWebAssetManifest.Parse(Stream manifest) in /_/src/Shared/StaticWebAssets/ManifestStaticWebAssetFileProvider.cs:line 325
   at Microsoft.AspNetCore.Hosting.Tests.StaticWebAssets.ManifestStaticWebAssetsFileProviderTest.ParseWorksWithNodesThatOnlyDifferOnCasing(String path, Boolean exists) in /_/src/Hosting/Hosting/test/StaticWebAssets/ManifestStaticWebAssetsFileProviderTests.cs:line 102
   at InvokeStub_ManifestStaticWebAssetsFileProviderTest.ParseWorksWithNodesThatOnlyDifferOnCasing(Object, Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

In this case, SourceGenerationContext is a bit more complicated:

    [JsonSourceGenerationOptions]
    [JsonSerializable(typeof(StaticWebAssetManifest))]
    [JsonSerializable(typeof(IDictionary<string, StaticWebAssetNode>))]
    internal sealed partial class SourceGenerationContext : JsonSerializerContext
    {
        public static readonly SourceGenerationContext DefaultWithConverter = new SourceGenerationContext(new JsonSerializerOptions
        {
            Converters = { new OSBasedCaseConverter() }
        });
    }

@eiriktsarpalis
Copy link
Member

JsonTypeInfo.MakeReadOnly() is new API that was added recently as part of dotnet/runtime#76899 and is invoked by STJ source generated code. Not sure what could be causing this exact issue, but I it's the type of error one might see when using a GA SDK at compile time and then executing it with an earlier version of the runtime.

@carlossanlop
Copy link
Member

This was an unusual case of adding new APIs after preview7. What does aspnetcore need to do (maybe consume dependencies in a certain order) to be able to detect these new APIs?

@wtgodbe
Copy link
Member

wtgodbe commented Nov 3, 2022

Oh, maybe it's this?

<DefaultRuntimeFrameworkVersion Condition=" '$(IsServicingBuild)' != 'true' AND
'%(TargetFramework)' == '${DefaultNetCoreTargetFramework}' AND
'$(TargetLatestDotNetRuntime)' != 'false' ">${MicrosoftNETCoreAppRuntimeVersion}</DefaultRuntimeFrameworkVersion>

Now that this is a servicing build, we're no longer defaulting to using the incoming runtime version, and are instead using what's in the SDK, which doesn't have the new API?

@eiriktsarpalis
Copy link
Member

and are instead using what's in the SDK, which doesn't have the new API?

Seems like a plausible explanation, the RC2 sdk doesn't come with the new API.

@jeffhandley
Copy link
Member

@wtgodbe Can you confirm that 7.0 GA does have the visibility into this API that was added during that milestone, and this only surfaced in servicing because the process for the build changed?

@wtgodbe
Copy link
Member

wtgodbe commented Nov 3, 2022

Can you confirm that 7.0 GA does have the visibility into this API that was added during that milestone, and this only surfaced in servicing because the process for the build changed?

I have no reason to believe otherwise - the API change will have flowed to the SDK as part of the normal process just like any other API change. This mismatch is caused by us using an SDK from rc2, and a runtime from RTM. Normally we have a workaround that avoids these types of mismatches, but we turn that workaround off in servicing based on the assumption that API surface area won't change.

@eiriktsarpalis
Copy link
Member

This mismatch is caused by us using an SDK from rc2, and a runtime from RTM.

I suspect it should be the other way around? An RTM source generator producing calls to JsonTypeInfo.MakeReadOnly() which is then run against an rc2 runtime.

@dougbu
Copy link
Member

dougbu commented Nov 3, 2022

I'll try using a more recent SDK in this PR.

@dougbu
Copy link
Member

dougbu commented Nov 3, 2022

We always use the same SDK and that's 7.0.100-rtm.22478.12 at the moment. The issue is more that we update the targeting pack version unconditionally (based on dependency flow) but rely on the servicing SDK to provide the latest runtime. I'm assuming that SDK version doesn't have a runtime including the JsonTypeInfo.MakeReadOnly() method.

@dougbu
Copy link
Member

dougbu commented Nov 3, 2022

@marcpopMSFT what 7.0.1xx SDK is actually the latest❔ I tried both the installer and SDK versions from https://aka.ms/dotnet/7.0.1xx/daily/productCommit-win-x64.txt w/o success. 7.0.101 is shown on the dotnet/installer home page (matching the installer version), but no luck there in particular.

@dougbu
Copy link
Member

dougbu commented Nov 3, 2022

@marcpopMSFT what 7.0.1xx SDK is actually the latest❔ I tried both the installer and SDK versions from https://aka.ms/dotnet/7.0.1xx/daily/productCommit-win-x64.txt w/o success. 7.0.101 is shown on the dotnet/installer home page (matching the installer version), but no luck there in particular.

@dsplaisted since @marcpopMSFT is under the weather

@wtgodbe
Copy link
Member

wtgodbe commented Nov 3, 2022

I grabbed the one from the latest build of release/7.0.1xx: https://dev.azure.com/dnceng/internal/_build/results?buildId=2034545&view=results

@dougbu
Copy link
Member

dougbu commented Nov 3, 2022

Thanks @wtgodbe. Still wondering what's going on w/ the dotnet/installer home page and the productCommit-win-x64.txt file.

@wtgodbe
Copy link
Member

wtgodbe commented Nov 3, 2022

Still wondering what's going on w/ the dotnet/installer home page and the productCommit-win-x64.txt file.

I think it lists the "shipping" version of the SDK, which (since branding is now stable), will always be unsuffixed. But dotnet/installer produces both a stable-versioned & non-stable-versioned SDK every time when branding is stable, and only the non-stable one can be picked up by dotnet-install because dotnet-install uses the SDK version you list to look for a folder in blob storage. It's only on release day that we create such a folder w/ stable branding.

@wtgodbe
Copy link
Member

wtgodbe commented Nov 3, 2022

I added the stable windowsdesktop feed from https://github.com/dotnet/installer/blob/release/7.0.1xx/NuGet.config#L10, I think that should do it

@wtgodbe
Copy link
Member

wtgodbe commented Nov 3, 2022

On Tuesday we should go to the 7.0.100 SDK (and make sure the windowsdesktop feed is removed)

@wtgodbe
Copy link
Member

wtgodbe commented Nov 3, 2022

Argh, this won't work either - dotnet/runtime#76899 never flowed to the public dotnet/installer repo. I think we should revert the SDK updates & my nuget.config change, and special case the workaround at

<DefaultRuntimeFrameworkVersion Condition=" '$(IsServicingBuild)' != 'true' AND
'%(TargetFramework)' == '${DefaultNetCoreTargetFramework}' AND
'$(TargetLatestDotNetRuntime)' != 'false' ">${MicrosoftNETCoreAppRuntimeVersion}</DefaultRuntimeFrameworkVersion>
for 7.0.1 - @dougbu thoughts?

@dougbu
Copy link
Member

dougbu commented Nov 3, 2022

@dougbu thoughts?

I think this is a convoluted mess that rhymes w/ source generators and late changes. I'll try changing some $(IsServicingBuild) checks. But, if that doesn't work, will give up and wait for the GA SDK to ship publicly.

@build-analysis build-analysis bot mentioned this pull request Nov 3, 2022
2 tasks
- use incoming runtime even though this is a servicing build
@dougbu dougbu force-pushed the branding-7.0.1-2022-10-27-0951 branch from 7b0f75f to 841e438 Compare November 3, 2022 22:21
@@ -53,7 +53,7 @@
e.g. tool projects (again, property not set) use latest.
On the other hand, $(TargetLatestDotNetRuntime) is specific to this repo and controls only the update below.
-->
<DefaultRuntimeFrameworkVersion Condition=" '$(IsServicingBuild)' != 'true' AND
<DefaultRuntimeFrameworkVersion Condition=" ('$(IsServicingBuild)' != 'true' OR '$(NETCoreSdkVersion)' == '7.0.100-rtm.22478.12') AND
Copy link
Member

Choose a reason for hiding this comment

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

Note this hack automatically stops working as soon as we change the SDK version.

@marcpopMSFT
Copy link
Member

Will's analysis above is correct. We're now on stable branding which dotnet-install can't use (until it actually ships) so that installer table is no longer useful for 7.0.1xx. I'll work on getting it removed (it's the same reason we removed all the other servicing branches from that table).

@dougbu
Copy link
Member

dougbu commented Nov 3, 2022

@marcpopMSFT is there a way to reliably get the latest public nonstable version (that dotnet-install scripts can use) other than @wtgodbe's method of visiting installer builds and finding the latest successful one for (say) the release/7.0.1xx branch❔ I'd like to update my Get-NightlySdkVersion.ps1 script so it works even during the X.0.0 to X.0.1 transition. (We stick w/ public stable versions once they exist.)

@dougbu
Copy link
Member

dougbu commented Nov 3, 2022

/fyi all: Builds are looking good so far, except for some macOS issues w/ AzDO feeds. Expect I'll need to rerun failed jobs, but should be able to get this in tonight.

@dougbu dougbu merged commit 01b8d25 into dotnet:release/7.0 Nov 4, 2022
@marcpopMSFT
Copy link
Member

@marcpopMSFT is there a way to reliably get the latest public nonstable version (that dotnet-install scripts can use) other than @wtgodbe's method of visiting installer builds and finding the latest successful one for (say) the release/7.0.1xx branch❔ I'd like to update my Get-NightlySdkVersion.ps1 script so it works even during the X.0.0 to X.0.1 transition. (We stick w/ public stable versions once they exist.)

I'm not sure if there is. @YuliiaKovalova might have some idea or suggestion on if there's a channel or something you can use to get the latest daily build of a stable release.

@ghost
Copy link

ghost commented Nov 4, 2022

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

@YuliiaKovalova
Copy link
Member

Hi @marcpopMSFT ,

It is possible to specify -Channel "LTS" -Quality "daily" for getting the latest build of the stable version.

Please see for getting more details:
https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-install-script#:~:text=%2D-,Quality%7C%2D%2Dquality%20%3CQUALITY%3E,-Downloads%20the%20latest

@ghost
Copy link

ghost commented Nov 7, 2022

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

@dougbu
Copy link
Member

dougbu commented Nov 7, 2022

Thanks @YuliiaKovalova, hadn't thought to use -quality quite that way. (Though it's actually ignored for the LTS channel.)

In any case, I'd reserve -quality ga for our regular SDK updates of 'main'. .\.dotnet\dotnet-install.ps1 -channel 7.0.1xx -dryrun -quality preview would have found the SDK build we tried (and eventually discarded) in this branch. -quality ga also looks like a great way to find the latest SDK for older branches.

@dougbu dougbu added this to the 7.0.1 milestone Nov 8, 2022
@dougbu dougbu added the tell-mode Indicates a PR which is being merged during tell-mode label Nov 8, 2022
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 tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants