Skip to content

Show the VMR commit hash in dotnet --info #99482

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.

Sign up for GitHub

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

Closed
omajid wants to merge 1 commit into from
Closed

Show the VMR commit hash in dotnet --info #99482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Runtime.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,12 @@
<ItemGroup>
<_VersionFile Include="$(IntermediateOutputPath).version" TargetPath="shared/$(SharedFrameworkName)/$(Version)/" />
</ItemGroup>
<PropertyGroup>
<CommitHashForVersionFile Condition="'$(DotNetGitCommitHash)' != ''">$(DotNetGitCommitHash)</CommitHashForVersionFile>
<CommitHashForVersionFile Condition="'$(DotNetGitCommitHash)' == ''">$(SourceRevisionId)</CommitHashForVersionFile>
</PropertyGroup>
<WriteLinesToFile
Lines="$(SourceRevisionId);$(Version)"
Lines="$(CommitHashForVersionFile);$(Version)"
File="@(_VersionFile)"
Overwrite="true"
WriteOnlyWhenDifferent="true" />
Expand Down
6 changes: 5 additions & 1 deletion src/native/corehost/corehost.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,13 @@
<CMakeBuildDir>$(IntermediateOutputRootPath)corehost\cmake\</CMakeBuildDir>
<BuildScript>$([MSBuild]::NormalizePath('$(MSBuildThisFileDirectory)', 'build.sh'))</BuildScript>

<CommitHash Condition="('$(CommitHash)' == '') and ('$(DotNetGitCommitHash)' != '')">$(DotNetGitCommitHash)</CommitHash>
<CommitHash Condition="('$(CommitHash)' == '') and ('$(SourcerevisionId)' != '')">$(SourceRevisionId)</CommitHash>
Comment on lines +77 to +78
Copy link
Member

@elinor-fung elinor-fung Mar 12, 2024

Choose a reason for hiding this comment

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

nit: I find it easier to read when the property just gets set based on being empty and the order indicates the precedence.

Suggested change
<CommitHash Condition="('$(CommitHash)' == '') and ('$(DotNetGitCommitHash)' != '')">$(DotNetGitCommitHash)</CommitHash>
<CommitHash Condition="('$(CommitHash)' == '') and ('$(SourcerevisionId)' != '')">$(SourceRevisionId)</CommitHash>
<CommitHash Condition="'$(CommitHash)' == ''">$(DotNetGitCommitHash)</CommitHash>
<CommitHash Condition="'$(CommitHash)' == ''">$(SourceRevisionId)</CommitHash>

<CommitHash Condition="'$(CommitHash)' == ''">N/A</CommitHash>

<_CoreHostUnixTargetOS>$(TargetOS)</_CoreHostUnixTargetOS>
<_CoreHostUnixTargetOS Condition="'$(TargetsLinuxBionic)' == 'true'">linux-bionic</_CoreHostUnixTargetOS>
<BuildArgs>$(Configuration) $(TargetArchitecture) -commithash "$([MSBuild]::ValueOrDefault('$(SourceRevisionId)', 'N/A'))" -os $(_CoreHostUnixTargetOS)</BuildArgs>
<BuildArgs>$(Configuration) $(TargetArchitecture) -commithash "$(CommitHash)" -os $(_CoreHostUnixTargetOS)</BuildArgs>
Copy link
Member

@jkotas jkotas Mar 9, 2024

Choose a reason for hiding this comment

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

There is another use of SourcerevisionId at later in this file: https://github.com/dotnet/runtime/pull/99482/files#diff-165e4dabf2097f79d2ed18244f5a9762cfc4fe25a69f9be5e8585f61fc871523R146 . Should it get the same treatment?

Would it be better to set SourcerevisionId globally?

Copy link
Member Author

@omajid omajid Mar 9, 2024

Choose a reason for hiding this comment

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

Should it get the same treatment?

I think it should. It does the same thing as this, except for Windows, right? I think we want the Unified Behaviour to be the same: show VMR hash with dotnet --info.

Would it be better to set SourcerevisionId globally?

Probably not. That's used by sourcelink, which we prefer to point to the indvidual repos (though I am not sure why).

@NikolaMilosavljevic , any thoughts?

Copy link
Member

@elinor-fung elinor-fung Mar 12, 2024

Choose a reason for hiding this comment

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

I think it should. It does the same thing as this, except for Windows, right?

Yes, same thing, except for Windows. Agree it should have the same behaviour.

<BuildArgs>$(BuildArgs) -cmakeargs "-DVERSION_FILE_PATH=$(NativeVersionFile)"</BuildArgs>
<BuildArgs Condition="'$(ConfigureOnly)' == 'true'">$(BuildArgs) -configureonly</BuildArgs>
<BuildArgs Condition="'$(PortableBuild)' != 'true'">$(BuildArgs) -portablebuild=false</BuildArgs>
Expand Down