Add arcade-powered source-build basic configuration and patches #651
Conversation
| internal string? ReadSubmoduleHeadCommitSha(string submoduleWorkingDirectoryFullPath) | ||
| { | ||
| if (Directory.Exists(Path.Combine(submoduleWorkingDirectoryFullPath, ".git"))) | ||
| { | ||
| return File.ReadAllText(Path.Combine(submoduleWorkingDirectoryFullPath, ".git", "HEAD")); | ||
| } |
There was a problem hiding this comment.
@crummel can you give some context on this patch? My understanding has been that a submodule by definition has a .git file that points to the superproject's Git data for that submodule. Maybe something's being treated as a submodule that shouldn't be?
There was a problem hiding this comment.
This patch seems to be breaking tests around this scenario, which makes sense:
sourcelink/src/Microsoft.Build.Tasks.Git.UnitTests/GitRepositoryTests.cs
Lines 267 to 269 in 8a3edd1
I'm going to revert it for now.
There was a problem hiding this comment.
The idea here was to use the Minimal git repository metadata support. Source-build doesn't have real repos (we purposely use non-git source directories to reduce confusion and tarball size), so we write out .git/HEAD to these folders to make sourcelink work. This didn't work in submodules because sourcelink always treats submodules as having .gitdir redirects instead of real .git folders (which in a normal situation is true). I'll take a look and try to figure out a clean fix.
There was a problem hiding this comment.
Opened #653 for a better version of this patch.
eng/common/tools.ps1
Outdated
| @@ -312,9 +312,15 @@ function InitializeVisualStudioMSBuild([bool]$install, [object]$vsRequirements = | |||
| return $global:_MSBuildExe | |||
| } | |||
|
|
|||
| $vsMinVersionReqdStr = '16.5' | |||
| # Minimum VS version to require. | |||
There was a problem hiding this comment.
Minimum VS version to require. [](start = 2, length = 32)
Shouldn't this be auto-updated by Maestro?
There was a problem hiding this comment.
It was auto-updated by Darc in this case, in my first commit. And yeah, I pointed at #650 in my comment on this PR. 😄
There was a problem hiding this comment.
Rebased on top of the merge, now this PR just has the one commit adding basic arcade-powered-source build configuration.
| @@ -63,7 +63,7 @@ | |||
| </Microsoft.SourceLink.AzureDevOpsServer.Git.TranslateRepositoryUrls> | |||
|
|
|||
| <ItemGroup> | |||
| <SourceRoot Remove="@(SourceRoot)"/> | |||
| <SourceRoot Remove="@(SourceRoot)" Condition="'@(_TranslatedSourceRoot)' != ''"/> | |||
There was a problem hiding this comment.
" Condition="'@(_TranslatedSourceRoot)' != ''"/> [](start = 39, length = 48)
This is a product change. Is there an issue filed that this is fixing? We should also have a test covering this if this is indeed a product bug.
There was a problem hiding this comment.
@crummel do you have any extra info on this patch?
There was a problem hiding this comment.
I remember that TranslateRepositoryUrls was returning an empty list for TranslatedSourceRoots. Then with the removal of the original SourceRoots, we ended up with no SourceRoots at all, which caused the usual "no SourceRoots" error. I'll take a look at why this was happening.
There was a problem hiding this comment.
My test build looks good without this, so I think it was a temporary issue.
|
@tmat can you take another look? I removed the patches, so this is now only the minimal infra. We can investigate further on the need for the patches and integrate changes as needed later after discussion. |
This PR adds arcade-powered source-build configuration, so
./build.sh -c Release /p:ArcadeBuildFromSource=truecreates a source-build intermediate nuget package. It also adds patches that we have been maintaining in dotnet/source-build.