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 a better error message when TargetFramework property has a semicolon in it #1274

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/Tasks/Microsoft.NET.Build.Tasks/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@
<value>Unexpected file type for '{0}'. Type is both '{1}' and '{2}'.</value>
</data>
<data name="CannotInferTargetFrameworkIdentiferAndVersion" xml:space="preserve">
<value>Cannot infer TargetFrameworkIdentifier and/or TargetFrameworkVersion from TargetFramework='{0}'. They must be specified explicitly.</value>
<value>The TargetFramework value '{0}' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be specified explicitly.</value>
</data>
<data name="ContentItemDoesNotProvideOutputPath" xml:space="preserve">
<value>Content item for '{0}' sets '{1}', but does not provide '{2}' or '{3}'.</value>
Expand Down Expand Up @@ -297,6 +297,9 @@
<data name="UnsupportedTargetFrameworkVersion" xml:space="preserve">
<value>The current .NET SDK does not support targeting {0} {1}. Either target {0} {2} or lower, or use a version of the .NET SDK that supports {0} {1}.</value>
</data>
<data name="TargetFrameworkWithSemicolon" xml:space="preserve">
<value>The TargetFramework value '{0}' is not valid. To multi-target, use the 'TargetFrameworks' property instead.</value>
</data>
<data name="AssetsFileMissingRuntimeIdentifier" xml:space="preserve">
<value>Assets file '{0}' doesn't have a target for '{1}'. Ensure you have included '{2}' in the TargetFrameworks for your project. You may also need to include '{3}' in your project's RuntimeIdentifiers.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,17 @@ Copyright (c) .NET Foundation. All rights reserved.
<PropertyGroup Condition="'$(TargetFrameworkIdentifier)' == '' or '$(TargetFrameworkVersion)' == ''">
<_UnsupportedTargetFrameworkError>true</_UnsupportedTargetFrameworkError>
</PropertyGroup>
<Target Name="_CheckForUnsupportedTargetFramework" BeforeTargets="_CheckForInvalidConfigurationAndPlatform">
<NETSdkError Condition="'$(_UnsupportedTargetFrameworkError)' == 'true'"
<Target Name="_CheckForUnsupportedTargetFramework"
BeforeTargets="_CheckForInvalidConfigurationAndPlatform;_GenerateRestoreProjectSpec;CollectPackageReferences"
Condition="'$(_UnsupportedTargetFrameworkError)' == 'true'"
>
<NETSdkError Condition="!$(TargetFramework.Contains(';'))"
ResourceName="CannotInferTargetFrameworkIdentiferAndVersion"
FormatArguments="$([MSBuild]::Escape('$(TargetFramework)'))" />

<NETSdkError Condition="$(TargetFramework.Contains(';'))"
ResourceName="TargetFrameworkWithSemicolon"
FormatArguments="$([MSBuild]::Escape('$(TargetFramework)'))" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see tests for both cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added both tests

</Target>

<!--
Expand Down
118 changes: 96 additions & 22 deletions test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildALibrary.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public void It_builds_the_library_successfully()
});
}

[Fact]
[Fact]
public void It_builds_the_library_twice_in_a_row()
{
var testAsset = _testAssetsManager
Expand Down Expand Up @@ -533,31 +533,105 @@ public void It_implicitly_defines_compilation_constants_for_the_target_framework
definedConstants.Should().BeEquivalentTo(new[] { "DEBUG", "TRACE" }.Concat(expectedDefines).ToArray());
}

[Fact]
public void It_fails_gracefully_if_targetframework_is_empty()
[Theory]
[InlineData(false)]
[InlineData(true)]
public void It_fails_gracefully_if_targetframework_is_empty(bool useSolution)
{
var testAsset = _testAssetsManager
.CopyTestAsset("AppWithLibrary", "EmptyTargetFramework")
.WithSource()
.WithProjectChanges(project =>
string targetFramework = "";
TestInvalidTargetFramework("EmptyTargetFramework", targetFramework, useSolution,
$"The TargetFramework value '{targetFramework}' was not recognized");
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public void It_fails_gracefully_if_targetframework_is_invalid(bool useSolution)
{
string targetFramework = "notaframework";
TestInvalidTargetFramework("InvalidTargetFramework", targetFramework, useSolution,
$"The TargetFramework value '{targetFramework}' was not recognized");
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public void It_fails_gracefully_if_targetframework_should_be_targetframeworks(bool useSolution)
{
string targetFramework = "netcoreapp2.0;net461";
TestInvalidTargetFramework("InvalidTargetFramework", targetFramework, useSolution,
$"The TargetFramework value '{targetFramework}' is not valid. To multi-target, use the 'TargetFrameworks' property instead");
}

private void TestInvalidTargetFramework(string testName, string targetFramework, bool useSolution, string expectedOutput)
{
var testProject = new TestProject()
{
Name = testName,
TargetFrameworks = targetFramework,
IsSdkProject = true
};

string identifier = useSolution ? "_Solution" : "";
var testAsset = _testAssetsManager.CreateTestProject(testProject, testProject.Name, identifier);

if (targetFramework.Contains(";"))
{
// The TestProject class doesn't differentiate between TargetFramework and TargetFrameworks, and helpfully selects
// which property to use based on whether there's a semicolon.
// For this test, we need to override this behavior
testAsset = testAsset.WithProjectChanges(project =>
{
project.Root
.Elements("PropertyGroup")
.Elements("TargetFramework")
.Single()
.SetValue("");
})
.Restore(Log, relativePath: "TestLibrary");
var ns = project.Root.Name.Namespace;

var libraryProjectDirectory = Path.Combine(testAsset.TestRoot, "TestLibrary");
var buildCommand = new BuildCommand(Log, libraryProjectDirectory);
project.Root.Element(ns + "PropertyGroup")
.Element(ns + "TargetFrameworks")
.Name = ns + "TargetFramework";
});
}

buildCommand
.Execute()
.Should()
.Fail()
.And.HaveStdOutContaining("TargetFramework=''") // new deliberate error
.And.NotHaveStdOutContaining(">="); // old error about comparing empty string to version
TestCommand restoreCommand;
TestCommand buildCommand;

if (useSolution)
{
var dotnetCommand = new DotnetCommand(Log)
{
WorkingDirectory = testAsset.TestRoot
};

dotnetCommand.Execute("new", "sln")
.Should()
.Pass();

var relativePathToProject = Path.Combine(testProject.Name, testProject.Name + ".csproj");
dotnetCommand.Execute($"sln", "add", relativePathToProject)
.Should()
.Pass();

var relativePathToSln = testProject.Name + identifier + ".sln";

restoreCommand = testAsset.GetRestoreCommand(Log, relativePathToSln);
buildCommand = new BuildCommand(Log, testAsset.TestRoot, relativePathToSln);
}
else
{
restoreCommand = testAsset.GetRestoreCommand(Log, testProject.Name);
buildCommand = new BuildCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
}

foreach (var command in new TestCommand[] { restoreCommand, buildCommand })
{
// Set RestoreContinueOnError=ErrorAndContinue to force failure on error
// See https://github.com/NuGet/Home/issues/5309
command
.Execute("/p:RestoreContinueOnError=ErrorAndContinue")
.Should()
.Fail()
.And
.HaveStdOutContaining(expectedOutput)
.And.NotHaveStdOutContaining(">="); // old error about comparing empty string to version when TargetFramework was blank;
}
}

[Theory]
Expand Down
31 changes: 31 additions & 0 deletions test/Microsoft.NET.TestFramework/Commands/DotnetCommand.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
using System.Text;
using Microsoft.DotNet.Cli.Utils;
using Xunit.Abstractions;

namespace Microsoft.NET.TestFramework.Commands
{
public class DotnetCommand : TestCommand
{
public string WorkingDirectory { get; set; }

public DotnetCommand(ITestOutputHelper log) : base(log)
{
}

protected override ICommand CreateCommand(string[] args)
{
ICommand ret = Command.Create(RepoInfo.DotNetHostPath, args);
if (!string.IsNullOrEmpty(WorkingDirectory))
{
ret = ret.WorkingDirectory(WorkingDirectory);
}

return ret;
}
}
}
4 changes: 2 additions & 2 deletions test/Microsoft.NET.TestFramework/TestAsset.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public void SetSdkVersion(XDocument project)

}

public RestoreCommand GetRestoreCommand(ITestOutputHelper log, string relativePath = "", params string[] args)
public RestoreCommand GetRestoreCommand(ITestOutputHelper log, string relativePath = "")
{
return new RestoreCommand(log, System.IO.Path.Combine(TestRoot, relativePath))
.AddSourcesFromCurrentConfig()
Expand All @@ -146,7 +146,7 @@ public RestoreCommand GetRestoreCommand(ITestOutputHelper log, string relativePa

public TestAsset Restore(ITestOutputHelper log, string relativePath = "", params string[] args)
{
var commandResult = GetRestoreCommand(log, relativePath, args)
var commandResult = GetRestoreCommand(log, relativePath)
.Execute(args);

commandResult.Should().Pass();
Expand Down