Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Set DOTNET_ROOT from the CLI to ensure any child processes launched use same root #9192

Closed
wants to merge 1 commit into from

Conversation

natemcmaster
Copy link

Partial solution to https://github.com/dotnet/cli/issues/9114.

This sets DOTNET_ROOT (or DOTNET_ROOT(x86)) at the beginning of the dotnet.dll process. This should help ensure any child processes launched by the SDK via apphost will run on the same installation of .NET Core as the dotnet SDK. These child processes may include dotnet run, bundled tools, global tools, processes launched through MSBuild, and more.

@livarcocc
Copy link

@peterhuene @wli3

@natemcmaster are you going to take this change to shiproom for approval?

@livarcocc livarcocc added this to the 2.1.3xx milestone May 3, 2018
@livarcocc
Copy link

livarcocc commented May 3, 2018

Please, fill out the ask mode template and tag matt for M2 approval. After which point we can take it to shiproom.

You can see the ask mode template here: #9160.

@natemcmaster
Copy link
Author

are you going to take this change to shiproom for approval?

Yes. I'll send in a few minutes.

@nguerrera
Copy link
Contributor

I think we agreed NOT to do this. cc @richlander @steveharter

@wli3
Copy link

wli3 commented May 3, 2018

@nguerrera @dsplaisted we need carefully consider this change. This will alter the behavior of future selfcontain

try
{
var rootPath = Path.GetDirectoryName(new Muxer().MuxerPath);
env.SetEnvironmentVariable(dotnetRootEnv, rootPath, EnvironmentVariableTarget.Process);

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Contributor

@nguerrera nguerrera left a comment

Choose a reason for hiding this comment

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

I don't think we should do this.

@natemcmaster
Copy link
Author

This will alter the behavior of future selfcontain

@wli3 Can you explain? Why would self-contained apps need to look at DOTNET_ROOT at all?

DOTNET_ROOT was already set to a location with runtimes needed for my tools,

So you're saying you want to support a scenario where the .NET Core SDK can launch tools using runtimes from different folder, not next tot the SDK. How common is this scenario? Has this every worked before 2.1.300? My guess is this is less common than users who want to invoke dotnet watch, if it's ever used at all.

DOTNET_ROOT was not set, but default global location has the runtimes needed for my tools

Hasn't this already been solved via multi-level lookup? Or does DOTNET_ROOT imply DOTNET_MULTILEVEL_LOOKUP=0?

AFAICT, this change doesn't even give me a workaround if I'm broken by it.

It's a new feature, right? so this can't break customers who have only used stable tooling. IMO it's already a broken feature. I'm just trying to help by pointing out that the current design is broken for common scenarios.

catch
{
// swallow and ignore in the event MuxerPath throws because it cannot find dotnet.exe
return;

This comment was marked as spam.

@nguerrera
Copy link
Contributor

nguerrera commented May 3, 2018

This should help ensure any child processes launched by the SDK via apphost will run on the same installation of .NET Core as the dotnet SDK. These child processes may include dotnet run, bundled tools, global tools, processes launched through MSBuild, and more.

This seems to imply more power than DOTNET_ROOT actually has. It is used only by apphost for activation of framework-dependent executables.

bundled tools

This is a case where we absolutely do want to control the runtime, but we don't run these as framework-dependent exes, so DOTNET_ROOT does not apply. Just tried some\path\to\dotnet watch run with noC:\Program Files\dotnet and it is working fine already.

I have commented before that I don't believe we should ever bundle framework-dependent exes with the CLI.

global tools

Only for dotnet-foo case run as dotnet foo.

Hasn't this already been solved via multi-level lookup? Or does DOTNET_ROOT imply DOTNET_MULTILEVEL_LOOKUP=0?

I'm double checking this now, but I don't believe multi-level lookup has any impact on apphost activation of framework dependent exes.

(EDIT: I was wrong here, but multi-level lookup currently only works on Windows.)

@nguerrera
Copy link
Contributor

It's a new feature, right? so this can't break customers who have only used stable tooling

I wasn't talking about breaking changes in that sense. I meant: if this behavior is not what I want, how do I opt out of it?

@nguerrera
Copy link
Contributor

nguerrera commented May 3, 2018

So you're saying you want to support a scenario where the .NET Core SDK can launch tools using runtimes from different folder, not next tot the SDK. How common is this scenario? Has this every worked before 2.1.300?

The sub-processes of the SDK have always been open-ended. MSBuild is arbitrarily extensible. This change is asserting that the correct runtime for any framework-dependent .exe launched anywhere in the process subtree of a CLI command must be the same as the CLI runtime. I don't think this will hold always.

Let's say we have framework-dependent.exe on disk somewhere. I run framework-dependent.exe from my shell and it works fine. I then add <Exec Command="framework-dependent.exe" /> to my project and then do /some/experimental/dotnet build and it fails.

@peterhuene
Copy link

peterhuene commented May 3, 2018

@nguerrera would you be more inclined to support this PR if:

  • It respected DOTNET_ROOT from the environment as an opt-out of this default behavior.
  • It limited setting the variable to spawned resolved external commands and not process-wide (i.e. to specifically improve the UX for dotnet-prefixed global tools). I don't think it's entirely unreasonable for dotnet "commands" to share the same root as dotnet itself, at least by default.

@nguerrera
Copy link
Contributor

@peterhuene Those would make me slightly happier, but I think we need to think about this carefully and find the right solution at the right layer.

For example, a nicer solution would be if we could append ourselves to a DOTNET_ROOTS (plural) such that we only give an extra place to look for compatible runtimes. It should be technically feasible to fix broken cases without breaking working cases. In the meantime, I just don't think that having to set DOTNET_ROOT in addition to PATH is onerous enough to rush a half solution that may have unintended consequences.

@natemcmaster
Copy link
Author

It doesn't sound like this is going to get shiproom approval so we'll just have to live with the current behavior.

Beffyman added a commit to Beffyman/AspNetCore.Client that referenced this pull request Aug 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants