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

[release/8.0] Stable branding for .NET 8 GA #93807

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

carlossanlop
Copy link
Member

This is it folks.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release area-Infrastructure labels Oct 20, 2023
@carlossanlop carlossanlop added this to the 8.0.0 milestone Oct 20, 2023
@carlossanlop carlossanlop self-assigned this Oct 20, 2023
@ghost
Copy link

ghost commented Oct 20, 2023

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

This is it folks.

Author: carlossanlop
Assignees: carlossanlop
Labels:

Servicing-approved, area-Infrastructure

Milestone: 8.0.0

@lewing lewing requested a review from radical as a code owner October 21, 2023 02:42
@lewing
Copy link
Member

lewing commented Oct 21, 2023

This needs #93801 to land first as things are. I can work around that but not tonight

@carlossanlop carlossanlop added the blocked Issue/PR is blocked on something - see comments label Oct 23, 2023
@lewing lewing removed the blocked Issue/PR is blocked on something - see comments label Oct 23, 2023
@lewing
Copy link
Member

lewing commented Oct 23, 2023

Failure now in runtime (Build windows-x64 Checked NativeAOT) is

      > C:\h\w\B2ED097A\w\AEAA094B\e\tracing\eventpipe\processinfo3\processinfo3\processinfo3.cmd
      Expected: True
      Actual:   False
      Stack Trace:
        D:\a\_work\1\s\artifacts\tests\coreclr\windows.x64.Checked\TestWrappers\tracing.eventpipe\tracing.eventpipe.XUnitWrapper.cs(1040,0): at tracing_eventpipe._processinfo3_processinfo3_processinfo3_._processinfo3_processinfo3_processinfo3_cmd()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
           at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
      Output:
        Unhandled Exception: System.Exception: ClrProductVersion must match. Expected: "8.0.0-ci", received: "8.0.0"
           at Tracing.Tests.Common.Utils.Assert(Boolean predicate, String message) + 0x36
           at Tracing.Tests.ProcessInfoValidation.ProcessInfoValidation.Main() + 0x29ca
           at processinfo3!<BaseAddress>+0xa8c02b

@ViktorHofer
Copy link
Member

Why didn't this show up in servicing readiness? cc @dkurepa

@akoeplinger
Copy link
Member

akoeplinger commented Oct 23, 2023

Not sure why it didn't show up in servicing readiness but we probably need to add this override to NativeAOT corelib, it is there both in CoreCLR and Mono corelib:

<!-- Override InformationalVersion during servicing as it's returned via public api. -->
<InformationalVersion Condition="'$(PreReleaseVersionLabel)' == 'servicing'">$(ProductVersion)</InformationalVersion>
<InformationalVersion Condition="'$(StabilizePackageVersion)' == 'true'">$(ProductVersion)</InformationalVersion>

Some more discussion in this issue: #60532

@ericstj
Copy link
Member

ericstj commented Oct 23, 2023

Here's the test:

string expectedClrProductVersion = typeof(object).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;

Its checking the AssemblyInformationalVersionAttribute on System.Private.Corelib against the diagnostics protocol which returns CLR_PRODUCT_VERSION which seems to be defined as RuntimeProductVersion ->

#define RuntimeProductVersion $(Version)

@dkurepa
Copy link
Member

dkurepa commented Oct 23, 2023

Why didn't this show up in servicing readiness? cc @dkurepa

I'm not sure. I just checked and it definitely didn't show up

@akoeplinger
Copy link
Member

akoeplinger commented Oct 23, 2023

This is actually not a regression, I checked the 7.0.12 version of the NativeAOT runtime.linux-x64.Microsoft.DotNet.ILCompiler nuget package and it does include the servicing version suffix in AssemblyInformationalVersionAttribute:

  .custom instance void System.Reflection.AssemblyInformationalVersionAttribute::.ctor(string) = ( 01 00 42 37 2E 30 2E 31 32 2D 73 65 72 76 69 63   // ..B7.0.12-servic
                                                                                                   69 6E 67 2E 32 33 34 37 37 2E 32 30 2B 34 61 38   // ing.23477.20+4a8
                                                                                                   32 34 65 66 33 37 63 61 61 35 31 30 37 32 32 32   // 24ef37caa5107222
                                                                                                   31 35 38 34 63 36 34 63 62 66 31 35 34 35 35 66   // 1584c64cbf15455f
                                                                                                   34 30 36 63 61 00 00 )                            // 406ca..

... while a normal Microsoft.NETCore.App.Runtime.linux-x64 CoreCLR runtime pack does not:

  .custom instance void System.Reflection.AssemblyInformationalVersionAttribute::.ctor(string) = ( 01 00 2F 37 2E 30 2E 31 32 2B 34 61 38 32 34 65   // ../7.0.12+4a824e
                                                                                                   66 33 37 63 61 61 35 31 30 37 32 32 32 31 35 38   // f37caa5107222158
                                                                                                   34 63 36 34 63 62 66 31 35 34 35 35 66 34 30 36   // 4c64cbf15455f406
                                                                                                   63 61 00 00 )                                     // ca..

I assume this test didn't run on NativeAOT in 7.0.

@carlossanlop
Copy link
Member Author

@akoeplinger are you the right person to look into this remaining failure?

@akoeplinger
Copy link
Member

@carlossanlop yes I'm 99% sure I know the fix, if it works should I push to this PR or should we go to main first and then backport?

@carlossanlop
Copy link
Member Author

carlossanlop commented Oct 23, 2023

@carlossanlop yes I'm 99% sure I know the fix, if it works should I push to this PR or should we go to main first and then backport?

Let's do main + backport. Thanks!

@carlossanlop
Copy link
Member Author

Actually, since we're late, i think it will be better to do main + submit the fix to this PR directly, not backport. @akoeplinger

@akoeplinger
Copy link
Member

Pushed, main PR is in #93889

@agocke
Copy link
Member

agocke commented Oct 23, 2023

Changes look OK to me, but FWIW Roslyn always ships with a suffix in the version. It doesn't seem like we care much either way. This test seems overly aggressive to me.

@mmitche
Copy link
Member

mmitche commented Oct 23, 2023

Thanks, please merge ASAP.

@akoeplinger
Copy link
Member

Changes look OK to me, but FWIW Roslyn always ships with a suffix in the version. It doesn't seem like we care much either way. This test seems overly aggressive to me.

@agocke this is actually exposed through System.Runtime.InteropServices.RuntimeInformation.FrameworkDescription and there have been multiple reports in the past if it shows the wrong version (see #45812 and duplicated issues)

public static string FrameworkDescription
{
get
{
if (s_frameworkDescription == null)
{
ReadOnlySpan<char> versionString = typeof(object).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
// Strip the git hash if there is one
int plusIndex = versionString.IndexOf('+');
if (plusIndex >= 0)
{
versionString = versionString.Slice(0, plusIndex);
}
s_frameworkDescription = !versionString.Trim().IsEmpty ? $"{FrameworkName} {versionString}" : FrameworkName;
}
return s_frameworkDescription;
}
}

akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Oct 24, 2023
We need to make sure the string contains only the stabilized version in servicing, or a non-stabilized one otherwise.
This prevents issues like dotnet#45812 and what we hit in dotnet#93807.

Closes dotnet#45812
akoeplinger added a commit that referenced this pull request Oct 25, 2023
…93913)

We need to make sure the string contains only the stabilized version in servicing, or a non-stabilized one otherwise. This prevents issues like #45812 and what we hit in #93807.

Closes #45812
liveans pushed a commit to liveans/dotnet-runtime that referenced this pull request Nov 9, 2023
…otnet#93913)

We need to make sure the string contains only the stabilized version in servicing, or a non-stabilized one otherwise. This prevents issues like dotnet#45812 and what we hit in dotnet#93807.

Closes dotnet#45812
@ghost ghost locked as resolved and limited conversation to collaborators Nov 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants