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

Fail Restore when an SDK is unresolved or entry target does not exist #6312

Merged
merged 2 commits into from Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions ref/Microsoft.Build/net/Microsoft.Build.cs
Expand Up @@ -772,6 +772,7 @@ public enum ProjectLoadSettings
DoNotEvaluateElementsWithFalseCondition = 32,
IgnoreInvalidImports = 64,
ProfileEvaluation = 128,
FailOnUnresolvedSdk = 256,
}
public partial class ProjectMetadata : System.IEquatable<Microsoft.Build.Evaluation.ProjectMetadata>
{
Expand Down Expand Up @@ -1047,6 +1048,8 @@ public enum BuildRequestDataFlags
SkipNonexistentTargets = 16,
ProvideSubsetOfStateAfterBuild = 32,
IgnoreMissingEmptyAndInvalidImports = 64,
SkipNonexistentNonTopLevelTargets = 128,
FailOnUnresolvedSdk = 256,
}
public partial class BuildResult
{
Expand Down
3 changes: 3 additions & 0 deletions ref/Microsoft.Build/netstandard/Microsoft.Build.cs
Expand Up @@ -772,6 +772,7 @@ public enum ProjectLoadSettings
DoNotEvaluateElementsWithFalseCondition = 32,
IgnoreInvalidImports = 64,
ProfileEvaluation = 128,
FailOnUnresolvedSdk = 256,
}
public partial class ProjectMetadata : System.IEquatable<Microsoft.Build.Evaluation.ProjectMetadata>
{
Expand Down Expand Up @@ -1042,6 +1043,8 @@ public enum BuildRequestDataFlags
SkipNonexistentTargets = 16,
ProvideSubsetOfStateAfterBuild = 32,
IgnoreMissingEmptyAndInvalidImports = 64,
SkipNonexistentNonTopLevelTargets = 128,
FailOnUnresolvedSdk = 256,
}
public partial class BuildResult
{
Expand Down
5 changes: 5 additions & 0 deletions src/Build/BackEnd/BuildManager/BuildManager.cs
Expand Up @@ -1702,6 +1702,11 @@ private void ExecuteGraphBuildScheduler(GraphBuildSubmission submission)
projectLoadSettings |= ProjectLoadSettings.IgnoreMissingImports | ProjectLoadSettings.IgnoreInvalidImports | ProjectLoadSettings.IgnoreEmptyImports;
}

if (submission.BuildRequestData.Flags.HasFlag(BuildRequestDataFlags.FailOnUnresolvedSdk))
{
projectLoadSettings |= ProjectLoadSettings.FailOnUnresolvedSdk;
}

return new ProjectInstance(
path,
properties,
Expand Down
15 changes: 15 additions & 0 deletions src/Build/BackEnd/BuildManager/BuildRequestData.cs
Expand Up @@ -75,6 +75,21 @@ public enum BuildRequestDataFlags
/// This is especially useful during a restore since some imports might come from packages that haven't been restored yet.
/// </summary>
IgnoreMissingEmptyAndInvalidImports = 1 << 6,

/// <summary>
/// When this flag is present, non top level target(s) in the build request will be skipped if those targets
/// are not defined in the Project to build. The build will still fail if a top lvel target does not exist.
/// This only applies to this build request (if another target calls the "missing target" at any other point
/// this will still result in an error).
/// </summary>
SkipNonexistentNonTopLevelTargets = 1 << 7,

/// <summary>
/// When this flag is present, an unresolved MSBuild project SDK will fail the build. This flag is used to
/// change the <see cref="IgnoreMissingEmptyAndInvalidImports" /> behavior to still fail when an SDK is missing
/// because those are more fatal.
/// </summary>
FailOnUnresolvedSdk = 1 << 8,
}

/// <summary>
Expand Down
10 changes: 8 additions & 2 deletions src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs
Expand Up @@ -143,10 +143,16 @@ public async Task<BuildResult> BuildTargets(ProjectLoggingContext loggingContext
foreach (string targetName in targetNames)
{
var targetExists = _projectInstance.Targets.TryGetValue(targetName, out ProjectTargetInstance targetInstance);
if (!targetExists && entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentTargets))
// Ignore the missing target if:
// SkipNonexistentTargets is set
// -or-
// SkipNonexistentNonTopLevelTargets and the target is is not a top level target
if (!targetExists
&& entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentTargets)
|| entry.Request.BuildRequestDataFlags.HasFlag(BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets) && !entry.Request.Targets.Contains(targetName))
{
_projectLoggingContext.LogComment(Framework.MessageImportance.Low,
"TargetSkippedWhenSkipNonexistentTargets", targetName);
"TargetSkippedWhenSkipNonexistentTargets", targetName);
}
else
{
Expand Down
5 changes: 5 additions & 0 deletions src/Build/BackEnd/Shared/BuildRequestConfiguration.cs
Expand Up @@ -466,6 +466,11 @@ private void SetProjectBasedState(ProjectInstance project)
projectLoadSettings |= ProjectLoadSettings.IgnoreMissingImports | ProjectLoadSettings.IgnoreInvalidImports | ProjectLoadSettings.IgnoreEmptyImports;
}

if (buildRequestDataFlags.HasFlag(buildRequestDataFlags & BuildRequestDataFlags.FailOnUnresolvedSdk))
{
projectLoadSettings |= ProjectLoadSettings.FailOnUnresolvedSdk;
}

return new ProjectInstance(
ProjectFullPath,
globalProperties,
Expand Down
5 changes: 5 additions & 0 deletions src/Build/Definition/ProjectLoadSettings.cs
Expand Up @@ -59,5 +59,10 @@ public enum ProjectLoadSettings
/// Whether to profile the evaluation
/// </summary>
ProfileEvaluation = 128,

/// <summary>
/// Used in combination with <see cref="IgnoreMissingImports" /> to still treat an unresolved MSBuild project SDK as an error.
/// </summary>
FailOnUnresolvedSdk = 256,
}
}
3 changes: 2 additions & 1 deletion src/Build/Evaluation/Evaluator.cs
Expand Up @@ -1761,7 +1761,8 @@ private List<ProjectRootElement> ExpandAndLoadImports(string directoryOfImportin

if (!sdkResult.Success)
{
if (_loadSettings.HasFlag(ProjectLoadSettings.IgnoreMissingImports))
// Ignore the missing import if IgnoreMissingImports is set unless FailOnUnresolvedSdk is also set
if (_loadSettings.HasFlag(ProjectLoadSettings.IgnoreMissingImports) && !_loadSettings.HasFlag(ProjectLoadSettings.FailOnUnresolvedSdk))
{
ProjectImportedEventArgs eventArgs = new ProjectImportedEventArgs(
importElement.Location.Line,
Expand Down
97 changes: 93 additions & 4 deletions src/MSBuild.UnitTests/XMake_Tests.cs
Expand Up @@ -2108,6 +2108,79 @@ public void RestoreIgnoresMissingImports()
logContents.ShouldContain(guid2);
}

/// <summary>
/// When specifying /t:restore, fail when an SDK can't be resolved. Previous behavior was to try and continue anyway but then "restore" would succeed and build workflows continue on.
/// </summary>
[Fact]
public void RestoreFailsOnUnresolvedSdk()
{
string projectContents = ObjectModelHelpers.CleanupFileContents(
$@"<Project>
<Sdk Name=""UnresolvedSdk"" />
<Target Name=""Restore"">
<Message Text=""Restore target ran"" />
</Target>
</Project>");

string logContents = ExecuteMSBuildExeExpectFailure(projectContents, arguments: "/t:restore");

logContents.ShouldContain("error MSB4236: The SDK 'UnresolvedSdk' specified could not be found.");
}

/// <summary>
/// Verifies a non-existent target doesn't fail restore as long as its not considered "top-level" or a target that we're directly executing, in this case Restore.
/// </summary>
[Fact]
public void RestoreSkipsNonExistentNonTopLevelTargets()
{
string restoreFirstProps = $"{Guid.NewGuid():N}.props";

string projectContents = ObjectModelHelpers.CleanupFileContents(
$@"<Project DefaultTargets=""Build"" InitialTargets=""TargetThatComesFromRestore"">
<PropertyGroup>
<RestoreFirstProps>{restoreFirstProps}</RestoreFirstProps>
</PropertyGroup>

<Import Project=""$(RestoreFirstProps)"" />
<Target Name=""Restore"">
<Message Text=""Restore target ran"" />
<ItemGroup>
<Lines Include=""&lt;Project&gt;&lt;Target Name=&quot;TargetThatComesFromRestore&quot;&gt;&lt;Message Text=&quot;Initial target ran&quot; /&gt;&lt;/Target&gt;&lt;/Project&gt;"" />
</ItemGroup>

<WriteLinesToFile File=""$(RestoreFirstProps)"" Lines=""@(Lines)"" Overwrite=""true"" />
</Target>

<Target Name=""Build"">
<Message Text=""Build target ran&quot;"" />
</Target>
</Project>");

string logContents = ExecuteMSBuildExeExpectSuccess(projectContents, arguments: "/restore");

logContents.ShouldContain("Restore target ran");
logContents.ShouldContain("Build target ran");
logContents.ShouldContain("Initial target ran");
}

/// <summary>
/// Verifies restore will fail if the "top-level" target doesn't exist, in this case Restore.
/// </summary>
[Fact]
public void RestoreFailsWhenTopLevelTargetIsNonExistent()
{
string projectContents = ObjectModelHelpers.CleanupFileContents(
@"<Project DefaultTargets=""Build"">
<Target Name=""Build"">
<Message Text=""Build target ran&quot;"" />
</Target>
</Project>");

string logContents = ExecuteMSBuildExeExpectFailure(projectContents, arguments: "/t:restore");

logContents.ShouldContain("error MSB4057: The target \"Restore\" does not exist in the project.");
}

/// <summary>
/// We check if there is only one target name specified and this logic caused a regression: https://github.com/Microsoft/msbuild/issues/3317
/// </summary>
Expand Down Expand Up @@ -2312,6 +2385,24 @@ private string CopyMSBuild()
}

private string ExecuteMSBuildExeExpectSuccess(string projectContents, IDictionary<string, string> filesToCreate = null, IDictionary<string, string> envsToCreate = null, params string[] arguments)
{
(bool result, string output) = ExecuteMSBuildExe(projectContents, filesToCreate, envsToCreate, arguments);

result.ShouldBeTrue(() => output);

return output;
}

private string ExecuteMSBuildExeExpectFailure(string projectContents, IDictionary<string, string> filesToCreate = null, IDictionary<string, string> envsToCreate = null, params string[] arguments)
{
(bool result, string output) = ExecuteMSBuildExe(projectContents, filesToCreate, envsToCreate, arguments);

result.ShouldBeFalse(() => output);

return output;
}

private (bool result, string output) ExecuteMSBuildExe(string projectContents, IDictionary<string, string> filesToCreate = null, IDictionary<string, string> envsToCreate = null, params string[] arguments)
{
using (TestEnvironment testEnvironment = UnitTests.TestEnvironment.Create())
{
Expand All @@ -2336,10 +2427,8 @@ private string ExecuteMSBuildExeExpectSuccess(string projectContents, IDictionar
bool success;

string output = RunnerUtilities.ExecMSBuild($"\"{testProject.ProjectFile}\" {String.Join(" ", arguments)}", out success, _output);

success.ShouldBeTrue(() => output);

return output;

return (success, output);
}
}
}
Expand Down
7 changes: 5 additions & 2 deletions src/MSBuild/XMake.cs
Expand Up @@ -1422,16 +1422,19 @@ private static (BuildResultCode result, Exception exception) ExecuteRestore(stri

// Create a new request with a Restore target only and specify:
// - BuildRequestDataFlags.ClearCachesAfterBuild to ensure the projects will be reloaded from disk for subsequent builds
// - BuildRequestDataFlags.SkipNonexistentTargets to ignore missing targets since Restore does not require that all targets exist
// - BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets to ignore missing non-top-level targets since Restore does not require that all targets
// exist, only top-level ones like Restore itself
// - BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports to ignore imports that don't exist, are empty, or are invalid because restore might
// make available an import that doesn't exist yet and the <Import /> might be missing a condition.
// - BuildRequestDataFlags.FailOnUnresolvedSdk to still fail in the case when an MSBuild project SDK can't be resolved since this is fatal and should
// fail the build.
BuildRequestData restoreRequest = new BuildRequestData(
projectFile,
restoreGlobalProperties,
toolsVersion,
targetsToBuild: new[] { MSBuildConstants.RestoreTargetName },
hostServices: null,
flags: BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports);
flags: BuildRequestDataFlags.ClearCachesAfterBuild | BuildRequestDataFlags.SkipNonexistentNonTopLevelTargets | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports | BuildRequestDataFlags.FailOnUnresolvedSdk);

return ExecuteBuild(buildManager, restoreRequest);
}
Expand Down