Skip to content

Conversation

@omajid
Copy link
Member

@omajid omajid commented Mar 9, 2024

The VMR defines DotNetGitCommitHash when building runtime. Use that to populate the .version file and corehost's commit hash. That results in showing the VMR's commit hash in the dotnet --info output.

Contributes to dotnet/source-build#3643

The VMR defines DotNetGitCommitHash when building runtime. Use that to
populate the .version file and corehost's commit hash. That results in
showing the VMR's commit hash in the `dotnet --info` output.

Contributes to dotnet/source-build#3643
@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 9, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 9, 2024
<_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

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

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

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.

@AaronRobinsonMSFT
Copy link
Member

/cc @elinor-fung

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

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>

@omajid
Copy link
Member Author

omajid commented Mar 12, 2024

Based on the feedback I got at dotnet/installer#18941, I am going to discard this PR. It seems we can get all the benefits of this change without any code changes (and even reduce infra/maintenance).

@omajid omajid closed this Mar 19, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants