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.

Sign up for GitHub

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

Jump to bottom

Provide nice error when an environment variable is not expanded on the command line, including the full command line for all switch errors Fixes #7210 #7213

Merged
merged 28 commits into from
Feb 18, 2022
Merged

Provide nice error when an environment variable is not expanded on the command line, including the full command line for all switch errors Fixes #7210 #7213

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
66ae06a
Ignore env variable switches
Forgind Jan 4, 2022
df38393
Log command line on switch error
Forgind Jan 5, 2022
ab4b6ba
Add hack to string
Forgind Jan 5, 2022
4bf3613
Always point to the environment variable
Forgind Jan 5, 2022
11a2359
Create new error
Forgind Jan 5, 2022
062b313
Move check earlier
Forgind Jan 5, 2022
41427e4
Add test for new behavior
Forgind Jan 5, 2022
76afef4
Delete unused method
Forgind Jan 5, 2022
44a87c9
Include rsps in command line
Forgind Jan 6, 2022
4d9d090
Use TestEnvironment
Forgind Jan 6, 2022
ba4283c
Include response file paths in command line
Forgind Jan 7, 2022
1e9d5c7
Fix resource string
Forgind Jan 7, 2022
6174c75
Build
Forgind Jan 7, 2022
ad31551
Adjust format
Forgind Jan 10, 2022
4753c7d
Merge branch 'main' into ignore-environment-variable-switches
Forgind Jan 10, 2022
af1f39c
Make it easier on the loc team
Forgind Jan 14, 2022
596a7c1
Even easier
Forgind Jan 14, 2022
5af7815
Properly make easier
Forgind Jan 14, 2022
55e9955
Release properly
Forgind Jan 15, 2022
7fcf177
Use ' ' and make tests pass
Forgind Jan 20, 2022
e624a5e
Merge branch 'main' into ignore-environment-variable-switches
Forgind Jan 31, 2022
5113a67
PR comments + added test
Forgind Feb 8, 2022
f50182a
Little TestEnvironment cleanup
Forgind Feb 8, 2022
50894f0
GC SwitchesFromResponseFile
Forgind Feb 8, 2022
4244db2
Ensure initialized
Forgind Feb 8, 2022
1c6c2c3
Keep switches in order
Forgind Feb 14, 2022
af3d14b
Merge branch 'main' into ignore-environment-variable-switches
Forgind Feb 16, 2022
0d7e80f
Add commandLine arg
Forgind Feb 16, 2022
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
80 changes: 53 additions & 27 deletions src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ public void TargetsSwitchIdentificationTests(string @switch)
public void TargetsSwitchParameter()
{
CommandLineSwitches switches = new CommandLineSwitches();
MSBuildApp.GatherCommandLineSwitches(new List<string>() { "/targets:targets.txt" }, switches);
MSBuildApp.GatherCommandLineSwitches(new List<string>() { "/targets:targets.txt" }, switches, string.Empty);

switches.HaveErrors().ShouldBeFalse();
switches[CommandLineSwitches.ParameterizedSwitch.Targets].ShouldBe(new[] { "targets.txt" });
Expand All @@ -468,7 +468,7 @@ public void TargetsSwitchParameter()
public void TargetsSwitchDoesNotSupportMultipleOccurrences()
{
CommandLineSwitches switches = new CommandLineSwitches();
MSBuildApp.GatherCommandLineSwitches(new List<string>() { "/targets /targets" }, switches);
MSBuildApp.GatherCommandLineSwitches(new List<string>() { "/targets /targets" }, switches, string.Empty);

switches.HaveErrors().ShouldBeTrue();
}
Expand Down Expand Up @@ -546,7 +546,7 @@ public void GraphBuildSwitchCanHaveParameters()
{
CommandLineSwitches switches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>{ "/graph", "/graph:true; NoBuild ;; ;", "/graph:foo"}, switches);
MSBuildApp.GatherCommandLineSwitches(new List<string>{ "/graph", "/graph:true; NoBuild ;; ;", "/graph:foo"}, switches, string.Empty);

switches[CommandLineSwitches.ParameterizedSwitch.GraphBuild].ShouldBe(new[] {"true", " NoBuild ", " ", "foo"});

Expand All @@ -558,7 +558,7 @@ public void GraphBuildSwitchCanBeParameterless()
{
CommandLineSwitches switches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>{ "/graph" }, switches);
MSBuildApp.GatherCommandLineSwitches(new List<string>{ "/graph" }, switches, string.Empty);

switches[CommandLineSwitches.ParameterizedSwitch.GraphBuild].ShouldBe(new string[0]);

Expand All @@ -570,7 +570,7 @@ public void InputResultsCachesSupportsMultipleOccurrence()
{
CommandLineSwitches switches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>(){"/irc", "/irc:a;b", "/irc:c;d"}, switches);
MSBuildApp.GatherCommandLineSwitches(new List<string>(){"/irc", "/irc:a;b", "/irc:c;d"}, switches, string.Empty);

switches[CommandLineSwitches.ParameterizedSwitch.InputResultsCaches].ShouldBe(new []{null, "a", "b", "c", "d"});

Expand All @@ -582,7 +582,7 @@ public void OutputResultsCache()
{
CommandLineSwitches switches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>(){"/orc:a"}, switches);
MSBuildApp.GatherCommandLineSwitches(new List<string>(){"/orc:a"}, switches, string.Empty);

switches[CommandLineSwitches.ParameterizedSwitch.OutputResultsCache].ShouldBe(new []{"a"});

Expand All @@ -594,7 +594,7 @@ public void OutputResultsCachesDoesNotSupportMultipleOccurrences()
{
CommandLineSwitches switches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>(){"/orc:a", "/orc:b"}, switches);
MSBuildApp.GatherCommandLineSwitches(new List<string>(){"/orc:a", "/orc:b"}, switches, string.Empty);

switches.HaveErrors().ShouldBeTrue();
}
Expand Down Expand Up @@ -768,24 +768,24 @@ public void AppendErrorTests1()
Assert.False(switchesLeft.HaveErrors());
Assert.False(switchesRight.HaveErrors());

switchesLeft.Append(switchesRight);
switchesLeft.Append(switchesRight, string.Empty);

Assert.False(switchesLeft.HaveErrors());
Assert.False(switchesRight.HaveErrors());

switchesLeft.SetUnknownSwitchError("/bogus");
switchesLeft.SetUnknownSwitchError("/bogus", string.Empty);

Assert.True(switchesLeft.HaveErrors());
Assert.False(switchesRight.HaveErrors());

switchesLeft.Append(switchesRight);
switchesLeft.Append(switchesRight, string.Empty);

Assert.True(switchesLeft.HaveErrors());
Assert.False(switchesRight.HaveErrors());

VerifySwitchError(switchesLeft, "/bogus");

switchesRight.Append(switchesLeft);
switchesRight.Append(switchesLeft, string.Empty);

Assert.True(switchesLeft.HaveErrors());
Assert.True(switchesRight.HaveErrors());
Expand All @@ -803,16 +803,16 @@ public void AppendErrorTests2()
Assert.False(switchesLeft.HaveErrors());
Assert.False(switchesRight.HaveErrors());

switchesLeft.SetUnknownSwitchError("/bogus");
switchesRight.SetUnexpectedParametersError("/nologo:foo");
switchesLeft.SetUnknownSwitchError("/bogus", string.Empty);
switchesRight.SetUnexpectedParametersError("/nologo:foo", string.Empty);

Assert.True(switchesLeft.HaveErrors());
Assert.True(switchesRight.HaveErrors());

VerifySwitchError(switchesLeft, "/bogus");
VerifySwitchError(switchesRight, "/nologo:foo");

switchesLeft.Append(switchesRight);
switchesLeft.Append(switchesRight, string.Empty);

VerifySwitchError(switchesLeft, "/bogus");
VerifySwitchError(switchesRight, "/nologo:foo");
Expand All @@ -835,7 +835,7 @@ public void AppendParameterlessSwitchesTests()
Assert.False(switchesRight1.IsParameterlessSwitchSet(CommandLineSwitches.ParameterlessSwitch.Help));
Assert.True(switchesRight1.IsParameterlessSwitchSet(CommandLineSwitches.ParameterlessSwitch.NoConsoleLogger));

switchesLeft.Append(switchesRight1);
switchesLeft.Append(switchesRight1, string.Empty);

Assert.Equal("/noconlog", switchesLeft.GetParameterlessSwitchCommandLineArg(CommandLineSwitches.ParameterlessSwitch.NoConsoleLogger));
Assert.True(switchesLeft.IsParameterlessSwitchSet(CommandLineSwitches.ParameterlessSwitch.NoConsoleLogger));
Expand All @@ -853,7 +853,7 @@ public void AppendParameterlessSwitchesTests()
Assert.False(switchesRight2.IsParameterlessSwitchSet(CommandLineSwitches.ParameterlessSwitch.Help));
Assert.True(switchesRight2.IsParameterlessSwitchSet(CommandLineSwitches.ParameterlessSwitch.NoConsoleLogger));

switchesLeft.Append(switchesRight2);
switchesLeft.Append(switchesRight2, string.Empty);

Assert.Equal("/NOCONSOLELOGGER", switchesLeft.GetParameterlessSwitchCommandLineArg(CommandLineSwitches.ParameterlessSwitch.NoConsoleLogger));
Assert.True(switchesLeft.IsParameterlessSwitchSet(CommandLineSwitches.ParameterlessSwitch.NoConsoleLogger));
Expand Down Expand Up @@ -883,7 +883,7 @@ public void AppendParameterizedSwitchesTests1()
Assert.False(switchesRight.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Project));
Assert.True(switchesRight.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Target));

switchesLeft.Append(switchesRight);
switchesLeft.Append(switchesRight, string.Empty);

Assert.Equal("tempproject.proj", switchesLeft.GetParameterizedSwitchCommandLineArg(CommandLineSwitches.ParameterizedSwitch.Project));
Assert.True(switchesLeft.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Project));
Expand Down Expand Up @@ -919,7 +919,7 @@ public void AppendParameterizedSwitchesTests2()

Assert.True(switchesRight.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Target));

switchesLeft.Append(switchesRight);
switchesLeft.Append(switchesRight, string.Empty);

Assert.Equal("/t:\"RESOURCES\";build", switchesLeft.GetParameterizedSwitchCommandLineArg(CommandLineSwitches.ParameterizedSwitch.Target));
Assert.True(switchesLeft.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Target));
Expand Down Expand Up @@ -948,7 +948,7 @@ public void AppendParameterizedSwitchesTests3()

Assert.True(switchesRight.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Project));

switchesLeft.Append(switchesRight);
switchesLeft.Append(switchesRight, string.Empty);

Assert.Equal("tempproject.proj", switchesLeft.GetParameterizedSwitchCommandLineArg(CommandLineSwitches.ParameterizedSwitch.Project));
Assert.True(switchesLeft.IsParameterizedSwitchSet(CommandLineSwitches.ParameterizedSwitch.Project));
Expand Down Expand Up @@ -1074,7 +1074,7 @@ public void ProcessWarnAsErrorSwitchNotSpecified()
{
CommandLineSwitches commandLineSwitches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>(new[] { "" }), commandLineSwitches);
MSBuildApp.GatherCommandLineSwitches(new List<string>(new[] { "" }), commandLineSwitches, string.Empty);

Assert.Null(MSBuildApp.ProcessWarnAsErrorSwitch(commandLineSwitches));
}
Expand All @@ -1097,7 +1097,7 @@ public void ProcessWarnAsErrorSwitchWithCodes()
"/err:D,d;E,e", // A different source with new items and uses the short form
"/warnaserror:a", // A different source with a single duplicate
"/warnaserror:a,b", // A different source with multiple duplicates
}), commandLineSwitches);
}), commandLineSwitches, string.Empty);

ISet<string> actualWarningsAsErrors = MSBuildApp.ProcessWarnAsErrorSwitch(commandLineSwitches);

Expand All @@ -1118,7 +1118,7 @@ public void ProcessWarnAsErrorSwitchEmptySwitchClearsSet()
{
"/warnaserror:a;b;c",
"/warnaserror",
}), commandLineSwitches);
}), commandLineSwitches, string.Empty);

ISet<string> actualWarningsAsErrors = MSBuildApp.ProcessWarnAsErrorSwitch(commandLineSwitches);

Expand All @@ -1142,7 +1142,7 @@ public void ProcessWarnAsErrorSwitchValuesAfterEmptyAddOn()
"/warnaserror:a;b;c",
"/warnaserror",
"/warnaserror:e;f;g",
}), commandLineSwitches);
}), commandLineSwitches, string.Empty);

ISet<string> actualWarningsAsErrors = MSBuildApp.ProcessWarnAsErrorSwitch(commandLineSwitches);

Expand All @@ -1159,7 +1159,7 @@ public void ProcessWarnAsErrorSwitchEmpty()
{
CommandLineSwitches commandLineSwitches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>(new [] { "/warnaserror" }), commandLineSwitches);
MSBuildApp.GatherCommandLineSwitches(new List<string>(new [] { "/warnaserror" }), commandLineSwitches, string.Empty);

ISet<string> actualWarningsAsErrors = MSBuildApp.ProcessWarnAsErrorSwitch(commandLineSwitches);

Expand All @@ -1176,11 +1176,37 @@ public void ProcessWarnAsMessageSwitchEmpty()
{
CommandLineSwitches commandLineSwitches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>(new[] { "/warnasmessage" }), commandLineSwitches);
MSBuildApp.GatherCommandLineSwitches(new List<string>(new[] { "/warnasmessage" }), commandLineSwitches, /*This is a hack so the error message contains the exact resource string.*/ "{0}");

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()
{
string savedEnvironmentVariable = Environment.GetEnvironmentVariable("ENVIRONMENTVARIABLE");
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can this use the TestEnvironment class? Similar to this:

msbuild/src/Build.OM.UnitTests/Definition/Project_Tests.cs

Lines 4104 to 4106 in 797fd82

using (var env = TestEnvironment.Create(_output))
{
env.SetEnvironmentVariable("MSBUILDLOGIMPORTS", "1");

Environment.SetEnvironmentVariable("ENVIRONMENTVARIABLE", null);

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));

if (savedEnvironmentVariable is not null)
{
Environment.SetEnvironmentVariable("ENVIRONMENTVARIABLE", savedEnvironmentVariable);
}
}

/// <summary>
/// Verifies that the /warnasmessage switch is parsed properly when codes are specified.
/// </summary>
Expand All @@ -1199,7 +1225,7 @@ public void ProcessWarnAsMessageSwitchWithCodes()
"/nowarn:D,d;E,e", // A different source with new items and uses the short form
"/warnasmessage:a", // A different source with a single duplicate
"/warnasmessage:a,b", // A different source with multiple duplicates
}), commandLineSwitches);
}), commandLineSwitches, string.Empty);

ISet<string> actualWarningsAsMessages = MSBuildApp.ProcessWarnAsMessageSwitch(commandLineSwitches);

Expand All @@ -1216,7 +1242,7 @@ public void ProcessProfileEvaluationEmpty()
{
CommandLineSwitches commandLineSwitches = new CommandLineSwitches();

MSBuildApp.GatherCommandLineSwitches(new List<string>(new[] { "/profileevaluation" }), commandLineSwitches);
MSBuildApp.GatherCommandLineSwitches(new List<string>(new[] { "/profileevaluation" }), commandLineSwitches, string.Empty);
commandLineSwitches[CommandLineSwitches.ParameterizedSwitch.ProfileEvaluation][0].ShouldBe("no-file");
}

Expand Down
10 changes: 5 additions & 5 deletions src/MSBuild.UnitTests/XMake_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public void GatherCommandLineSwitchesTwoProperties()
var arguments = new List<string>();
rainersigwald marked this conversation as resolved.
Show resolved Hide resolved
arguments.AddRange(new[] { "/p:a=b", "/p:c=d" });

MSBuildApp.GatherCommandLineSwitches(arguments, switches);
MSBuildApp.GatherCommandLineSwitches(arguments, switches, string.Empty);

string[] parameters = switches[CommandLineSwitches.ParameterizedSwitch.Property];
parameters[0].ShouldBe("a=b");
Expand All @@ -67,7 +67,7 @@ public void GatherCommandLineSwitchesAnyDash()
"--p:maxcpucount=8"
};

MSBuildApp.GatherCommandLineSwitches(arguments, switches);
MSBuildApp.GatherCommandLineSwitches(arguments, switches, string.Empty);

string[] parameters = switches[CommandLineSwitches.ParameterizedSwitch.Property];
parameters[0].ShouldBe("a=b");
Expand All @@ -82,7 +82,7 @@ public void GatherCommandLineSwitchesMaxCpuCountWithArgument()
var arguments = new List<string>();
arguments.AddRange(new[] { "/m:2" });

MSBuildApp.GatherCommandLineSwitches(arguments, switches);
MSBuildApp.GatherCommandLineSwitches(arguments, switches, string.Empty);

string[] parameters = switches[CommandLineSwitches.ParameterizedSwitch.MaxCPUCount];
parameters[0].ShouldBe("2");
Expand All @@ -99,7 +99,7 @@ public void GatherCommandLineSwitchesMaxCpuCountWithoutArgument()
var arguments = new List<string>();
arguments.AddRange(new[] { "/m:3", "/m" });

MSBuildApp.GatherCommandLineSwitches(arguments, switches);
MSBuildApp.GatherCommandLineSwitches(arguments, switches, string.Empty);

string[] parameters = switches[CommandLineSwitches.ParameterizedSwitch.MaxCPUCount];
parameters[1].ShouldBe(Convert.ToString(NativeMethodsShared.GetLogicalCoreCount()));
Expand All @@ -119,7 +119,7 @@ public void GatherCommandLineSwitchesMaxCpuCountWithoutArgumentButWithColon()
var arguments = new List<string>();
arguments.AddRange(new[] { "/m:" });

MSBuildApp.GatherCommandLineSwitches(arguments, switches);
MSBuildApp.GatherCommandLineSwitches(arguments, switches, string.Empty);

string[] parameters = switches[CommandLineSwitches.ParameterizedSwitch.MaxCPUCount];
parameters.Length.ShouldBe(0);
Expand Down
Loading