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

Do not consider DOTNET_ROOT when finding dotnet #69010

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

jaredpar
Copy link
Member

This changes our dotnet launch code to not consider $DOTNET_ROOT as that is a apphost specific variable.

After discussion with the runtime team determined that only $PATH will be considered for finding dotnet.

dotnet/runtime#88754

This changes our `dotnet` launch code to not consider `$DOTNET_ROOT` as
that is a apphost specific variable.

After discussion with the runtime team determined that only `$PATH` will
be considered for finding `dotnet`.

dotnet/runtime#88754
@jaredpar jaredpar requested a review from a team as a code owner July 12, 2023 21:20
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 12, 2023
@jaredpar jaredpar added this to the 17.8 milestone Jul 12, 2023
@333fred
Copy link
Member

333fred commented Jul 13, 2023

Reading the linked issue, I have no idea why not looking in DOTNET_ROOT is the right thing to do. If anything, I draw the opposite conclusion. Before we proceed with this, I think we need the guidance @richlander mentioned clearly documented.

@jaredpar
Copy link
Member Author

https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-environment-variables#dotnet_root-dotnet_rootx86

You can see there that DOTNET_ROOT is meant only for apphost scenarios. Our use of it in my previous change ended up breaking scenarios where it was being set in infra for apphost only but we were using it for normal launch. That discussion is what spawned that linked issue.

@@ -53,15 +53,15 @@ internal static (string processFilePath, string commandLineArguments, string too
/// Get the path to the dotnet executable. In the case the host did not provide this information
/// in the environment this will return simply "dotnet".
/// </summary>
/// <remarks>
/// See the following issue for rationale why only %PATH% is considered
/// https://github.com/dotnet/runtime/issues/88754
Copy link
Member

Choose a reason for hiding this comment

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

I'd honestly prefer the docs link here, that was far clearer than this thread 🙂

@jaredpar
Copy link
Member Author

@dotnet/roslyn-compiler can i get one more review here, small change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers 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.

4 participants