Remove reflection from FrameworkDescription - #102164

Merged
jkotas merged 7 commits into
dotnet:mainfrom
am11:feature/versioning/cleanups
May 15, 2024
Merged

Remove reflection from FrameworkDescription#102164
jkotas merged 7 commits into
dotnet:mainfrom
am11:feature/versioning/cleanups

Conversation

@am11

@am11am11 commented May 13, 2024

Copy link
Copy Markdown
Member

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

@ghostghost 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-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label May 13, 2024
@am11am11 added area-System.Runtime community-contribution Indicates that the PR has been added by a community member and removed community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 13, 2024
@dotnet-policy-service

dotnet-policy-serviceBot commented May 13, 2024

Copy link
Copy Markdown
Contributor

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

jkotas
jkotas reviewed May 13, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs Outdated
}
}

internal const string InternalVersion = "9.0.0";

@jkotasjkotasMay 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@am11am11May 13, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

@jkotasjkotasMay 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@am11am11May 13, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Ah, the patch version. 🤭

Let me think a bit how to dedup it.

jkotas
jkotas reviewed May 13, 2024
View reviewed changes
Comment thread...untime/tests/System.Runtime.InteropServices.RuntimeInformation.Tests/DescriptionNameTests.cs

Assert.DoesNotContain("+", version); // no git hash

#if STABILIZE_PACKAGE_VERSION

@jkotasjkotasMay 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@am11
am11force-pushed the feature/versioning/cleanups branch from 4c08f94 to db1420dCompareMay 14, 2024 01:48
jkotas
jkotas reviewed May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs Outdated
jkotas
jkotas reviewed May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems Outdated
@am11
am11force-pushed the feature/versioning/cleanups branch 2 times, most recently from 80d8f1f to 791a6e7CompareMay 14, 2024 07:03
@am11
am11force-pushed the feature/versioning/cleanups branch from 791a6e7 to c1ce415CompareMay 14, 2024 07:10
am11
am11 commented May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
@@ -5,6 +5,7 @@
</PropertyGroup>

@am11am11May 14, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

@jkotasjkotasMay 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@am11

am11 commented May 14, 2024

Copy link
Copy Markdown
MemberAuthor

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

jkotas
jkotas reviewed May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs Outdated
@am11@jkotas
Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGen…
bd4f66e
…erator.cs
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas

jkotas commented May 14, 2024

Copy link
Copy Markdown
Member

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
jkotas approved these changes May 14, 2024
View reviewed changes

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@build-analysisbuild-analysisBot mentioned this pull request May 15, 2024
Closed
This was referenced May 15, 2024
Closed
Closed
@jkotas
jkotas merged commit 426edd0 into dotnet:mainMay 15, 2024
stephentoub
stephentoub reviewed May 15, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
{
context.RegisterPostInitializationOutput(ctx =>
{
string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;

@stephentoubstephentoubMay 15, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@jkotasjkotasMay 15, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@am11@jkotas@Ruihan-Yin
Remove reflection from FrameworkDescription (dotnet#102164)
c100549
* 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>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jun 15, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas approved these changes

Assignees

No one assigned

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@am11@jkotas@stephentoub
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Remove reflection from FrameworkDescription - #102164
Merged
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
May 15, 2024
Merged
Remove reflection from FrameworkDescription#102164
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
Conversation
@am11
@am11am11
commented
May 13, 2024

Copy link


Copy Markdown

Member
Tiny help for trimming; shaved 2KB off of artifacts/bin/coreclr/osx.arm64.Release/System.Private.CoreLib.dll.
@ghostghost
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-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 13, 2024
@am11am11
added

area-System.Runtime

community-contribution
Indicates that the PR has been added by a community member
and removed

community-contribution
Indicates that the PR has been added by a community member
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
May 13, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
May 13, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

 }
 }

 internal const string InternalVersion = "9.0.0";
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@am11am11May 13, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ah, the patch version. 🤭
Let me think a bit how to dedup it.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment thread...untime/tests/System.Runtime.InteropServices.RuntimeInformation.Tests/DescriptionNameTests.cs

 Assert.DoesNotContain("+", version); // no git hash

#if STABILIZE_PACKAGE_VERSION
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should stay as is. It is a regression test for a bug that slipped through the cracks some time ago.
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
4c08f94 to
db1420dCompareMay 14, 2024 01:48
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated

@am11
am11force-pushed
the
feature/versioning/cleanups
branch
2 times, most recently
from
80d8f1f to
791a6e7CompareMay 14, 2024 07:03
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
791a6e7 to
c1ce415CompareMay 14, 2024 07:10
am11
am11
commented
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
@@ -5,6 +5,7 @@
 </PropertyGroup>
@am11am11May 14, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

There may be cases where the generator runs on .NET Framework if people open the project in Visual Studio.
@am11
am11
commented
May 14, 2024

Copy link


Copy Markdown

MemberAuthor
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
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
Outdated

@am11@jkotas
Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGen…
bd4f66e
…erator.cs
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas
jkotas
commented
May 14, 2024

Copy link


Copy Markdown

Member
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
jkotas
approved these changes
May 14, 2024
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
May 15, 2024

Closed


This was referenced May 15, 2024

Closed


Closed

@jkotas
jkotas
merged commit 426edd0
into
dotnet:mainMay 15, 2024
stephentoub
stephentoub
reviewed
May 15, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
 {
 context.RegisterPostInitializationOutput(ctx =>
 {
 string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
@stephentoubstephentoubMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?
@jkotasjkotasMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

As far as I can tell, the build system stamps the exact same AssemblyInformationalVersionAttribute on all binaries.
Ruihan-Yin pushed a commit
to Ruihan-Yin/runtime
that referenced
this pull request
May 30, 2024
@am11@jkotas@Ruihan-Yin
Remove reflection from FrameworkDescription (dotnet#102164)
c100549
* 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>
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jun 15, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas approved these changes

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@am11@jkotas@stephentoub

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove reflection from FrameworkDescription - #102164
Merged
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
May 15, 2024
Merged
Remove reflection from FrameworkDescription#102164
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
Conversation
@am11
@am11am11
commented
May 13, 2024

Copy link


Copy Markdown

Member
Tiny help for trimming; shaved 2KB off of artifacts/bin/coreclr/osx.arm64.Release/System.Private.CoreLib.dll.
@ghostghost
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-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 13, 2024
@am11am11
added

area-System.Runtime

community-contribution
Indicates that the PR has been added by a community member
and removed

community-contribution
Indicates that the PR has been added by a community member
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
May 13, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
May 13, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

 }
 }

 internal const string InternalVersion = "9.0.0";
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@am11am11May 13, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ah, the patch version. 🤭
Let me think a bit how to dedup it.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment thread...untime/tests/System.Runtime.InteropServices.RuntimeInformation.Tests/DescriptionNameTests.cs

 Assert.DoesNotContain("+", version); // no git hash

#if STABILIZE_PACKAGE_VERSION
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should stay as is. It is a regression test for a bug that slipped through the cracks some time ago.
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
4c08f94 to
db1420dCompareMay 14, 2024 01:48
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated

@am11
am11force-pushed
the
feature/versioning/cleanups
branch
2 times, most recently
from
80d8f1f to
791a6e7CompareMay 14, 2024 07:03
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
791a6e7 to
c1ce415CompareMay 14, 2024 07:10
am11
am11
commented
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
@@ -5,6 +5,7 @@
 </PropertyGroup>
@am11am11May 14, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

There may be cases where the generator runs on .NET Framework if people open the project in Visual Studio.
@am11
am11
commented
May 14, 2024

Copy link


Copy Markdown

MemberAuthor
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
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
Outdated

@am11@jkotas
Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGen…
bd4f66e
…erator.cs
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas
jkotas
commented
May 14, 2024

Copy link


Copy Markdown

Member
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
jkotas
approved these changes
May 14, 2024
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
May 15, 2024

Closed


This was referenced May 15, 2024

Closed


Closed

@jkotas
jkotas
merged commit 426edd0
into
dotnet:mainMay 15, 2024
stephentoub
stephentoub
reviewed
May 15, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
 {
 context.RegisterPostInitializationOutput(ctx =>
 {
 string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
@stephentoubstephentoubMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?
@jkotasjkotasMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

As far as I can tell, the build system stamps the exact same AssemblyInformationalVersionAttribute on all binaries.
Ruihan-Yin pushed a commit
to Ruihan-Yin/runtime
that referenced
this pull request
May 30, 2024
@am11@jkotas@Ruihan-Yin
Remove reflection from FrameworkDescription (dotnet#102164)
c100549
* 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>
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jun 15, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas approved these changes

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@am11@jkotas@stephentoub

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove reflection from FrameworkDescription - #102164
Merged
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
May 15, 2024
Merged
Remove reflection from FrameworkDescription#102164
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
Conversation
@am11
@am11am11
commented
May 13, 2024

Copy link


Copy Markdown

Member
Tiny help for trimming; shaved 2KB off of artifacts/bin/coreclr/osx.arm64.Release/System.Private.CoreLib.dll.
@ghostghost
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-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 13, 2024
@am11am11
added

area-System.Runtime

community-contribution
Indicates that the PR has been added by a community member
and removed

community-contribution
Indicates that the PR has been added by a community member
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
May 13, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
May 13, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

 }
 }

 internal const string InternalVersion = "9.0.0";
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@am11am11May 13, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ah, the patch version. 🤭
Let me think a bit how to dedup it.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment thread...untime/tests/System.Runtime.InteropServices.RuntimeInformation.Tests/DescriptionNameTests.cs

 Assert.DoesNotContain("+", version); // no git hash

#if STABILIZE_PACKAGE_VERSION
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should stay as is. It is a regression test for a bug that slipped through the cracks some time ago.
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
4c08f94 to
db1420dCompareMay 14, 2024 01:48
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated

@am11
am11force-pushed
the
feature/versioning/cleanups
branch
2 times, most recently
from
80d8f1f to
791a6e7CompareMay 14, 2024 07:03
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
791a6e7 to
c1ce415CompareMay 14, 2024 07:10
am11
am11
commented
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
@@ -5,6 +5,7 @@
 </PropertyGroup>
@am11am11May 14, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

There may be cases where the generator runs on .NET Framework if people open the project in Visual Studio.
@am11
am11
commented
May 14, 2024

Copy link


Copy Markdown

MemberAuthor
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
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
Outdated

@am11@jkotas
Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGen…
bd4f66e
…erator.cs
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas
jkotas
commented
May 14, 2024

Copy link


Copy Markdown

Member
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
jkotas
approved these changes
May 14, 2024
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
May 15, 2024

Closed


This was referenced May 15, 2024

Closed


Closed

@jkotas
jkotas
merged commit 426edd0
into
dotnet:mainMay 15, 2024
stephentoub
stephentoub
reviewed
May 15, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
 {
 context.RegisterPostInitializationOutput(ctx =>
 {
 string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
@stephentoubstephentoubMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?
@jkotasjkotasMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

As far as I can tell, the build system stamps the exact same AssemblyInformationalVersionAttribute on all binaries.
Ruihan-Yin pushed a commit
to Ruihan-Yin/runtime
that referenced
this pull request
May 30, 2024
@am11@jkotas@Ruihan-Yin
Remove reflection from FrameworkDescription (dotnet#102164)
c100549
* 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>
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jun 15, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas approved these changes

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@am11@jkotas@stephentoub

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Remove reflection from FrameworkDescription - #102164
Merged
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
May 15, 2024
Merged
Remove reflection from FrameworkDescription#102164
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
Conversation
@am11
@am11am11
commented
May 13, 2024

Copy link


Copy Markdown

Member
Tiny help for trimming; shaved 2KB off of artifacts/bin/coreclr/osx.arm64.Release/System.Private.CoreLib.dll.
@ghostghost
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-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 13, 2024
@am11am11
added

area-System.Runtime

community-contribution
Indicates that the PR has been added by a community member
and removed

community-contribution
Indicates that the PR has been added by a community member
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
May 13, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
May 13, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

 }
 }

 internal const string InternalVersion = "9.0.0";
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@am11am11May 13, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ah, the patch version. 🤭
Let me think a bit how to dedup it.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment thread...untime/tests/System.Runtime.InteropServices.RuntimeInformation.Tests/DescriptionNameTests.cs

 Assert.DoesNotContain("+", version); // no git hash

#if STABILIZE_PACKAGE_VERSION
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should stay as is. It is a regression test for a bug that slipped through the cracks some time ago.
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
4c08f94 to
db1420dCompareMay 14, 2024 01:48
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated

@am11
am11force-pushed
the
feature/versioning/cleanups
branch
2 times, most recently
from
80d8f1f to
791a6e7CompareMay 14, 2024 07:03
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
791a6e7 to
c1ce415CompareMay 14, 2024 07:10
am11
am11
commented
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
@@ -5,6 +5,7 @@
 </PropertyGroup>
@am11am11May 14, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

There may be cases where the generator runs on .NET Framework if people open the project in Visual Studio.
@am11
am11
commented
May 14, 2024

Copy link


Copy Markdown

MemberAuthor
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
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
Outdated

@am11@jkotas
Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGen…
bd4f66e
…erator.cs
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas
jkotas
commented
May 14, 2024

Copy link


Copy Markdown

Member
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
jkotas
approved these changes
May 14, 2024
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
May 15, 2024

Closed


This was referenced May 15, 2024

Closed


Closed

@jkotas
jkotas
merged commit 426edd0
into
dotnet:mainMay 15, 2024
stephentoub
stephentoub
reviewed
May 15, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
 {
 context.RegisterPostInitializationOutput(ctx =>
 {
 string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
@stephentoubstephentoubMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?
@jkotasjkotasMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

As far as I can tell, the build system stamps the exact same AssemblyInformationalVersionAttribute on all binaries.
Ruihan-Yin pushed a commit
to Ruihan-Yin/runtime
that referenced
this pull request
May 30, 2024
@am11@jkotas@Ruihan-Yin
Remove reflection from FrameworkDescription (dotnet#102164)
c100549
* 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>
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jun 15, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas approved these changes

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@am11@jkotas@stephentoub

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove reflection from FrameworkDescription - #102164
Merged
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
May 15, 2024
Merged
Remove reflection from FrameworkDescription#102164
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
Conversation
@am11
@am11am11
commented
May 13, 2024

Copy link


Copy Markdown

Member
Tiny help for trimming; shaved 2KB off of artifacts/bin/coreclr/osx.arm64.Release/System.Private.CoreLib.dll.
@ghostghost
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-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 13, 2024
@am11am11
added

area-System.Runtime

community-contribution
Indicates that the PR has been added by a community member
and removed

community-contribution
Indicates that the PR has been added by a community member
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
May 13, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
May 13, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

 }
 }

 internal const string InternalVersion = "9.0.0";
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@am11am11May 13, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ah, the patch version. 🤭
Let me think a bit how to dedup it.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment thread...untime/tests/System.Runtime.InteropServices.RuntimeInformation.Tests/DescriptionNameTests.cs

 Assert.DoesNotContain("+", version); // no git hash

#if STABILIZE_PACKAGE_VERSION
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should stay as is. It is a regression test for a bug that slipped through the cracks some time ago.
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
4c08f94 to
db1420dCompareMay 14, 2024 01:48
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated

@am11
am11force-pushed
the
feature/versioning/cleanups
branch
2 times, most recently
from
80d8f1f to
791a6e7CompareMay 14, 2024 07:03
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
791a6e7 to
c1ce415CompareMay 14, 2024 07:10
am11
am11
commented
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
@@ -5,6 +5,7 @@
 </PropertyGroup>
@am11am11May 14, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

There may be cases where the generator runs on .NET Framework if people open the project in Visual Studio.
@am11
am11
commented
May 14, 2024

Copy link


Copy Markdown

MemberAuthor
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
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
Outdated

@am11@jkotas
Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGen…
bd4f66e
…erator.cs
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas
jkotas
commented
May 14, 2024

Copy link


Copy Markdown

Member
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
jkotas
approved these changes
May 14, 2024
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
May 15, 2024

Closed


This was referenced May 15, 2024

Closed


Closed

@jkotas
jkotas
merged commit 426edd0
into
dotnet:mainMay 15, 2024
stephentoub
stephentoub
reviewed
May 15, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
 {
 context.RegisterPostInitializationOutput(ctx =>
 {
 string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
@stephentoubstephentoubMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?
@jkotasjkotasMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

As far as I can tell, the build system stamps the exact same AssemblyInformationalVersionAttribute on all binaries.
Ruihan-Yin pushed a commit
to Ruihan-Yin/runtime
that referenced
this pull request
May 30, 2024
@am11@jkotas@Ruihan-Yin
Remove reflection from FrameworkDescription (dotnet#102164)
c100549
* 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>
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jun 15, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas approved these changes

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@am11@jkotas@stephentoub

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Remove reflection from FrameworkDescription - #102164
Merged
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
May 15, 2024
Merged
Remove reflection from FrameworkDescription#102164
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
Conversation
@am11
@am11am11
commented
May 13, 2024

Copy link


Copy Markdown

Member
Tiny help for trimming; shaved 2KB off of artifacts/bin/coreclr/osx.arm64.Release/System.Private.CoreLib.dll.
@ghostghost
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-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 13, 2024
@am11am11
added

area-System.Runtime

community-contribution
Indicates that the PR has been added by a community member
and removed

community-contribution
Indicates that the PR has been added by a community member
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
May 13, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
May 13, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

 }
 }

 internal const string InternalVersion = "9.0.0";
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@am11am11May 13, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ah, the patch version. 🤭
Let me think a bit how to dedup it.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment thread...untime/tests/System.Runtime.InteropServices.RuntimeInformation.Tests/DescriptionNameTests.cs

 Assert.DoesNotContain("+", version); // no git hash

#if STABILIZE_PACKAGE_VERSION
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should stay as is. It is a regression test for a bug that slipped through the cracks some time ago.
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
4c08f94 to
db1420dCompareMay 14, 2024 01:48
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated

@am11
am11force-pushed
the
feature/versioning/cleanups
branch
2 times, most recently
from
80d8f1f to
791a6e7CompareMay 14, 2024 07:03
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
791a6e7 to
c1ce415CompareMay 14, 2024 07:10
am11
am11
commented
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
@@ -5,6 +5,7 @@
 </PropertyGroup>
@am11am11May 14, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

There may be cases where the generator runs on .NET Framework if people open the project in Visual Studio.
@am11
am11
commented
May 14, 2024

Copy link


Copy Markdown

MemberAuthor
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
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
Outdated

@am11@jkotas
Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGen…
bd4f66e
…erator.cs
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas
jkotas
commented
May 14, 2024

Copy link


Copy Markdown

Member
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
jkotas
approved these changes
May 14, 2024
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
May 15, 2024

Closed


This was referenced May 15, 2024

Closed


Closed

@jkotas
jkotas
merged commit 426edd0
into
dotnet:mainMay 15, 2024
stephentoub
stephentoub
reviewed
May 15, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
 {
 context.RegisterPostInitializationOutput(ctx =>
 {
 string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
@stephentoubstephentoubMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?
@jkotasjkotasMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

As far as I can tell, the build system stamps the exact same AssemblyInformationalVersionAttribute on all binaries.
Ruihan-Yin pushed a commit
to Ruihan-Yin/runtime
that referenced
this pull request
May 30, 2024
@am11@jkotas@Ruihan-Yin
Remove reflection from FrameworkDescription (dotnet#102164)
c100549
* 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>
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jun 15, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas approved these changes

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@am11@jkotas@stephentoub

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Remove reflection from FrameworkDescription - #102164
Merged
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
May 15, 2024
Merged
Remove reflection from FrameworkDescription#102164
jkotas merged 7 commits into
dotnet:mainfrom 
am11:feature/versioning/cleanups
Conversation
@am11
@am11am11
commented
May 13, 2024

Copy link


Copy Markdown

Member
Tiny help for trimming; shaved 2KB off of artifacts/bin/coreclr/osx.arm64.Release/System.Private.CoreLib.dll.
@ghostghost
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-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 13, 2024
@am11am11
added

area-System.Runtime

community-contribution
Indicates that the PR has been added by a community member
and removed

community-contribution
Indicates that the PR has been added by a community member
needs-area-label
An area label is needed to ensure this gets routed to the appropriate area owners
labels
May 13, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
May 13, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @dotnet/area-system-runtime

See info in area-owners.md if you want to be subscribed.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

 }
 }

 internal const string InternalVersion = "9.0.0";
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@am11am11May 13, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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.
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ah, the patch version. 🤭
Let me think a bit how to dedup it.
jkotas
jkotas
reviewed
May 13, 2024
View reviewed changes
Comment thread...untime/tests/System.Runtime.InteropServices.RuntimeInformation.Tests/DescriptionNameTests.cs

 Assert.DoesNotContain("+", version); // no git hash

#if STABILIZE_PACKAGE_VERSION
@jkotasjkotasMay 13, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think this should stay as is. It is a regression test for a bug that slipped through the cracks some time ago.
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
4c08f94 to
db1420dCompareMay 14, 2024 01:48
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Environment.cs
Outdated

jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated

@am11
am11force-pushed
the
feature/versioning/cleanups
branch
2 times, most recently
from
80d8f1f to
791a6e7CompareMay 14, 2024 07:03
@am11
am11force-pushed
the
feature/versioning/cleanups
branch
from
791a6e7 to
c1ce415CompareMay 14, 2024 07:10
am11
am11
commented
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
@@ -5,6 +5,7 @@
 </PropertyGroup>
@am11am11May 14, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

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

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

There may be cases where the generator runs on .NET Framework if people open the project in Visual Studio.
@am11
am11
commented
May 14, 2024

Copy link


Copy Markdown

MemberAuthor
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
jkotas
jkotas
reviewed
May 14, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
Outdated

@am11@jkotas
Update src/libraries/System.Private.CoreLib/gen/ProductVersionInfoGen…
bd4f66e
…erator.cs
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas
jkotas
commented
May 14, 2024

Copy link


Copy Markdown

Member
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
jkotas
approved these changes
May 14, 2024
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thank you!
@build-analysisbuild-analysisBot
mentioned this pull request
May 15, 2024

Closed


This was referenced May 15, 2024

Closed


Closed

@jkotas
jkotas
merged commit 426edd0
into
dotnet:mainMay 15, 2024
stephentoub
stephentoub
reviewed
May 15, 2024
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/gen/ProductVersionInfoGenerator.cs
 {
 context.RegisterPostInitializationOutput(ctx =>
 {
 string? informationalVersion = typeof(ProductVersionInfoGenerator).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
@stephentoubstephentoubMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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?
@jkotasjkotasMay 15, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

As far as I can tell, the build system stamps the exact same AssemblyInformationalVersionAttribute on all binaries.
Ruihan-Yin pushed a commit
to Ruihan-Yin/runtime
that referenced
this pull request
May 30, 2024
@am11@jkotas@Ruihan-Yin
Remove reflection from FrameworkDescription (dotnet#102164)
c100549
* 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>
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jun 15, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments
@jkotasjkotasjkotas approved these changes

Assignees


No one assigned


Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@am11@jkotas@stephentoub