From 43b7eee41aedc5aeee7cc9974e5ec8c76975bff3 Mon Sep 17 00:00:00 2001 From: tmat Date: Wed, 5 Apr 2023 12:53:29 -0700 Subject: [PATCH] Avoid creating source link .json file when no source control mapping is available --- .../RepositoryTask.cs | 20 +++---- .../build/Microsoft.Build.Tasks.Git.targets | 3 +- .../GenerateSourceLinkFileTests.cs | 52 ++++++++++++++-- .../GenerateSourceLinkFile.cs | 59 +++++++++++++------ .../build/Microsoft.SourceLink.Common.targets | 17 +++--- .../CloudHostedProvidersTests.cs | 16 ++++- 6 files changed, 122 insertions(+), 45 deletions(-) diff --git a/src/Microsoft.Build.Tasks.Git/RepositoryTask.cs b/src/Microsoft.Build.Tasks.Git/RepositoryTask.cs index 16e9382e..2c412844 100644 --- a/src/Microsoft.Build.Tasks.Git/RepositoryTask.cs +++ b/src/Microsoft.Build.Tasks.Git/RepositoryTask.cs @@ -60,6 +60,14 @@ bool logAssemblyLoadingErrors() return !Log.HasLoggedErrors; } + private void ReportMissingRepositoryWarning(string initialPath) + { + if (!NoWarnOnMissingRepository) + { + Log.LogWarning(Resources.UnableToLocateRepository, initialPath); + } + } + private protected abstract void Execute(GitRepository repository); protected abstract string? GetRepositoryId(); @@ -103,11 +111,7 @@ private void ExecuteImpl() if (!GitRepository.TryFindRepository(initialPath, out var location)) { - if (!NoWarnOnMissingRepository) - { - Log.LogWarning(Resources.UnableToLocateRepository, initialPath); - } - + ReportMissingRepositoryWarning(initialPath); return null; } @@ -129,11 +133,7 @@ private void ExecuteImpl() if (repository?.WorkingDirectory == null) { - if (!NoWarnOnMissingRepository) - { - Log.LogWarning(Resources.UnableToLocateRepository, initialPath); - } - + ReportMissingRepositoryWarning(initialPath); repository = null; } diff --git a/src/Microsoft.Build.Tasks.Git/build/Microsoft.Build.Tasks.Git.targets b/src/Microsoft.Build.Tasks.Git/build/Microsoft.Build.Tasks.Git.targets index 12fec562..f2128f70 100644 --- a/src/Microsoft.Build.Tasks.Git/build/Microsoft.Build.Tasks.Git.targets +++ b/src/Microsoft.Build.Tasks.Git/build/Microsoft.Build.Tasks.Git.targets @@ -51,7 +51,8 @@ RepositoryId="$(_GitRepositoryId)" ConfigurationScope="$(GitRepositoryConfigurationScope)" ProjectDirectory="$(MSBuildProjectDirectory)" - Files="@(Compile)"> + Files="@(Compile)" + Condition="'$(_GitRepositoryId)' != ''"> diff --git a/src/SourceLink.Common.UnitTests/GenerateSourceLinkFileTests.cs b/src/SourceLink.Common.UnitTests/GenerateSourceLinkFileTests.cs index 07fa14be..b914a1b8 100644 --- a/src/SourceLink.Common.UnitTests/GenerateSourceLinkFileTests.cs +++ b/src/SourceLink.Common.UnitTests/GenerateSourceLinkFileTests.cs @@ -3,6 +3,7 @@ // See the License.txt file in the project root for more information. using System; using System.IO; +using System.Text; using Xunit; using TestUtilities; using static TestUtilities.KeyValuePairUtils; @@ -14,23 +15,56 @@ public class GenerateSourceLinkFileTests private static string AdjustSeparatorsInJson(string json) => Path.DirectorySeparatorChar == '/' ? json.Replace(@"\\", "/") : json; - [Fact] - public void Empty() + [Theory] + [CombinatorialData] + public void Empty(bool noWarning) { + var sourceLinkFilePath = Path.Combine(TempRoot.Root, Guid.NewGuid().ToString()); + var engine = new MockEngine(); var task = new GenerateSourceLinkFile() { BuildEngine = engine, + OutputFile = sourceLinkFilePath, SourceRoots = new MockItem[0], + NoWarnOnMissingSourceControlInformation = noWarning, }; - var content = task.GenerateSourceLinkContent(); + Assert.True(task.Execute()); AssertEx.AssertEqualToleratingWhitespaceDifferences( - "WARNING : " + string.Format(Resources.SourceControlInformationIsNotAvailableGeneratedSourceLinkEmpty), engine.Log); + noWarning ? "" : "WARNING : " + string.Format(Resources.SourceControlInformationIsNotAvailableGeneratedSourceLinkEmpty), + engine.Log); - AssertEx.AreEqual(@"{""documents"":{}}", content); + Assert.Null(task.SourceLink); + Assert.Null(task.FileWrite); + } + + [Fact] + public void Empty_DeleteExistingFile() + { + using var tempRoot = new TempRoot(); + + var sourceLinkFile = tempRoot.CreateFile(); + sourceLinkFile.WriteAllText("XYZ"); + + var engine = new MockEngine(); + + var task = new GenerateSourceLinkFile() + { + BuildEngine = engine, + OutputFile = sourceLinkFile.Path, + SourceRoots = new MockItem[0], + NoWarnOnMissingSourceControlInformation = true, + }; + + Assert.True(task.Execute()); + + AssertEx.AssertEqualToleratingWhitespaceDifferences("", engine.Log); + + Assert.Null(task.SourceLink); + Assert.Equal(sourceLinkFile.Path, task.FileWrite); } [Fact] @@ -145,7 +179,9 @@ public void DoesNotRewriteContentIfFileContentIsSame() { using var temp = new TempRoot(); var tempFile = temp.CreateFile(); - + + tempFile.WriteAllText("XYZ"); + var engine = new MockEngine(); var task = new GenerateSourceLinkFile() { @@ -161,6 +197,10 @@ public void DoesNotRewriteContentIfFileContentIsSame() var beforeWriteTime = File.GetLastWriteTime(tempFile.Path); + Assert.Equal(@"{""documents"":{""/_\""_/*"":""https://raw.githubusercontent.com/repo/*""}}", File.ReadAllText(tempFile.Path, Encoding.UTF8)); + Assert.Equal(tempFile.Path, task.SourceLink); + Assert.Equal(tempFile.Path, task.FileWrite); + result = task.Execute(); var afterWriteTime = File.GetLastWriteTime(tempFile.Path); diff --git a/src/SourceLink.Common/GenerateSourceLinkFile.cs b/src/SourceLink.Common/GenerateSourceLinkFile.cs index e46f9a91..db5d00e9 100644 --- a/src/SourceLink.Common/GenerateSourceLinkFile.cs +++ b/src/SourceLink.Common/GenerateSourceLinkFile.cs @@ -21,15 +21,23 @@ public sealed class GenerateSourceLinkFile : Task [Required, NotNull] public string? OutputFile { get; set; } + /// + /// Set to if the output file was written to, null otherwise. + /// + [Output] + public string? FileWrite { get; set; } + + /// + /// Set to if the output Source Link file should be passed to the compiler. + /// + [Output] + public string? SourceLink { get; set; } + public bool NoWarnOnMissingSourceControlInformation { get; set; } public override bool Execute() { - var content = GenerateSourceLinkContent(); - if (content != null) - { - WriteSourceLinkFile(content); - } + WriteSourceLinkFile(GenerateSourceLinkContent()); return !Log.HasLoggedErrors; } @@ -43,7 +51,7 @@ static string jsonEscape(string str) result.Append("{\"documents\":{"); bool success = true; - bool first = true; + bool isEmpty = true; foreach (var root in SourceRoots) { string mappedPath = root.GetMetadata(Names.SourceRoot.MappedPath); @@ -79,9 +87,9 @@ static string jsonEscape(string str) continue; } - if (first) + if (isEmpty) { - first = false; + isEmpty = false; } else { @@ -100,38 +108,51 @@ static string jsonEscape(string str) result.Append("}}"); - if (!success) - { - return null; - } + return success && !isEmpty ? result.ToString() : null; + } - if (first && !NoWarnOnMissingSourceControlInformation) + private void WriteSourceLinkFile(string? content) + { + if (content == null && !NoWarnOnMissingSourceControlInformation) { Log.LogWarning(Resources.SourceControlInformationIsNotAvailableGeneratedSourceLinkEmpty); } - return result.ToString(); - } - - private void WriteSourceLinkFile(string content) - { try { if (File.Exists(OutputFile)) { + if (content == null) + { + File.Delete(OutputFile); + FileWrite = OutputFile; + return; + } + var originalContent = File.ReadAllText(OutputFile); if (originalContent == content) { - // Don't rewrite the file if the contents are the same + // Don't rewrite the file if the contents is the same, just pass it to the compiler. + SourceLink = OutputFile; return; } } + else if (content == null) + { + // File doesn't exist and the output is empty: + // Do not write the file and don't pass it to the compiler. + return; + } File.WriteAllText(OutputFile, content); + FileWrite = SourceLink = OutputFile; } catch (Exception e) { Log.LogError(Resources.ErrorWritingToSourceLinkFile, OutputFile, e.Message); + + // Part of the file might have been written. + FileWrite = OutputFile; } } } diff --git a/src/SourceLink.Common/build/Microsoft.SourceLink.Common.targets b/src/SourceLink.Common/build/Microsoft.SourceLink.Common.targets index add71e29..ea05b196 100644 --- a/src/SourceLink.Common/build/Microsoft.SourceLink.Common.targets +++ b/src/SourceLink.Common/build/Microsoft.SourceLink.Common.targets @@ -7,7 +7,7 @@ - $(IntermediateOutputPath)$(MSBuildProjectName).sourcelink.json + <_SourceLinkFilePath>$(IntermediateOutputPath)$(MSBuildProjectName).sourcelink.json @@ -48,20 +48,23 @@ + Outputs="$(_SourceLinkFilePath)"> + OutputFile="$(_SourceLinkFilePath)"> - - - + + + + + + - + %(Link.AdditionalOptions) /sourcelink:"$(SourceLink)" diff --git a/src/SourceLink.Git.IntegrationTests/CloudHostedProvidersTests.cs b/src/SourceLink.Git.IntegrationTests/CloudHostedProvidersTests.cs index c485eef9..824ae088 100644 --- a/src/SourceLink.Git.IntegrationTests/CloudHostedProvidersTests.cs +++ b/src/SourceLink.Git.IntegrationTests/CloudHostedProvidersTests.cs @@ -26,6 +26,8 @@ public CloudHostedProvidersTests() [ConditionalFact(typeof(DotNetSdkAvailable))] public void NoRepository_Warnings() { + var sourceLinkFilePath = Path.Combine(ProjectObjDir.Path, Configuration, TargetFramework, "test.sourcelink.json"); + VerifyValues( customProps: "", customTargets: "", @@ -36,21 +38,27 @@ public void NoRepository_Warnings() expressions: new[] { "@(SourceRoot)", + "$(SourceLink)", }, expectedResults: new[] { - NuGetPackageFolders + NuGetPackageFolders, + "", }, expectedWarnings: new[] { string.Format(Build.Tasks.Git.Resources.UnableToLocateRepository, ProjectDir.Path), string.Format(Common.Resources.SourceControlInformationIsNotAvailableGeneratedSourceLinkEmpty), }); + + Assert.False(File.Exists(sourceLinkFilePath)); } [ConditionalFact(typeof(DotNetSdkAvailable))] public void NoRepository_NoWarnings() { + var sourceLinkFilePath = Path.Combine(ProjectObjDir.Path, Configuration, TargetFramework, "test.sourcelink.json"); + VerifyValues( customProps: """ @@ -66,11 +74,15 @@ public void NoRepository_NoWarnings() expressions: new[] { "@(SourceRoot)", + "$(SourceLink)", }, expectedResults: new[] { - NuGetPackageFolders + NuGetPackageFolders, + "", }); + + Assert.False(File.Exists(sourceLinkFilePath)); } [ConditionalFact(typeof(DotNetSdkAvailable))]