Include commit SHA in [AssemblyInformationalVersion] value

[dougbu]

• see Standardization of runtime assembly attributes arcade#5866 discussion

[dougbu]

/fyi @ajcvickers @bricelam @smitpatel

@Pilchie this is the other 5.0 fix to use consistent "format" for our [AssemblyInformationalVersion] attributes. Will do 3.1.10 variants after checking 3.1.9 assemblies. Suspect I'll need to change dotnet/extensions too.

[ajcvickers]

Note that we do expose and use [AssemblyInformationalVersion] in model snapshots, etc. I doubt this will break anything, since I don't think we do any parsing. @bricelam?

[ajcvickers]

Ref: https://github.com/dotnet/efcore/blob/main/src/EFCore/Infrastructure/ProductInfo.cs

[bricelam]

It breaks—the column length is too small in the database

[bricelam]

Well, it broke when we tried this in the past anyway

[ajcvickers]

If only we had a way to update the schema... ;-)

[dougbu]

Well, it broke when we tried this in the past anyway

Do tests cover any database mapping from the ProductInfo class❔ PR validation succeeded.

[bricelam]

I think it was on MySQL or Oracle or something. We should review the previous attempt before moving forward on this.

[bricelam]

Migrating the history table might be a bit too chicken and egg. 😐

[dougbu]

Where's the previous attempt❔ Would like to get this into 5.0 RC2. We have a bit longer for the 3.1 version of this (#22650).

[dougbu]

Found previous attempt I think. @bricelam your comment in #14177 (the original Arcade conversion you did) says

Fixed the version, broke some tests.

[AssemblyInformationalVersion("3.0.0-preview.18618.5+078a3054c6c04951dd00c71c6d3ad85d4cb350d6")]

But __EFMigrationsHistory.ProductVersion only allows up to 32 characters.

We can remove the commit part by setting IncludeSourceRevisionInInformationalVersion to false.

So, what controls the length of __EFMigrationsHistory.ProductVersion and why aren't tests failing here and in #22650❔

[ajcvickers]

@dougbu We likely need to add coverage.

Is there a functional reason for making this change?

[dougbu]

Is there a functional reason for making this change?

Well, perhaps a corner case, but not including the SHA makes some Reflection introspection difficult or, in the EF case, impossible.

@sebastienros pointed out inconsistent assembly metadata between aspnetcore and runtime DLLs in dotnet/arcade#5866. That led us to look at other repos and their assemblies and we noticed EF assemblies don't contain the commit SHA anywhere. We also saw Arcade documentation stressing use of this "full" [AssemblyInformationalVersion] format.

If we find including the SHA in the [AssemblyInformationalVersion] causes problems, I suggest adding [AssemblyMetadata("CommitHash", ...)]. That's an option aspnetcore uses though it's now redundant.

Realize the PR validation build doesn't exercise this change; [AssemblyInformationalVersion] values remain 5.0.0-ci in an aspnetcore validation build that includes a similar update. I'll kick off an internal build to (a) ensure we get artifacts to double-check and (b) run the tests w/ values like 5.0.0-rc.2.20472.5+93f73c77640abc472fb901e18bf5055961e23e44.

[bricelam]

+1 for using [AssemblyMetadata("CommitHash", ...)] instead

[bricelam]

Here's the comment (hidden by default) from last time we tried. The column length is 32.

                                v
3.0.0-preview.18618.5+078a3054c6c04951dd00c71c6d3ad85d4cb350d6
                                ^

[ajcvickers]

@bricelam Could we update ProductInfo.cs to trim the value coming from the attribute?

[dougbu]

@bricelam that comment looks like a separate signing issue hit in #14177 that @JohnTortugo fixed in Arcade. But the

                                v
3.0.0-preview.18618.5+078a3054c6c04951dd00c71c6d3ad85d4cb350d6
                                ^

bit is definitely relevant.

I think #14177 (comment) remains the most relevant comment in #14177.

Not sure why https://dev.azure.com/dnceng/internal/_build/results?buildId=825609 isn't failing (yet❔) though.

[bricelam]

@ajcvickers I'd want to add a full semantic version parser as part of that. (see also dotnet/runtime#19317)

[ajcvickers]

@bricelam Consider for 6.0?

[bricelam]

Why not just use an AssemblyMetadata attribute instead? And do it in RC2.

[ajcvickers]

@dougbu

Well, perhaps a corner case, but not including the SHA makes some Reflection introspection difficult or, in the EF case, impossible.

Can you link to more information about this? What, specifically, is impossible?

[ajcvickers]

@bricelam I'm still trying to figure out the actual reason for this change so that if we do that we know whether or not it fixes anything.

[bricelam]

Additional context: We lost this information when we moved form KoreBuild to Arcade. I thought I filed an issue to add it back, but it must've fallen through the cracks.

[dougbu]

Can you link to more information about this?

See dotnet/arcade#5866 and https://github.com/dotnet/arcade/blob/master/Documentation/CorePackages/Versioning.md#assembly-informational-version-generation

What, specifically, is impossible?

Figuring out the commit SHA used when building an assembly.

We lost this information when we moved form KoreBuild to Arcade

and

+1 for using [AssemblyMetadata("CommitHash", ...)] instead

I'm fine with this because https://dev.azure.com/dnceng/internal/_build/results?buildId=825609 was successful despite assemblies having [AssemblyInformationalVersion("5.0.0-rc.2.20472.2+fba9fd552018bb9bde9e6424c1a7c3db97721265")]. Would make the metadata available though it wouldn't match the Arcade guidance or @sebastienros's hope for consistency.

I agree this change would need to be larger i.e. include tests and either a column width update or a parser in ProductInfo. Either is risky this late in 5.0 and not reasonable at all for 3.1.

Any objection to making the minimal [AssemblyMetadata("CommitHash", ...)] addition and leaving [AssemblyInformationalVersion(...)] alone❔

[ajcvickers]

@dougbu @bricelam Thanks for the detailed info. Seems like the CommitHash approach is the way to go.

[dougbu]

🆙📅 to switch approaches

dotnet/aspnetcore also adds the following in its version of the same target.

<AssemblyAttribute Include="System.Reflection.AssemblyMetadataAttribute" Condition="$(RepositoryUrl.StartsWith('https://github.com'))">
  <_Parameter1>SourceCommitUrl</_Parameter1>
  <_Parameter2>$(RepositoryUrl)/tree/$(SourceRevisionId)</_Parameter2>
</AssemblyAttribute>

<AssemblyAttribute Include="System.Reflection.AssemblyMetadataAttribute" Condition="'$(Serviceable)' == 'true'">
  <_Parameter1>Serviceable</_Parameter1>
  <_Parameter2>True</_Parameter2>
</AssemblyAttribute>

SourceCommitUrl information looks a bit redundant since assemblies already contain [AssemblyMetadata("RepositoryUrl", "https://github.com/dotnet/efcore")] and will now contain [AssemblyMetadata("CommitHash", ...)]. Not sure anything would read the SourceCommitUrl data.

But, is the lack of Serviceable metadata a problem @blowdart❔

[bricelam]

But, is the lack of Serviceable metadata a problem

IIUC, it's only used on .NET Framework. See dotnet/arcade#1526

[blowdart]

IIUC, it was only used on .NET Framework. See dotnet/arcade#1526

I don't believe that's true. The Servicable attribute was supposed to stay in place to enable the runtime to drop information on what assemblies have ever been loaded on a system, and that includes on Core. I don't believe this has ever changed.

Put it back, unless @ericstj says hammer patching uses another mechanism.

[dougbu]

Put it back, unless @ericstj says hammer patching uses another mechanism.

Easy to do though the Serviceable information hasn't been included in EF attributes at least since the migration to Arcade. I'll give @ericstj a chance to respond.

[ericstj]

Serviceable assembly metadata is used by .NETFramework loader to write breadcrumbs. I believe the .NETCore host uses the serviceable metadata from the deps file. cc @agocke @vitek-karas

[dougbu]

Hmm, I believe this repo produces a number of assemblies usable on .NET Framework e.g. Micrsoft.Data.Sqlite as seen at

efcore/src/Microsoft.Data.Sqlite/Microsoft.Data.Sqlite.csproj

Line 18 in b970bf2

<TargetFramework>netstandard2.0</TargetFramework>

[bricelam]

The nuspecs include <serviceable>true</serviceable>, which is maybe what .NET Core uses instead.

[bricelam]

Happy to add it back--that's why I filed the issue on Aracade two years ago. 🙂 (to get to the bottom of whether it was actually needed or not)

[dougbu]

I need lunch but will at least add the Serviceable information after that. Not hearing a clamor for SourceCommitUrl but let me know if you want that too.

Will get this ready to go and then update the 3.1 PR.

Any reason to worry about EF6's release/6.4 and master branches given how quiet that repo is❔

[dougbu]

@ajcvickers is this PR still blocked❔

[ajcvickers]

@dougbu Nope. @bricelam signing off is good for me. :-)

[bricelam]

Thanks, Doug! I'm really glad we have this information back. Reverse engineering it from the build number was really painful the few times I had to do it.