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

Enable building with a dotnet not on PATH #69186

Merged
merged 3 commits into from
Jul 24, 2023
Merged

Conversation

eerhardt
Copy link
Member

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

cc @jaredpar @333fred @ericstj @vitek-karas @dsplaisted

In dotnet#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 dotnet#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 dotnet#69150
@eerhardt eerhardt requested a review from a team as a code owner July 24, 2023 16:25
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 24, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jul 24, 2023
@am11
Copy link
Member

am11 commented Jul 24, 2023

Thank you! Just ran into this error with rc1 daily build on osx-arm64. The workaround was to symlink the sfx dir like this:

# download daily build at ~/.dotnet8 (with `alias dotnet8=~/.dotnet8/dotnet`)
# expect 'dotent8 publish' etc. to work as it's always been..

# (hopefully a temporary) workaround
sudo ln -s ~/.dotnet8/shared/Microsoft.NETCore.App/8.0.0-rc.1.23371.3 /usr/local/share/dotnet/shared/Microsoft.NETCore.App/8.0.0-rc.1.23371.3

@333fred
Copy link
Member

333fred commented Jul 24, 2023

Before I approve a PR here I think I need a complete explanation of how dotnet is supposed to be found by a subprocess. DOTNET_HOST_PATH is not a variable that has been documented anywhere AFAICT, and from what I can recall @jaredpar was mentioning that it was an arcade invention (though I could be misremembering this and he was referring to a different variable?).

Additionally, if this is a revert of a prior commit, I would prefer it was actually a revert commit, not a new change.

@am11
Copy link
Member

am11 commented Jul 24, 2023

DOTNET_HOST_PATH is internally set by SDK for the entire toolchain. I think, it is not supposed to be set publicly but .NET Foundation tools are encouraged/free to rely on it.

@vitek-karas
Copy link
Member

I think I need a complete explanation of how dotnet is supposed to be found by a subprocess.

The problem is that we really don't have one: dotnet/runtime#88754
Changes to this are problematic as there's no clear "right way" of doing things. In my opinion it's better to keep things as they are, or apply only targeted fixes, until we come up with a good higher-level design.

Adding @agocke for some more context as well.

@eerhardt
Copy link
Member Author

eerhardt commented Jul 24, 2023

I can't speak to the documentation, but this has been the way things work for ~6 years. Multiple places across the product use this mechanism.

@marcpopMSFT @dsplaisted @baronfel - is it possible to document this environment variable to address @333fred's concerns?

@eerhardt
Copy link
Member Author

In my opinion it's better to keep things as they are, or apply only targeted fixes, until we come up with a good higher-level design.

@vitek-karas - by "keep things as they are" do you mean as they currently are in dotnet/roslyn main? Or "as they are" in .NET 8.0-preview6?

@vitek-karas
Copy link
Member

@vitek-karas - by "keep things as they are" do you mean as they currently are in dotnet/roslyn main? Or "as they are" in .NET 8.0-preview6?

Sorry for not being clear - I meant the behavior before the break (basically what we shipped in .NET 7).

@baronfel
Copy link
Member

Many community tools for making use of the SDK have added this as well - I'm thinking of Buildalyzer and Ionide.ProjInfo here specifically. We should document it as a way to ensure consistency of experience across a single scope/lifetime - often a single dotnet CLI command, but could also be things like using user-local SDKs with language tooling, for example. Overall the guidance for tools that want to use the same dotnet as a theoretical parent caller seems like it should be:

  • check DOTNET_HOST_PATH and use that explicitly if set, else
  • probe PATH according to the rules of your platform

@eerhardt
Copy link
Member Author

Additionally, if this is a revert of a prior commit, I would prefer it was actually a revert commit, not a new change.

@333fred, there were multiple things being changed/fixed in #68918. If we reverted that commit, we would then need to re-open and re-fix the bug it was intended to be fixing - #67102.

@marcpopMSFT
Copy link
Member

Many community tools for making use of the SDK have added this as well - I'm thinking of Buildalyzer and Ionide.ProjInfo here specifically. We should document it as a way to ensure consistency of experience across a single scope/lifetime - often a single dotnet CLI command, but could also be things like using user-local SDKs with language tooling, for example. Overall the guidance for tools that want to use the same dotnet as a theoretical parent caller seems like it should be:

  • check DOTNET_HOST_PATH and use that explicitly if set, else
  • probe PATH according to the rules of your platform

Broader usage of the env variable appears to be somewhat limited: https://github.com/search?q=DOTNET_HOST_PATH++&type=code Doesn't mean we shouldn't support it as an alternative to the path though as it has come up a few times (some customers have requested SDK path be part of global.json to solve this). Seems like an area worth investing in.

Make the change smaller until @jaredpar gets back. Only make the minimal change required, which is to check DOTNET_HOST_PATH.
@333fred 333fred enabled auto-merge (squash) July 24, 2023 21:21
@333fred 333fred merged commit 7db1e56 into dotnet:main Jul 24, 2023
25 checks passed
@ghost ghost added this to the Next milestone Jul 24, 2023
@eerhardt eerhardt deleted the Fix69150 branch July 24, 2023 22:00
@allisonchou allisonchou modified the milestones: Next, 17.8 P1 Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building with a dotnet not on the PATH is broken
8 participants