Remove reflection from FrameworkDescription by am11 · Pull Request #102164 · dotnet/runtime

am11 · 2024-05-13T18:37:26Z

Tiny help for trimming; shaved 2KB off of artifacts/bin/coreclr/osx.arm64.Release/System.Private.CoreLib.dll.

dotnet-policy-service · 2024-05-13T18:39:27Z

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

jkotas · 2024-05-13T20:16:04Z

            }
        }

+        internal const string InternalVersion = "9.0.0";


I do not think we want to add yet another place to update for version bumps. If you want to do this, it needs to be wired to eng/Versions.props.

The version update bump looks like this 96c2e1d once a year. Is it really a big deal to update a string? If you think it is, lets close this PR then. It was a simple improvement with no visible effect on production bits.

The version bumps are done every month in servicing, and they look like this: #101779

Ah, the patch version. 🤭

Let me think a bit how to dedup it.

jkotas · 2024-05-13T20:21:31Z


            Assert.DoesNotContain("+", version); // no git hash
-
-#if STABILIZE_PACKAGE_VERSION


I think this should stay as is. It is a regression test for a bug that slipped through the cracks some time ago.

am11 · 2024-05-14T13:23:26Z

@@ -5,6 +5,7 @@
  </PropertyGroup>


Side note, <TargetFramework>netstandard2.0</TargetFramework> this can probably use $(NetCoreAppCurrent) as it is only used by corelib building for current version.

There may be cases where the generator runs on .NET Framework if people open the project in Visual Studio.

am11 · 2024-05-14T13:30:11Z

Tested with ./build.sh (all subsets), ./build.sh libs only, then mono only and clr only. It generates ProductVersionInfo.g.cs under artifacts/obj correctly; but at different locations, which we can consolidate with <CompilerGeneratedFilesOutputPath> prop if needed, e.g. at the root of obj/ next to:

$ ls artifacts/obj/*version*
artifacts/obj/_version.c	artifacts/obj/_version.h	artifacts/obj/runtime_version.h

…erator.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>

jkotas · 2024-05-14T15:00:21Z

It generates ProductVersionInfo.g.cs under artifacts/obj correctly; but at different locations

That's expected. Different CoreLib builds should not be sharing files.

jkotas

Thank you!

stephentoub · 2024-05-15T14:38:19Z

+        {
+            context.RegisterPostInitializationOutput(ctx =>
+            {
+                string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;


I'm not sure how our build system handles this. Is this guaranteed to always match what typeof(object).Assembly would in the built corelib?

As far as I can tell, the build system stamps the exact same AssemblyInformationalVersionAttribute on all binaries.

* Remove reflection from FrameworkDescription * Switch to source gen * Address CR feedback * Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com> --------- Co-authored-by: Jan Kotas <jkotas@microsoft.com>

ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 13, 2024

dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label May 13, 2024

jkotas reviewed May 13, 2024

View reviewed changes

am11 added 3 commits May 14, 2024 04:48

Remove reflection from FrameworkDescription

c398549

Remove prerelease tests

0bbf65b

Switch to source gen

db1420d

am11 force-pushed the feature/versioning/cleanups branch from 4c08f94 to db1420d Compare May 14, 2024 01:48