Make RunCsc/Vbc scripts respect DOTNET_HOST_PATH #21237

khyperia · 2017-08-01T17:28:18Z

Logic: If DOTNET_HOST_PATH is set, use that as the path to dotnet. Otherwise, search in PATH.

agocke

LGTM

agocke · 2017-08-01T19:33:13Z

build/NuGetAdditionalFiles/RunCsc

+#  If VAR is non-empty => TEXT
+# Do this expression to make sure HOST_PATH ends with a slash,
+#  but if it's unset, have it be completely empty (look up dotnet in PATH)
+HOST_PATH=${DOTNET_HOST_PATH:+${DOTNET_HOST_PATH-}/}


Why would we do this vs. the approach in the CMD files: if DOTNET_HOST_PATH is defined then just use it directly?

It is doing that. @{VAR:+TXT} is equivalent to if (defined VAR) then (TXT) else ("") (pseudo-syntax).

The reason why it's not just directly ${DOTNET_HOST_PATH}dotnet is: what if DOTNET_HOST_PATH doesn't end in a slash? Say, /some/path, we'd then be invoking /some/pathdotnet.

If we wrote ${DOTNET_HOST_PATH}/dotnet, then an empty host path would cause /dotnet to be invoked, not dotnet.

The reason it's not an if statement is because bash doesn't have a "is this variable defined" builtin - the way to check if a var is defined is to use one of these features like :+, so if we're doing that anyway, why not just jump straight to the right thing.

Also, the reason there's a dash at the end of ${DOTNET_HOST_PATH-} in the inner expression is because I wasn't sure of evaluation order (outer->inner or inner->outer). If the inner is evaluated first, and DOTNET_HOST_PATH is unset, then that would trigger an error (if you have sane bash options set). However, as we know it can possibly be unset in non-error-cases, adding the - does a simple "replace with empty string if unset" instead of erroring. (${VAR-TXT}/${VAR:-TXT} mean "if VAR set, return ${VAR}, else return TXT", with : behavior differing on if VAR is the empty string - when the replacement string is the empty string, though, it doesn't matter, so - is fine)

tl;dr variables are complicated

Hmm, think I need to get a "bash for recovering unix guys" book. :)

jaredpar · 2017-08-01T19:36:16Z

build/NuGetAdditionalFiles/RunCsc

+#  If VAR is non-empty => TEXT
+# Do this expression to make sure HOST_PATH ends with a slash,
+#  but if it's unset, have it be completely empty (look up dotnet in PATH)
+HOST_PATH=${DOTNET_HOST_PATH:+${DOTNET_HOST_PATH-}/}


Why would we do this vs. the approach in the CMD files: if DOTNET_HOST_PATH is defined then just use it directly?

jaredpar · 2017-08-01T19:38:06Z

build/NuGetAdditionalFiles/RunCsc.cmd

 echo off
-dotnet %~dp0\csc.dll %*
+if defined DOTNET_HOST_PATH set HOST_PATH=%DOTNET_HOST_PATH%\
+if not defined HOST_PATH set HOST_PATH=


This line seems redundant. Why do we need it?

I wasn't sure how cmd treats unset variables vs. empty variables (e.g. if it raises an error in some cases if a variable is unset). I decided to not risk it and set it to the empty string if the variable is unset. If you know for sure that cmd always replaces unset variables with the empty string (without error/warning), I can remove this line.

(Docs are really difficult to find on cmd...)

I suppose this is actually kind of buggy, as it allows leakage of the HOST_PATH variable (if DOTNET_HOST_PATH is unset and HOST_PATH is set, it erroneously uses HOST_PATH). Changing it to if not defined DOTNET_HOST_PATH set HOST_PATH= would resolve that.

khyperia · 2017-08-02T16:18:04Z

Ping @agocke, could you re-review the latest changes? Took feedback for the .cmd scripts (and fixed the issue I mentioned above). Ping @jaredpar to re-review and see if that addressed feedback.

Also, batch stuff is ridiculous - when in a REPL, doing echo "%an_unset_var%" prints "%an_unset_var%", but doing the same thing in a .cmd script prints "". I can't find docs anywhere.

agocke

LGTM

jaredpar · 2017-08-02T20:47:31Z

build/NuGetAdditionalFiles/RunCsc

+#  If VAR is non-empty => TEXT
+# Do this expression to make sure HOST_PATH ends with a slash,
+#  but if it's unset, have it be completely empty (look up dotnet in PATH)
+HOST_PATH=${DOTNET_HOST_PATH:+${DOTNET_HOST_PATH-}/}


Hmm, think I need to get a "bash for recovering unix guys" book. :)

jaredpar · 2017-08-02T20:48:51Z

build/NuGetAdditionalFiles/RunCsc.cmd

-echo off
-dotnet %~dp0\csc.dll %*
+@echo off
+if defined DOTNET_HOST_PATH (


You mentioned you researched CMD defined variables a bit. Did you come to have a preference between the following two styles we use?

if "%DOTNET_HOST_PATH%" != "" ( if defined DOTNET_HOST_PATH (

No - I'm not sure if there's a significant difference. if defined looks stylistically cleaner, though, and I'd be less nervous about it (because of the following).

I was specifically researching behavior of unset variables - for example, my comment earlier in this thread:

when in a REPL, doing echo "%an_unset_var%" prints "%an_unset_var%", but doing the same thing in a .cmd script prints "".

I was trying to validate correct behavior, since it seems like CMD vars don't have two distinct null/empty states, only the one - so I was trying to validate that the set HOST_PATH= line (which unsets the var HOST_PATH) does the right thing in all scenarios (so we don't execute literally %HOST_PATH%dotnet), but all I'm really sure about at this point is that it works when testing locally :(

@jaredpar

* Enable building with a dotnet not on PATH In #68918 we removed inspecting DOTNET_HOST_PATH environment variable. This broke the scenario where a specific dotnet "hive" was installed to a location not on the PATH. When the .NET SDK commands invoke a sub-process (for example MSBuild), it sets the DOTNET_HOST_PATH environment variable to tell the sub-process "this is where the dotnet.exe that invoked this command is located". See #21237 and dotnet/cli#7311 for more info. This change reverts the behavior back to respect DOTNET_HOST_PATH, and if it isn't set it will just use "dotnet" and let the OS take care of finding the executable on the PATH. Fix #69150 * Fix tests to workaround MSBuild searching the PATH itself. * Respond to PR feedback. Make the change smaller until @jaredpar gets back. Only make the minimal change required, which is to check DOTNET_HOST_PATH.

Make RunCsc/Vbc scripts respect DOTNET_HOST_PATH

204b1c7

This is related to dotnet/cli#7311

khyperia added the Area-Infrastructure label Aug 1, 2017

dnfclas added the cla-already-signed label Aug 1, 2017

agocke requested review from agocke and jaredpar August 1, 2017 19:32

agocke approved these changes Aug 1, 2017

View reviewed changes

agocke reviewed Aug 1, 2017

View reviewed changes

jaredpar reviewed Aug 1, 2017

View reviewed changes

Clean up RunCsc/Vbc.cmd a tiny bit

d72bcf5

agocke approved these changes Aug 2, 2017

View reviewed changes

jaredpar approved these changes Aug 2, 2017

View reviewed changes

khyperia merged commit 6a6e456 into dotnet:master Aug 2, 2017

khyperia deleted the run_scripts_host_path branch August 2, 2017 21:16

khyperia mentioned this pull request Aug 17, 2017

Use DOTNET_HOST_PATH as the full path to dotnet, not the containing dir #21590

Merged

ericstj mentioned this pull request Jul 21, 2023

Building with a dotnet not on the PATH is broken #69150

Closed

eerhardt mentioned this pull request Jul 24, 2023

Enable building with a dotnet not on PATH #69186

Merged

Make RunCsc/Vbc scripts respect DOTNET_HOST_PATH #21237

Make RunCsc/Vbc scripts respect DOTNET_HOST_PATH #21237

Uh oh!

Conversation

khyperia commented Aug 1, 2017

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

agocke Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khyperia Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khyperia Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khyperia commented Aug 2, 2017

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

agocke Aug 1, 2017 •

edited

Loading

khyperia Aug 1, 2017 •

edited

Loading

khyperia Aug 1, 2017 •

edited

Loading