Skip to content
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.

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

Use the VMR commit hash for dotnet --info #18941

Closed
wants to merge 1 commit into from

Conversation

omajid
Copy link
Member

@omajid omajid commented Mar 9, 2024

Remove the logic that overrides the git commit info for each repo. Let each repo infer the VMR's git commit (via sourcelink).

That will let the installer and sdk repos use the VMR's commit hash and show that in dotnet --info.

This works for both when building the VMR out of git, as well as when building it from a tarball/archive thanks to sourcelink's support for minimal git metadata [1].

[1] https://github.com/dotnet/sourcelink/tree/main/docs#minimal-git-repository-metadata

Fixes: dotnet/source-build#3643


Original PR description:

This defines a new environment variable DotNetGitCommitHash which contains the commit hash of the VMR. Individual repositories can use this to find the VMR's commit hash and then embed/use it as appropriate.

It then uses this hash when generating the SDK's .version file, so dotnet --info shows this as the SDK version.

Contributes to dotnet/source-build#3643

@omajid omajid requested review from a team as code owners March 9, 2024 14:32

<ReadLinesFromFile File="$(RepoRoot)/.git/HEAD">
<Output TaskParameter="Lines" PropertyName="GitHeadContents" />
</ReadLinesFromFile>
Copy link
Member Author

Choose a reason for hiding this comment

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

Executing git rev-parse HEAD and reading .git/HEAD is something sourcelink already does: https://github.com/dotnet/sourcelink/tree/main/docs#minimal-git-repository-metadata. Anyone know of an existing sourcelink task that we can re-use here to avoid this manual work?

Copy link
Member

Choose a reason for hiding this comment

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

yes - check out Microsoft.Build.Tasks.Git.LocateRepository. You can see it in action in a 'normal' build during the InitializeSourceControlInformationFromSourceControlManager target, but its contents is the following:

  <Target Name="InitializeSourceControlInformationFromSourceControlManager">
    <!--
      Reports a warning if the given project doesn't belong to a repository under source control,
      unless the targets were implicily imported from an SDK without a package reference.
    -->
    <Microsoft.Build.Tasks.Git.LocateRepository 
      Path="$(MSBuildProjectDirectory)"
      RemoteName="$(GitRepositoryRemoteName)"
      ConfigurationScope="$(GitRepositoryConfigurationScope)"
      NoWarnOnMissingInfo="$(PkgMicrosoft_Build_Tasks_Git.Equals(''))">

      <Output TaskParameter="RepositoryId" PropertyName="_GitRepositoryId" />
      <Output TaskParameter="Url" PropertyName="ScmRepositoryUrl" />
      <Output TaskParameter="Roots" ItemName="SourceRoot" />
      <Output TaskParameter="RevisionId" PropertyName="SourceRevisionId" Condition="'$(SourceRevisionId)' == ''" />
    </Microsoft.Build.Tasks.Git.LocateRepository>

    <PropertyGroup>
      <RepositoryType Condition="'$(RepositoryType)' == ''">git</RepositoryType>
    </PropertyGroup>
  </Target>

If you'd rather re-use already-computed information, you could see if the @(SourceRoot) items have been populated yet. SourceLink will generate and add metadata to those, so you could get the same information by finding the SourceRoot item with a SourceControl metadata of git and then reading the RevisionId metadata from that item:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

</PropertyGroup>

<ItemGroup>
<EnvironmentVariables Include="DotNetGitCommitHash=$(DotNetGitCommitHash)" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this need to be documented somewhere?

@omajid omajid force-pushed the vmr-commit-in-dotnet-info branch from ce60b2b to 368c806 Compare March 9, 2024 14:43
@omajid omajid changed the title Pass the VMR commit hash to individual repos Use the VMR commit hash for dotnet --info Mar 9, 2024
@omajid omajid force-pushed the vmr-commit-in-dotnet-info branch from 368c806 to 921f038 Compare March 9, 2024 14:54
@ViktorHofer
Copy link
Member

ViktorHofer commented Mar 10, 2024

The VMR's commit will be identical with the sdk's and installer's commit as part of the ongoing work during NET 9. I'm not sure if this is then needed at all. cc @MichaelSimons @mmitche

But maybe I'm missing something obvious here.

@MichaelSimons
Copy link
Member

The VMR's commit will be identical with the sdk's and installer's commit as part of the ongoing work during NET 9. I'm not sure if this is then needed at all. cc @MichaelSimons @mmitche

But maybe I'm missing something obvious here.

Some change is going to be needed. Right now the git-info properties, specifically the GitCommitHash property is "getting in the way" and causing the installer to pick up the installer commit sha which was mirrored into the VMR to be used.

@ViktorHofer
Copy link
Member

ViktorHofer commented Mar 12, 2024

Some change is going to be needed. Right now the git-info properties, specifically the GitCommitHash property is "getting in the way" and causing the installer to pick up the installer commit sha which was mirrored into the VMR to be used.

Right. IIRC we discussed that we will get rid of these git commits with the full VMR experience. dotnet/source-build#3934 is the epic that tracks this. We will soon start with that work.

@omajid
Copy link
Member Author

omajid commented Mar 12, 2024

It sounds like I should close all my PRs that are trying to add the VMR-specific hash, then?

@ViktorHofer
Copy link
Member

We will discuss this offline and get back to you shortly. Sorry for the inconvenience here.

@omajid
Copy link
Member Author

omajid commented Mar 12, 2024

No worries! I don't want to step on any toes here.

@mmitche
Copy link
Member

mmitche commented Mar 12, 2024

@omajid I think we want the following state:

  • When building from a git clone of the VMR, the sha of the VMR is used for all of the repo builds and is automatically detected.
  • When building from a tarball of the VMR, the sha of the VMR is passed in.

When building in a git repo, all of the default infra (sourcelink) should detect the sha seamlessly. All of the git-info sha information can simply be removed, and https://github.com/dotnet/dotnet/blob/main/repo-projects/Directory.Build.props#L140-L147 should just not be passed at all.

When building outside a git repo, we need to set the envvars https://github.com/dotnet/dotnet/blob/main/repo-projects/Directory.Build.props#L140-L147 appropriately based on the --source param passed to the build script, My guess is that a number of these params are no longer in use. I think some git grepping should tell you.

In either case, the git sha properties from the git info files (e.g. https://github.com/dotnet/dotnet/blob/main/prereqs/git-info/wpf.props#L4 https://github.com/dotnet/dotnet/blob/main/prereqs/git-info/AllRepoVersions.props) are unneeded at this point and can be removed.

So in other words, I think the overall infra should get simpler with less VMR-specific info.

@mmitche
Copy link
Member

mmitche commented Mar 12, 2024

No worries! I don't want to step on any toes here.

And you're definitely not stepping on toes. Any help is appreciated.

@MichaelSimons
Copy link
Member

In either case, the git sha properties from the git info files (e.g. https://github.com/dotnet/dotnet/blob/main/prereqs/git-info/wpf.props#L4 https://github.com/dotnet/dotnet/blob/main/prereqs/git-info/AllRepoVersions.props) are unneeded at this point and can be removed.

These files are generated by the vmr synchronization process. You can track this down by starting at the GitInfoFile model in arcade services.

@omajid
Copy link
Member Author

omajid commented Mar 12, 2024

When building outside a git repo, we need to set the envvars https://github.com/dotnet/dotnet/blob/main/repo-projects/Directory.Build.props#L140-L147 appropriately based on the --source param passed to the build script

I wonder if we can avoid this too. Sourcelink, which computes git information in a git-repo-scenario, already knows how to handle the --source params from build.sh.

@omajid
Copy link
Member Author

omajid commented Mar 18, 2024

This set of changes work for me: omajid/dotnet-dotnet@1fe5b89. I can build a git-archive from the VMR with these changes.

@mmitche is this along the lines of what you had in mind? Shall I clean it up and do PRs?

omajid added a commit to omajid/dotnet-aspnetcore that referenced this pull request Mar 19, 2024
This reverts commit 818d0e3.

Over at dotnet/installer#18941, we decied that
we wanted to go in a different (and simpler!) direction. We will make
sure `SourceRevisionId` is defined/used correctly everywhere.
@mmitche
Copy link
Member

mmitche commented Mar 19, 2024

This set of changes work for me: omajid/dotnet-dotnet@1fe5b89. I can build a git-archive from the VMR with these changes.

@mmitche is this along the lines of what you had in mind? Shall I clean it up and do PRs?

I think this looks correct. Only possibility is that maybe source-build-externals will do something odd? Since it still clones submodules. Anyways, because we don't pass in explicit properties for the source version in our official builds, it stands to reason that simply allowing the repo infra to do whatever it wants will work just fine.

I would clean it up and do PRs. After the use of the commit hashes is removed, we can then remove the generation of that part of those files.

wtgodbe pushed a commit to dotnet/aspnetcore that referenced this pull request Mar 19, 2024
This reverts commit 818d0e3.

Over at dotnet/installer#18941, we decied that
we wanted to go in a different (and simpler!) direction. We will make
sure `SourceRevisionId` is defined/used correctly everywhere.
@omajid
Copy link
Member Author

omajid commented Mar 20, 2024

GitCommitCount/git is used by GenerateMSIs.targets

<Exec Command="git rev-list --count HEAD"
ConsoleToMSBuild="true"
Condition=" '$(GitCommitCount)' == '' AND '$(_PatchNumber)' == '' ">
<Output TaskParameter="ConsoleOutput" PropertyName="GitCommitCount" />
</Exec>

Us distro folks who use the VMR git archive tarball don't generate MSIs, so that shouldn't matter.

@premun
Copy link
Member

premun commented Mar 21, 2024

@omajid thanks for taking care of this!

I am only a bystander in this effort but once you are done with the whole feature change, can you please let me know and I can make the VMR synchronization tooling stop producing the git-info files?

Thanks!

@omajid
Copy link
Member Author

omajid commented Mar 22, 2024

I found something that I can't figure out.

The file ./artifacts/obj/installer/artifacts-layout/.version is supposed to contain the commit sha too. It is generated by https://github.com/dotnet/dotnet/blob/a6e7f40ccdcea190d96ba5915bff6269ff569500/repo-projects/installer.proj#L119-L126

However, in that file, I can't use SourceLink targets (like IncludeSourceRevisionInInformationalVersion) or SDK properties like SourceRevisionId. Any idea why that is or how to work around it?

@mmitche
Copy link
Member

mmitche commented Mar 22, 2024

I found something that I can't figure out.

The file ./artifacts/obj/installer/artifacts-layout/.version is supposed to contain the commit sha too. It is generated by https://github.com/dotnet/dotnet/blob/a6e7f40ccdcea190d96ba5915bff6269ff569500/repo-projects/installer.proj#L119-L126

However, in that file, I can't use SourceLink targets (like IncludeSourceRevisionInInformationalVersion) or SDK properties like SourceRevisionId. Any idea why that is or how to work around it?

Based on some grepping and https://github.com/dotnet/dotnet/blob/main/repo-projects/installer.proj#L4-L5 It looks like RepositoryCommit should be set by the arcade infra. And there's no suggestion that this is set explicitly in the outer build or repo projects at all. What are you seeing?

@omajid
Copy link
Member Author

omajid commented Mar 22, 2024

What are you seeing?

Building the VMR git repo shows:

$ cat ./artifacts/obj/installer/artifacts-layout/.version
0000000000000000000000000000000000000000
9.0.100-preview.3.24164.1

The 0000... version seems to come from https://github.com/dotnet/dotnet/blob/c8e40d153a3bc95c4620e8a0b5e8c2439667a3ea/src/arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/RepositoryInfo.targets#L27-L30:

  <PropertyGroup Condition="'$(EnableSourceControlManagerQueries)' != 'true' and '$(DesignTimeBuild)' != 'true' and '$(RepositoryCommit)' == ''">
    <RepositoryCommit Condition="'$(BUILD_SOURCEVERSION)' != ''">$(BUILD_SOURCEVERSION)</RepositoryCommit>
    <RepositoryCommit Condition="'$(BUILD_SOURCEVERSION)' == ''">0000000000000000000000000000000000000000</RepositoryCommit>
  </PropertyGroup>

And BUILD_SOURCEVERSION is now unset due to my changes.

So I tried using SourceRevisionId which is set by the SDK automatically, but that seems to not getting set for some reason.

@omajid
Copy link
Member Author

omajid commented Mar 22, 2024

Err. Nevermind, this file contains 000000.. as the commit version even without any of my changes:

$ git rev-parse HEAD
d7e4d18ba4d64b96381495ae0f5f72bad6cba58b
$ eng/prep-source-build.sh && ./build.sh --source-build
...
$ cat ./artifacts/obj/installer/artifacts-layout/.version
0000000000000000000000000000000000000000
9.0.100-preview.3.24164.1

@omajid
Copy link
Member Author

omajid commented Apr 3, 2024

Remove the logic that overrides the git commit info for each repo. Let
each repo infer the VMR's git commit (via sourcelink).

That will let the installer and sdk repos use the VMR's commit hash and
show that in `dotnet --info`.

This works for both when building the VMR out of git, as well as when
building it from a tarball/archive thanks to sourcelink's support for
minimal git metadata [1].

[1] https://github.com/dotnet/sourcelink/tree/main/docs#minimal-git-repository-metadata

Fixes: dotnet/source-build#3643
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Looks great and a little bit like magic. What steps did you perform to validate these changes? @MichaelSimons @mmitche can you please also take a look?

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

As part of the installer->sdk repository consolidation we are already switching the VMR over into dotnet/sdk. ETA: today or tomorrow. The last manual code sync happened today and we don't plan on doing another sync. Please close this PR and submit it into dotnet/sdk. Sorry for the inconvenience.

@omajid
Copy link
Member Author

omajid commented May 22, 2024

What steps did you perform to validate these changes?

I tested with these two commits:

commit bf3357462de7ef82156db43bfc54581d9db88c49 (HEAD -> no-more-hashes-try-3)
Author: Omair Majid <omajid@redhat.com>
Date:   Tue May 14 11:21:10 2024 -0400

    WIP: No more hashes

commit ce1e686e7410d1835ff33a4de2e978daa713fa7d (upstream/main, upstream/HEAD, main)
Author: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Date:   Mon May 13 19:52:12 2024 +0000

    [Recursive sync] sdk / 37b44c6 → 37b44c6

    Updated repositories:

    [[ commit created by automation ]]

The WIP commit is just the changes from this PR applied directly.

I built tarballs from both of these commits.

Then I built both the tarballs using the same command (which forces the commit bf3357 to pretend it's ce1e686 instead):

$ ./prep-source-build.sh && ./build.sh --source-only --source-repository https://github.com/dotnet/dotnet --source-version ce1e686e7410d1835ff33a4de2e978daa713fa7d

I then processed the built SDKs. I normalized the json files to sort the keys and replace all hash values with a constant string. And then a diff of the non-binary results in:

$ diff -ruN dotnet-sdk-ce1e686e7410d1835ff33a4de2e978daa713fa7d/ dotnet-sdk-bf3357462de7ef82156db43bfc54581d9db88c49/ | grep -v '^Binary files'
diff -ruN dotnet-sdk-ce1e686e7410d1835ff33a4de2e978daa713fa7d/sdk/9.0.100-preview.5.24263.1/.toolsetversion dotnet-sdk-bf3357462de7ef82156db43bfc54581d9db88c49/sdk/9.0.100-preview.5.24263.1/.toolsetversion
--- dotnet-sdk-ce1e686e7410d1835ff33a4de2e978daa713fa7d/sdk/9.0.100-preview.5.24263.1/.toolsetversion   2024-05-22 14:26:27.000000000 -0400
+++ dotnet-sdk-bf3357462de7ef82156db43bfc54581d9db88c49/sdk/9.0.100-preview.5.24263.1/.toolsetversion   2024-05-22 15:04:40.000000000 -0400
@@ -1,3 +1,3 @@
-37b44c6e724a7b078331debc315ca335f1a2be9e
+ce1e686e7410d1835ff33a4de2e978daa713fa7d
 9.0.100-preview.5.24263.1
 fedora.40-x64
diff -ruN dotnet-sdk-ce1e686e7410d1835ff33a4de2e978daa713fa7d/sdk/9.0.100-preview.5.24263.1/.version dotnet-sdk-bf3357462de7ef82156db43bfc54581d9db88c49/sdk/9.0.100-preview.5.24263.1/.version
--- dotnet-sdk-ce1e686e7410d1835ff33a4de2e978daa713fa7d/sdk/9.0.100-preview.5.24263.1/.version  2024-05-22 14:27:02.000000000 -0400
+++ dotnet-sdk-bf3357462de7ef82156db43bfc54581d9db88c49/sdk/9.0.100-preview.5.24263.1/.version  2024-05-22 15:05:16.000000000 -0400
@@ -1,4 +1,4 @@
-37b44c6e724a7b078331debc315ca335f1a2be9e
+ce1e686e7410d1835ff33a4de2e978daa713fa7d
 9.0.100-preview.5.24263.1
 fedora.40-x64
 9.0.100-preview.5.24263.1
diff -ruN dotnet-sdk-ce1e686e7410d1835ff33a4de2e978daa713fa7d/shared/Microsoft.AspNetCore.App/9.0.0-preview.5.24260.1/.version dotnet-sdk-bf3357462de7ef82156db43bfc54581d9db88c49/shared/Microsoft.AspNetCore.App/9.0.0-preview.5.24260.1/.version
--- dotnet-sdk-ce1e686e7410d1835ff33a4de2e978daa713fa7d/shared/Microsoft.AspNetCore.App/9.0.0-preview.5.24260.1/.version        2024-05-22 14:20:38.000000000 -0400
+++ dotnet-sdk-bf3357462de7ef82156db43bfc54581d9db88c49/shared/Microsoft.AspNetCore.App/9.0.0-preview.5.24260.1/.version        2024-05-22 14:58:56.000000000 -0400
@@ -1,2 +1,2 @@
-0e7349e41a112a0c62f364ce9666a1a7dd4dc2fd
+ce1e686e7410d1835ff33a4de2e978daa713fa7d
 9.0.0-preview.5.24260.1
diff -ruN dotnet-sdk-ce1e686e7410d1835ff33a4de2e978daa713fa7d/shared/Microsoft.NETCore.App/9.0.0-preview.5.24262.2/.version dotnet-sdk-bf3357462de7ef82156db43bfc54581d9db88c49/shared/Microsoft.NETCore.App/9.0.0-preview.5.24262.2/.version
--- dotnet-sdk-ce1e686e7410d1835ff33a4de2e978daa713fa7d/shared/Microsoft.NETCore.App/9.0.0-preview.5.24262.2/.version   2024-05-22 14:14:07.000000000 -0400
+++ dotnet-sdk-bf3357462de7ef82156db43bfc54581d9db88c49/shared/Microsoft.NETCore.App/9.0.0-preview.5.24262.2/.version   2024-05-22 14:52:26.000000000 -0400
@@ -1,2 +1,2 @@
-073e35e7e32624e0f841d94364d2f28c73662ff7
+ce1e686e7410d1835ff33a4de2e978daa713fa7d
 9.0.0-preview.5.24262.2

Is there any tool to help me diff the binaries to see the actual differences?

Any suggestions on anything else I should test?

@akoeplinger
Copy link
Member

@omajid looks good to me, I think we should continue the discussion in a new PR in dotnet/sdk as I'm not sure all people pay attention to closed PRs.

omajid added a commit to omajid/dotnet-sdk that referenced this pull request May 27, 2024
Remove the logic that overrides the git commit info for each repo. Let
each repo infer the VMR's git commit (via sourcelink).

That will let the sdk, runtime, aspnetcore (and other) repos use the
VMR's commit hash and show that in dotnet --info.

This works for both when building the VMR directly in a git repository,
as well as when building it from a git-archive tarball thanks to
sourcelink's support for minimal git metadata [1].

This is a port of dotnet/installer#18941 to
dotnet/sdk since the installer was merged into sdk.

[1] https://github.com/dotnet/sourcelink/tree/main/docs#minimal-git-repository-metadata

Fixes: dotnet/source-build#3643
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show VMR commit hash under dotnet --info for source-built SDKs
7 participants