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

Forgind · 2022-01-04T01:21:29Z

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.

Notes

Maybe consider giving a better error message instead of ignoring the environment variable?

Forgind · 2022-01-04T01:21:58Z

@CarlHeinrichs, what do you think?

CarlHeinrichs

LGTM, but I am not that familiar with this code. I assume you are going to author some tests before completing the PR?

ladipro · 2022-01-04T09:10:13Z

I am generally opposed to addressing the linked issue by overriding the default (and OS specific) command line expansion rules. If the shell passes %nonexistentvar% to all programs by design, there's IMO no good reason to have MSBuild try to detect and ignore it. It's not systematic and not worth the added complexity. I'm afraid that detecting and expanding environment variables in our code is asking for trouble. IsEnvironmentVariable, for example, should be platform specific, e.g. $somevar doesn't work on Windows.

Forgind · 2022-01-04T14:57:58Z

I am generally opposed to addressing the linked issue by overriding the default (and OS specific) command line expansion rules. If the shell passes %nonexistentvar% to all programs by design, there's IMO no good reason to have MSBuild try to detect and ignore it. It's not systematic and not worth the added complexity. I'm afraid that detecting and expanding environment variables in our code is asking for trouble. IsEnvironmentVariable, for example, should be platform specific, e.g. $somevar doesn't work on Windows.

I should definitely make IsEnvironmentVariable platform-specific. I can't think of a reason you'd want an unexpanded environment variable as part of your command line, though. I'm not exactly expanding it but ignoring it if and only if there's an alternate "project" candidate. The other option I was considering is detecting something like that and giving a custom error about "why do you have an unexpanded environment variable in your command line?" Since current behavior is throwing a confusing error, that would be just making the error clearer rather than changing behavior.

I could also put the current change under a changewave if you'd like or have it additionally throw a warning such that it doesn't silently ignore an environment variable. Open to any of these.

ladipro · 2022-01-04T21:11:07Z

My point is that many tools could use a similar logic (hey, this argument looks like an unexpanded env var and the command line otherwise makes sense so let's ignore it) but they don't. We shouldn't do it just because in our case it happens to be easy.

I would vote for adding a good command line logging instead. For example, I don't think you will find the full command line (as combined from response files) in the binlog today. When MSBuild is complaining about a command line argument or switch, it should always print the full command line together with the error so the user can tell what went wrong. I believe that it would have been enough for the linked issue to be a non-issue.

Forgind · 2022-01-05T00:21:16Z

I like the idea of adding the full command line to switch-related error messages, so I added that. I'm still tempted to, instead of silently ignoring unexpanded env vars, make a new error that says "hey, your env var didn't expand, see?" instead. How would you feel about that?

(And also make the env var style os-specific.)

rainersigwald

And also make the env var style os-specific.

Environment variables are not OS-specific; they are shell-specific. In addition, MSBuild's expansion of environment variables from response files uses Environment.ExpandEnvironmentVariables(string), which only considers %-delimited variables.

I do not think we should silently ignore unexpanded variables. I agree that we should log the full, expanded command line when giving a command-line-parsing error. I would not object to having a suberror if there is an unexpanded %variable_reference% to give a hint.

It otherwise only caught an environment variable if it was the first "project" switch but not if it was the duplicate.

ladipro

I took a very quick look and the command line now printed as part of error messages doesn't seem to include parts that came from response files. Example:

If Directory.Build.rsp contains %nonexistent% and MSBuild is invoked like so:
msbuild myproject.csproj

It prints:
MSBUILD : error MSB1060: Undefined environment variable passed in as switch. Full path: '...\msbuild.exe myproject.csproj'

I think that printing switches as combined from all sources (optionally also with provenance annotation, i.e. which switches came from where) would be a powerful diagnostic feature.

ladipro · 2022-01-06T13:16:07Z

src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs

+        [Fact]
+        public void ProcessEnvironmentVariableSwitch()
+        {
+            string savedEnvironmentVariable = Environment.GetEnvironmentVariable("ENVIRONMENTVARIABLE");


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

Forgind · 2022-01-06T20:34:14Z

@ladipro, I consider my last commit the "minimum" version, but there are a couple caveats. Most notably, we have to parse the command line not include the .rsp before parsing the .rsp because there can be switches that tell you to parse a particular .rsp, and there can be a switch to say not to parse the automatically found one. That means that if there's an error earlier in the command line parsing process, there's no way it will know about switches from .rsps, so the command line will necessarily be incomplete.

That aside, this should give you the full command line (not including extra switches from .rsps) plus any switches from .rsps we'd parsed. It does not specify where switches come from, but I can do something a little hacky to make that pretty clean. Currently, I add .rsp switches via:
commandLine += ' ' + string.Join(" ", argsFromResponseFile);

What would you think of making that:
commandLine += $"' '{responseFile}': '{string.Join(" ", argsFromResponseFile)}";

The advantage of something like that is that I can specify the exact path of every response file, and it's quick and easy. That doesn't include any words because they wouldn't be localized. Alternatively, I can add another string to localize like:
' Switches added from response file '{0}': '{1} and fill in 0 and 1 appropriately on this line.

ladipro · 2022-01-07T10:45:12Z

@Forgind thank you!

What would you think of making that:
commandLine += $"' '{responseFile}': '{string.Join(" ", argsFromResponseFile)}";

This would be awesome. I don't have a strong opinion on the format of the output. Presumably the above would be clear enough.

Code-wise, I'm wondering if passing the command line string to CommandLineSwitches at construction time instead of having it as an argument of all Set*Error methods wouldn't be cleaner. With the change to print responseFile: argsFromResponseFile there should be no need to pass it by ref and mutate it, if I'm understanding the logic correctly.

Forgind · 2022-01-07T20:12:34Z

@Forgind thank you!

What would you think of making that:
commandLine += $"' '{responseFile}': '{string.Join(" ", argsFromResponseFile)}";

This would be awesome. I don't have a strong opinion on the format of the output. Presumably the above would be clear enough.

Great! Will do.

Code-wise, I'm wondering if passing the command line string to CommandLineSwitches at construction time instead of having it as an argument of all Set*Error methods wouldn't be cleaner. With the change to print responseFile: argsFromResponseFile there should be no need to pass it by ref and mutate it, if I'm understanding the logic correctly.

This is tricky, and it might be easier to explain in PR reviews, but "the" command line mutates every time we encounter an .rsp. It's important to have all the switches we are currently aware of because if we've found an error, it must be among those, but at no point until we're finished are we confident that we have all the switches in our command line. If we were to try to set it once upon creating the CommandLineSwitches object, it would have the command line input by the user, but as you pointed out, that misses extra switches we find later. Since the list of known switches can change after processing any individual switch, we won't really have a chance to refresh CommandLineSwitches unless we either make a lot of them or accept that it isn't an intrinsic part of the object.

Forgind · 2022-01-10T16:37:53Z

Change format to:
Expanded command line: 'msbuild thing /switch'
\Directory.Build.rsp: /switch2 /switch3
\Directory.Build2.rsp: /switch4 /switch5

(Or full command line)

Forgind · 2022-01-10T18:20:09Z

Now looks like:

MSBUILD : error MSB1060: Undefined environment variable passed in as switch.
    Full command line: artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\MSBuild.exe  C:\Users\forgind\Desktop\Archives\Bug-specific\myTemp\myTemp.csproj
C:\Users\forgind\Desktop\Archives\Bug-specific\myTemp\Directory.Build.rsp: %ENVIRONMENTVARIABLE%
Switch: %ENVIRONMENTVARIABLE%

For switch syntax, type "MSBuild -help"

ladipro · 2022-01-10T15:35:24Z

src/MSBuild/XMake.cs

+#if FEATURE_GET_COMMANDLINE
+                        commandLine
+#else
+                        string.Join(" ", commandLine)


nit:

Suggested change

string.Join(" ", commandLine)

string.Join(' ', commandLine)

There isn't a string.Join overload for char.

There isn't a string.Join overload for char.

. . . until .NET Core 2.0.

This code is under an ifdef and does not run on Framework. Just a nit and I wrote it only because you're adding a call to the char-taking overload in this PR (line ~1653).

I thought I'd tested it at that location, but I was wrong. Thanks!

ladipro · 2022-01-11T21:21:35Z

src/MSBuild/XMake.cs

@@ -1901,25 +1911,27 @@ private static void GatherResponseFileSwitch(string unquotedCommandLineArg, Comm
                            }
                        }

-                        GatherCommandLineSwitches(argsFromResponseFile, commandLineSwitches);
+                        commandLine += $"\n{responseFile}: {string.Join(" ", argsFromResponseFile)}";


Ah, now I see why the command line string has to be passed by ref. I'm still not sure if this is the best approach. Ideally the command line would be kept in a more structured format (e.g. a dictionary mapping "response file" to "args from the response file") and maybe in a static field so it doesn't have to be passed around everywhere.

Aside from nicer and more allocation friendly code, It would make it possible to have a richer format of the output. I'm having second thoughts about the bare <response file>: <command line>, whether it is clear/expressive enough.

ladipro · 2022-01-11T21:24:48Z

src/MSBuild/Resources/Strings.resx

@@ -83,7 +83,8 @@
    </comment>
  </data>
  <data name="CannotAutoDisableAutoResponseFile" UESanitized="false" Visibility="Public">
-    <value>MSBUILD : error MSB1027: The -noAutoResponse switch cannot be specified in the MSBuild.rsp auto-response file, nor in any response file that is referenced by the auto-response file.</value>
+    <value>MSBUILD : error MSB1027: The -noAutoResponse switch cannot be specified in the MSBuild.rsp auto-response file, nor in any response file that is referenced by the auto-response file.
+    Full command line: {0}</value>


I think it's unfortunate that so many resource strings are changed and will need attention from the localization team. Do you think the command line could be printed using only one shared resource instead?

Couldn't find this for some reason. Found it now

I don't know why it suddenly realized that Execute sets MSBuildLoadMicrosoftTargetsReadOnly and started complaining about it, but it did. This effectively unsets it after relevant tests.

rainersigwald · 2022-01-31T17:48:16Z

src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs

@@ -476,7 +476,8 @@ public void TargetsSwitchIdentificationTests(string @switch)
        public void TargetsSwitchParameter()
        {
            CommandLineSwitches switches = new CommandLineSwitches();
-            MSBuildApp.GatherCommandLineSwitches(new List<string>() { "/targets:targets.txt" }, switches);
+            string command = string.Empty;
+            MSBuildApp.GatherCommandLineSwitches(new List<string>() { "/targets:targets.txt" }, switches, command);


Why not provide a default argument so you don't have to pass an empty string to these many callers?

Good point 🙂

rainersigwald · 2022-01-31T17:56:55Z

src/MSBuild.UnitTests/ProjectSchemaValidationHandler_Tests.cs

        {
            string[] msbuildTempXsdFilenames = Array.Empty<string>();
            string projectFilename = null;
            string oldValueForMSBuildOldOM = null;
+            string oldValueForMSBuildLoadMicrosoftTargetsReadOnly = Environment.GetEnvironmentVariable("MSBuildLoadMicrosoftTargetsReadOnly");


Why is this needed?

I don't know why it suddenly started noticing this, but there's an invariant check that we didn't change any environment variables, and this one gets set automatically early in Execute. This just ensures it isn't set before it checks that invariant.

src/MSBuild/CommandLineSwitches.cs

src/MSBuild.UnitTests/XMake_Tests.cs

src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs

src/MSBuild/CommandLineSwitches.cs

Ignore env variable switches

66ae06a

CarlHeinrichs reviewed Jan 4, 2022

View reviewed changes

Log command line on switch error

df38393

Add hack to string

ab4b6ba

rainersigwald requested changes Jan 5, 2022

View reviewed changes

Forgind added 5 commits January 5, 2022 11:34

Always point to the environment variable

4bf3613

Create new error

11a2359

Move check earlier

062b313

It otherwise only caught an environment variable if it was the first "project" switch but not if it was the duplicate.

Add test for new behavior

41427e4

Delete unused method

76afef4

Forgind changed the title ~~Ignore env variable switches Fixes #7210~~ 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 Jan 5, 2022

ladipro reviewed Jan 6, 2022

View reviewed changes

Include rsps in command line

44a87c9

Use TestEnvironment

4d9d090

Forgind added 3 commits January 7, 2022 12:37

Include response file paths in command line

ba4283c

Fix resource string

1e9d5c7

Build

6174c75

Forgind added 2 commits January 10, 2022 10:20

Adjust format

ad31551

Merge branch 'main' into ignore-environment-variable-switches

4753c7d

ladipro reviewed Jan 11, 2022

View reviewed changes

Forgind added 5 commits January 14, 2022 15:28

Make it easier on the loc team

af1f39c

Even easier

596a7c1

Properly make easier

5af7815

Release properly

55e9955

Couldn't find this for some reason. Found it now

Use ' ' and make tests pass

7fcf177

I don't know why it suddenly realized that Execute sets MSBuildLoadMicrosoftTargetsReadOnly and started complaining about it, but it did. This effectively unsets it after relevant tests.

ladipro mentioned this pull request Jan 24, 2022

MSBuild command line arguments aren't logged in dotnet build #7326

Merged

Merge branch 'main' into ignore-environment-variable-switches

e624a5e

rainersigwald reviewed Feb 7, 2022

View reviewed changes

Forgind added 4 commits February 7, 2022 16:11

PR comments + added test

5113a67

Little TestEnvironment cleanup

f50182a

GC SwitchesFromResponseFile

50894f0

Ensure initialized

4244db2

Forgind requested a review from rainersigwald February 14, 2022 04:51

rainersigwald approved these changes Feb 14, 2022

View reviewed changes

src/MSBuild.UnitTests/CommandLineSwitches_Tests.cs Outdated Show resolved Hide resolved

src/MSBuild/CommandLineSwitches.cs Outdated Show resolved Hide resolved

Keep switches in order

1c6c2c3

rainersigwald approved these changes Feb 14, 2022

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 14, 2022

Forgind added 2 commits February 15, 2022 17:49

Merge branch 'main' into ignore-environment-variable-switches

af3d14b

Add commandLine arg

0d7e80f

Forgind merged commit ceefec5 into dotnet:main Feb 18, 2022

Forgind deleted the ignore-environment-variable-switches branch February 18, 2022 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Forgind commented Jan 4, 2022

Forgind commented Jan 4, 2022

CarlHeinrichs left a comment

ladipro commented Jan 4, 2022

Forgind commented Jan 4, 2022

ladipro commented Jan 4, 2022

Forgind commented Jan 5, 2022 •

edited

Loading

rainersigwald left a comment

ladipro left a comment

ladipro Jan 6, 2022

Forgind commented Jan 6, 2022

ladipro commented Jan 7, 2022

Forgind commented Jan 7, 2022

Forgind commented Jan 10, 2022 •

edited

Loading

Forgind commented Jan 10, 2022

ladipro Jan 10, 2022

Forgind Jan 14, 2022

rainersigwald Jan 14, 2022

ladipro Jan 17, 2022

Forgind Jan 18, 2022

ladipro Jan 11, 2022

ladipro Jan 11, 2022

rainersigwald Jan 31, 2022

Forgind Feb 7, 2022

rainersigwald Jan 31, 2022

Forgind Feb 7, 2022

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

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

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

Conversation

Forgind commented Jan 4, 2022

Context

Changes Made

Testing

Notes

Forgind commented Jan 4, 2022

CarlHeinrichs left a comment

Choose a reason for hiding this comment

ladipro commented Jan 4, 2022

Forgind commented Jan 4, 2022

ladipro commented Jan 4, 2022

Forgind commented Jan 5, 2022 • edited Loading

rainersigwald left a comment

Choose a reason for hiding this comment

ladipro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind commented Jan 6, 2022

ladipro commented Jan 7, 2022

Forgind commented Jan 7, 2022

Forgind commented Jan 10, 2022 • edited Loading

Forgind commented Jan 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind commented Jan 5, 2022 •

edited

Loading

Forgind commented Jan 10, 2022 •

edited

Loading