Skip to content

Commit

Permalink
Provide nice error when an environment variable is not expanded on th…
Browse files Browse the repository at this point in the history 
…e command line, including the full command line for all switch errors Fixes #7210 (#7213)

In addition to fixing the error below, this captures information about the command line (including parts that came from .rsp files) and logs those along with any command line switch error. For switches from response files, it lists where they came from.

Fixes #7210

Context
When you specify an environment variable on the command line or via Directory.Build.rsp, and that environment variable is not defined, it puts the literal in instead. MSBuild misinterprets that as a duplicate project file and fails. This works around that.

Changes Made
Ignore environment variable "projects" if another project is found. (Otherwise doesn't check.)

Testing
Tried with a simplified repro. Fixed the problem.
  • Loading branch information
@Forgind
Forgind authored Feb 18, 2022
1 parent 334e448 commit ceefec5
Show file tree
Hide file tree
Showing 20 changed files with 552 additions and 111 deletions.
27 changes: 26 additions & 1 deletion src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1196,11 +1196,36 @@ public void ProcessWarnAsMessageSwitchEmpty()
{
CommandLineSwitches commandLineSwitches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>(new[] { "/warnasmessage" }), commandLineSwitches);
// Set "expanded" content to match the placeholder so the verify can use the exact resource string as "expected."
string command = "{0}";
MSBuildApp.GatherCommandLineSwitches(new List<string>(new[] { "/warnasmessage" }), commandLineSwitches, command);

VerifySwitchError(commandLineSwitches, "/warnasmessage", AssemblyResources.GetString("MissingWarnAsMessageParameterError"));
}

/// <summary>
/// Verify that environment variables cannot be passed in as command line switches.
/// Also verifies that the full command line is properly passed when a switch error occurs.
/// </summary>
[Fact]
public void ProcessEnvironmentVariableSwitch()
{
using (TestEnvironment env = TestEnvironment.Create())
{
env.SetEnvironmentVariable("ENVIRONMENTVARIABLE", string.Empty);

CommandLineSwitches commandLineSwitches = new();
string fullCommandLine = "msbuild validProject.csproj %ENVIRONMENTVARIABLE%";
MSBuildApp.GatherCommandLineSwitches(new List<string>() { "validProject.csproj", "%ENVIRONMENTVARIABLE%" }, commandLineSwitches, fullCommandLine);
VerifySwitchError(commandLineSwitches, "%ENVIRONMENTVARIABLE%", String.Format(AssemblyResources.GetString("EnvironmentVariableAsSwitch"), fullCommandLine));

commandLineSwitches = new();
fullCommandLine = "msbuild %ENVIRONMENTVARIABLE% validProject.csproj";
MSBuildApp.GatherCommandLineSwitches(new List<string>() { "%ENVIRONMENTVARIABLE%", "validProject.csproj" }, commandLineSwitches, fullCommandLine);
VerifySwitchError(commandLineSwitches, "%ENVIRONMENTVARIABLE%", String.Format(AssemblyResources.GetString("EnvironmentVariableAsSwitch"), fullCommandLine));
}
}

/// <summary>
/// Verifies that the /warnasmessage switch is parsed properly when codes are specified.
/// </summary>
Expand Down
30 changes: 15 additions & 15 deletions src/MSBuild.UnitTests/ProjectSchemaValidationHandler_Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,12 @@ public class ProjectSchemaValidationHandlerTest
*
**********************************************************************/
[Fact]
public void VerifyInvalidProjectSchema
(
)
public void VerifyInvalidProjectSchema()
{
string[] msbuildTempXsdFilenames = Array.Empty<string>();
string projectFilename = null;
string oldValueForMSBuildOldOM = null;
string oldValueForMSBuildLoadMicrosoftTargetsReadOnly = Environment.GetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly");
try
{
oldValueForMSBuildOldOM = Environment.GetEnvironmentVariable("MSBuildOldOM");
Expand All @@ -60,6 +59,7 @@ public void VerifyInvalidProjectSchema
if (projectFilename != null) File.Delete(projectFilename);
CleanupSchemaFiles(msbuildTempXsdFilenames);
Environment.SetEnvironmentVariable("MSBuildOldOM", oldValueForMSBuildOldOM);
Environment.SetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly", oldValueForMSBuildLoadMicrosoftTargetsReadOnly);
}
}

Expand All @@ -68,13 +68,12 @@ public void VerifyInvalidProjectSchema
/// against is itself invalid
/// </summary>
[Fact]
public void VerifyInvalidSchemaItself1
(
)
public void VerifyInvalidSchemaItself1()
{
string invalidSchemaFile = null;
string projectFilename = null;
string oldValueForMSBuildOldOM = null;
string oldValueForMSBuildLoadMicrosoftTargetsReadOnly = Environment.GetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly");
try
{
oldValueForMSBuildOldOM = Environment.GetEnvironmentVariable("MSBuildOldOM");
Expand All @@ -99,6 +98,7 @@ public void VerifyInvalidSchemaItself1
if (projectFilename != null) File.Delete(projectFilename);
if (invalidSchemaFile != null) File.Delete(invalidSchemaFile);
Environment.SetEnvironmentVariable("MSBuildOldOM", oldValueForMSBuildOldOM);
Environment.SetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly", oldValueForMSBuildLoadMicrosoftTargetsReadOnly);
}
}

Expand All @@ -107,13 +107,12 @@ public void VerifyInvalidSchemaItself1
/// against is itself invalid
/// </summary>
[Fact]
public void VerifyInvalidSchemaItself2
(
)
public void VerifyInvalidSchemaItself2()
{
string invalidSchemaFile = null;
string projectFilename = null;
string oldValueForMSBuildOldOM = null;
string oldValueForMSBuildLoadMicrosoftTargetsReadOnly = Environment.GetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly");

try
{
Expand Down Expand Up @@ -151,6 +150,7 @@ public void VerifyInvalidSchemaItself2
if (invalidSchemaFile != null) File.Delete(invalidSchemaFile);
if (projectFilename != null) File.Delete(projectFilename);
Environment.SetEnvironmentVariable("MSBuildOldOM", oldValueForMSBuildOldOM);
Environment.SetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly", oldValueForMSBuildLoadMicrosoftTargetsReadOnly);
}
}

Expand All @@ -163,10 +163,9 @@ public void VerifyInvalidSchemaItself2
*
**********************************************************************/
[Fact]
public void VerifyValidProjectSchema
(
)
public void VerifyValidProjectSchema()
{
string oldValueForMSBuildLoadMicrosoftTargetsReadOnly = Environment.GetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly");
string[] msbuildTempXsdFilenames = Array.Empty<string>();
string projectFilename = CreateTempFileOnDisk(@"
<Project xmlns=`msbuildnamespace`>
Expand Down Expand Up @@ -198,6 +197,7 @@ public void VerifyValidProjectSchema
File.Delete(projectFilename);
CleanupSchemaFiles(msbuildTempXsdFilenames);
Environment.SetEnvironmentVariable("MSBuildOldOM", oldValueForMSBuildOldOM);
Environment.SetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly", oldValueForMSBuildLoadMicrosoftTargetsReadOnly);
}
}

Expand All @@ -207,10 +207,9 @@ public void VerifyValidProjectSchema
/// should not be caught by the schema
/// </summary>
[Fact]
public void VerifyInvalidImportNotCaughtBySchema
(
)
public void VerifyInvalidImportNotCaughtBySchema()
{
string oldValueForMSBuildLoadMicrosoftTargetsReadOnly = Environment.GetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly");
string[] msbuildTempXsdFilenames = Array.Empty<string>();

string importedProjectFilename = CreateTempFileOnDisk(@"
Expand Down Expand Up @@ -252,6 +251,7 @@ public void VerifyInvalidImportNotCaughtBySchema
File.Delete(projectFilename);
File.Delete(importedProjectFilename);
Environment.SetEnvironmentVariable("MSBuildOldOM", oldValueForMSBuildOldOM);
Environment.SetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly", oldValueForMSBuildLoadMicrosoftTargetsReadOnly);
}
}

Expand Down
27 changes: 25 additions & 2 deletions src/MSBuild.UnitTests/XMake_Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,7 @@ public void Help(string indicator)
[Fact]
public void ErrorCommandLine()
{
string oldValueForMSBuildLoadMicrosoftTargetsReadOnly = Environment.GetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly");
#if FEATURE_GET_COMMANDLINE
MSBuildApp.Execute(@"c:\bin\msbuild.exe -junk").ShouldBe(MSBuildApp.ExitType.SwitchError);

Expand All @@ -538,6 +539,7 @@ public void ErrorCommandLine()

MSBuildApp.Execute(new[] { @"msbuild.exe", "@bogus.rsp" }).ShouldBe(MSBuildApp.ExitType.InitializationError);
#endif
Environment.SetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly", oldValueForMSBuildLoadMicrosoftTargetsReadOnly);
}

[Fact]
Expand Down Expand Up @@ -603,7 +605,6 @@ public void InvalidMaxCPUCountSwitch3()
{
Should.Throw<CommandLineSwitchException>(() =>
{
// Too big
MSBuildApp.ProcessMaxCPUCountSwitch(new[] { "foo" });
}
);
Expand All @@ -614,6 +615,7 @@ public void InvalidMaxCPUCountSwitch4()
{
Should.Throw<CommandLineSwitchException>(() =>
{
// Too big
MSBuildApp.ProcessMaxCPUCountSwitch(new[] { "1025" });
}
);
Expand Down Expand Up @@ -766,7 +768,7 @@ private void RobustDelete(string path)
/// Tests that the environment gets passed on to the node during build.
/// </summary>
[Fact]
public void TestEnvironment()
public void TestEnvironmentTest()
{
string projectString = ObjectModelHelpers.CleanupFileContents(
@"<?xml version=""1.0"" encoding=""utf-8""?>
Expand Down Expand Up @@ -801,6 +803,7 @@ public void TestEnvironment()
[Fact]
public void MSBuildEngineLogger()
{
string oldValueForMSBuildLoadMicrosoftTargetsReadOnly = Environment.GetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly");
string projectString =
"<?xml version=\"1.0\" encoding=\"utf-8\"?>" +
"<Project ToolsVersion=\"4.0\">" +
Expand Down Expand Up @@ -847,6 +850,7 @@ public void MSBuildEngineLogger()
{
File.Delete(projectFileName);
File.Delete(logFile);
Environment.SetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly", oldValueForMSBuildLoadMicrosoftTargetsReadOnly);
}
}

Expand Down Expand Up @@ -944,6 +948,25 @@ public void ResponseFileInProjectDirectoryFoundImplicitly()
output.ShouldContain("[A=1]");
}

[Fact]
public void ResponseFileSwitchesAppearInCommandLine()
{
using (TestEnvironment env = TestEnvironment.Create())
{
TransientTestFolder folder = env.CreateFolder(createFolder: true);
TransientTestFile autoRspFile = env.CreateFile(folder, AutoResponseFileName, "-nowarn:MSB1001 @myRsp.rsp %NONEXISTENTENVIRONMENTVARIABLE%");
TransientTestFile projectFile = env.CreateFile(folder, "project.proj", "<Project><Target Name=\"T\"><Message Text=\"Text\"/></Target></Project>");
TransientTestFile rpsFile = env.CreateFile(folder, "myRsp.rsp", "-nr:false -m:2");
env.SetCurrentDirectory(folder.Path);
string output = RunnerUtilities.ExecMSBuild("project.proj -nologo", out bool success);
success.ShouldBeFalse();
output.ShouldContain("-nr:false -m:2");
output.ShouldContain("-nowarn:MSB1001 @myRsp.rsp %NONEXISTENTENVIRONMENTVARIABLE%");
output.ShouldContain("project.proj -nologo");
output.ShouldContain(": %NONEXISTENTENVIRONMENTVARIABLE%");
}
}

/// <summary>
/// Any msbuild.rsp in the directory of the specified project/solution should be read, and should
/// take priority over any other response files.
Expand Down
42 changes: 27 additions & 15 deletions src/MSBuild/CommandLineSwitches.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Text;
using Microsoft.Build.Framework;
using Microsoft.Build.Shared;

#nullable disable
Expand Down Expand Up @@ -396,6 +398,8 @@ private struct DetectedParameterizedSwitch
private DetectedParameterizedSwitch[] _parameterizedSwitches;
// NOTE: the above arrays are instance members because this class is not required to be a singleton

internal static List<(string path, string contents)> SwitchesFromResponseFiles = new();

/// <summary>
/// Default constructor.
/// </summary>
Expand Down Expand Up @@ -710,18 +714,18 @@ internal bool HaveAnySwitchesBeenSet()
/// Called to flag an error when an unrecognized switch is detected on the command line.
/// </summary>
/// <param name="badCommandLineArg"></param>
internal void SetUnknownSwitchError(string badCommandLineArgValue)
internal void SetUnknownSwitchError(string badCommandLineArgValue, string commandLine = "")
{
SetSwitchError("UnknownSwitchError", badCommandLineArgValue);
SetSwitchError("UnknownSwitchError", badCommandLineArgValue, commandLine);
}

/// <summary>
/// Called to flag an error when a switch that doesn't take parameters is found with parameters on the command line.
/// </summary>
/// <param name="badCommandLineArg"></param>
internal void SetUnexpectedParametersError(string badCommandLineArgValue)
internal void SetUnexpectedParametersError(string badCommandLineArgValue, string commandLine = "")
{
SetSwitchError("UnexpectedParametersError", badCommandLineArgValue);
SetSwitchError("UnexpectedParametersError", badCommandLineArgValue, commandLine);
}

// information about last flagged error
Expand All @@ -730,25 +734,26 @@ internal void SetUnexpectedParametersError(string badCommandLineArgValue)
private string _badCommandLineArg;
private Exception _innerException;
private bool _isParameterError;
private string _commandLine;

/// <summary>
/// Used to flag/store switch errors.
/// </summary>
/// <param name="messageResourceName"></param>
/// <param name="badCommandLineArg"></param>
internal void SetSwitchError(string messageResourceNameValue, string badCommandLineArgValue)
internal void SetSwitchError(string messageResourceNameValue, string badCommandLineArgValue, string commandLine)
{
SetError(messageResourceNameValue, badCommandLineArgValue, null, false);
SetError(messageResourceNameValue, badCommandLineArgValue, null, false, commandLine);
}

/// <summary>
/// Used to flag/store parameter errors.
/// </summary>
/// <param name="messageResourceName"></param>
/// <param name="badCommandLineArg"></param>
internal void SetParameterError(string messageResourceNameValue, string badCommandLineArgValue)
internal void SetParameterError(string messageResourceNameValue, string badCommandLineArgValue, string commandLine)
{
SetParameterError(messageResourceNameValue, badCommandLineArgValue, null);
SetParameterError(messageResourceNameValue, badCommandLineArgValue, null, commandLine);
}

/// <summary>
Expand All @@ -757,9 +762,9 @@ internal void SetParameterError(string messageResourceNameValue, string badComma
/// <param name="messageResourceName"></param>
/// <param name="badCommandLineArg"></param>
/// <param name="innerException"></param>
internal void SetParameterError(string messageResourceNameValue, string badCommandLineArgValue, Exception innerExceptionValue)
internal void SetParameterError(string messageResourceNameValue, string badCommandLineArgValue, Exception innerExceptionValue, string commandLine)
{
SetError(messageResourceNameValue, badCommandLineArgValue, innerExceptionValue, true);
SetError(messageResourceNameValue, badCommandLineArgValue, innerExceptionValue, true, commandLine);
}

/// <summary>
Expand All @@ -769,14 +774,15 @@ internal void SetParameterError(string messageResourceNameValue, string badComma
/// <param name="badCommandLineArg"></param>
/// <param name="innerException"></param>
/// <param name="isParameterError"></param>
private void SetError(string messageResourceNameValue, string badCommandLineArgValue, Exception innerExceptionValue, bool isParameterErrorValue)
private void SetError(string messageResourceNameValue, string badCommandLineArgValue, Exception innerExceptionValue, bool isParameterErrorValue, string commandLine)
{
if (!HaveErrors())
{
_errorMessage = messageResourceNameValue;
_badCommandLineArg = badCommandLineArgValue;
_innerException = innerExceptionValue;
_isParameterError = isParameterErrorValue;
_commandLine = commandLine;
}
}

Expand All @@ -802,7 +808,12 @@ internal void ThrowErrors()
}
else
{
CommandLineSwitchException.Throw(_errorMessage, _badCommandLineArg);
StringBuilder sb = StringBuilderCache.Acquire();
foreach ((string path, string contents) in SwitchesFromResponseFiles)
{
sb.Append($"\n{ResourceUtilities.FormatResourceStringStripCodeAndKeyword("ResponseFileSwitchFromLocation", contents, path)}");
}
CommandLineSwitchException.Throw("SwitchErrorWithArguments", _badCommandLineArg, ResourceUtilities.GetResourceString(_errorMessage), _commandLine, StringBuilderCache.GetStringAndRelease(sb));
}
}
}
Expand All @@ -816,7 +827,7 @@ internal void ThrowErrors()
/// considered to be on the "left", and the switches being appended are on the "right".
/// </remarks>
/// <param name="switchesToAppend"></param>
internal void Append(CommandLineSwitches switchesToAppend)
internal void Append(CommandLineSwitches switchesToAppend, string commandLine = "")
{
// if this collection doesn't already have an error registered, but the collection being appended does
if (!HaveErrors() && switchesToAppend.HaveErrors())
Expand All @@ -829,6 +840,7 @@ internal void Append(CommandLineSwitches switchesToAppend)
_badCommandLineArg = switchesToAppend._badCommandLineArg;
_innerException = switchesToAppend._innerException;
_isParameterError = switchesToAppend._isParameterError;
_commandLine = commandLine;
}

// NOTE: we might run into some duplicate switch errors below, but if we've already registered the error from the
Expand All @@ -849,7 +861,7 @@ internal void Append(CommandLineSwitches switchesToAppend)
else
{
SetSwitchError(s_parameterlessSwitchesMap[i].duplicateSwitchErrorMessage,
switchesToAppend.GetParameterlessSwitchCommandLineArg((ParameterlessSwitch)i));
switchesToAppend.GetParameterlessSwitchCommandLineArg((ParameterlessSwitch)i), commandLine);
}
}
}
Expand All @@ -873,7 +885,7 @@ internal void Append(CommandLineSwitches switchesToAppend)
else
{
SetSwitchError(s_parameterizedSwitchesMap[j].duplicateSwitchErrorMessage,
switchesToAppend.GetParameterizedSwitchCommandLineArg((ParameterizedSwitch)j));
switchesToAppend.GetParameterizedSwitchCommandLineArg((ParameterizedSwitch)j), commandLine);
}
}
}
Expand Down
Loading

0 comments on commit ceefec5

Please sign in to comment.