-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add DOTNET_HOST_PATH
property when initializing for framework-dependent apps
#118249
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new DOTNET_HOST_PATH
runtime property that represents the path to the dotnet
executable corresponding to the runtime being used to run framework-dependent applications. The property is only set for framework-dependent apps, not self-contained ones, since self-contained apps don't have a corresponding dotnet executable.
Key changes:
- Adds the runtime property during hostpolicy initialization for framework-dependent apps only
- Refactors executable extension handling to use consistent macros across platforms
- Adds comprehensive test coverage for the new property in various scenarios
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/native/corehost/hostpolicy/hostpolicy_context.cpp | Implements the logic to set DOTNET_HOST_PATH property for framework-dependent apps |
src/native/corehost/hostpolicy/coreclr.h | Adds DotNetHostPath enum value to common_property |
src/native/corehost/hostpolicy/coreclr.cpp | Maps DotNetHostPath enum to "DOTNET_HOST_PATH" string |
src/native/corehost/hostmisc/utils.cpp | Refactors to use EXE_FILE_EXT macro instead of pal::exe_suffix() |
src/native/corehost/hostmisc/pal.h | Defines EXE_FILE_EXT macro and removes exe_suffix() functions |
src/installer/tests/TestUtils/Constants.cs | Adds constant for the new runtime property name |
src/installer/tests/TestUtils/Assertions/CommandResultExtensions.cs | Makes class partial to support additional assertion methods |
src/installer/tests/TestUtils/Assertions/CommandResultExtensions.RuntimeProperty.cs | Adds helper methods for asserting runtime property presence/absence |
src/installer/tests/HostActivation.Tests/SelfContainedAppLaunch.cs | Tests that self-contained apps don't have the property |
src/installer/tests/HostActivation.Tests/RuntimeProperties.cs | Comprehensive tests for the new property in various scenarios |
src/installer/tests/HostActivation.Tests/NativeHosting/GetFunctionPointer.cs | Tests property behavior in native hosting scenarios |
src/installer/tests/HostActivation.Tests/NativeHosting/ApplicationExecution.cs | Tests property in native application execution |
src/installer/tests/AppHost.Bundle.Tests/AppLaunch.cs | Tests property behavior for bundled applications |
src/installer/tests/HostActivation.Tests/NativeHosting/GetFunctionPointer.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@baronfel at the .NET SDK tool level DOTNET_HOST_PATH will be available via both |
It makes sense. Traditionally, tools in various ecosystems use this order of precedence:
So environment variables overriding AppContext (from .csproj) is consistent with common patterns. |
One scenario where I've had challenges is accessing copy-local files. Another way of asking this is that there multiple |
Correct - this will not change that or other |
Agreed. Note that |
Got it. That context helps. Thanks! Do we have a first internal user for this ENV so that we can validate the design? |
@jaredpar agree entirely with @elinor-fung here. AppContext would be a last-resort, and the |
Are there any other CLI ENVs that should be generally productized. Whenever we create those, it is typically an indication that there is a legit problem to be solved and it will often be the case that it is not unique to the CLI/SDK. |
What's the user scenario that this last resort is fixing - given that CLI and MSBuild has to be doing what they have been doing so far? |
Components inside of Visual Studio / VS Code that launch |
Could you please elaborate on the details how this scenario works? I assume that there are two runtimes involved in the test explorer scenario: VS runs on the private .NET runtime bundled in the VS and the user code runs in whatever runtime the user test project references. Where are the process launches where this property helps? It cannot help for launching user code from the VS. |
@jkotas Check out this section in a doc I wrote for some details: https://hackmd.io/pKvDHW89TwOKPbG7FvnLWw?view#Cracks-in-the-sidewalk The problem happens when build time components are enlightened about local SDKs, and things of that nature, but the apphosts and other tools that need a Runtime are not. This hits developers working on |
Not that I could think of - but I also didn't know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good from my perspective. Waiting on @jkotas for any additional queries.
I am missing the end to end here. I expected that this is going to make something significantly simpler, but I hear that it is only going to make things even more complicated. It would be useful to see several PRs that show end-to-end. I am not sure whether getting this in at the last minute .NET 10, without having time to validate that it actually solves any problems end-to-end, is a good idea.
This doc talks about the same problem as what the local SDK feature is trying to solve. As far as I know, we are going to edit PATH and set DOTNET_ROOT to make the local SDKs work. Why can't this be our one and only way to deal with this? I understand that it is no prettiest, but nobody was able to come with anything better that actually works. Also, I think we should be moving towards launching processes using |
@baronfel, @rainersigwald do we have an issue tracking doing this work? Want to make sure this doesn't get lost in the shuffle |
I thought the goal here was to replace
I don't understand this, actually. The example given in the doc will not be any better -- in order to run |
I had thought that it would replace how the SDK computes that value in the short term. And it would be a path to replacing
Great point about string dotnetDir = Path.GetDirectoryName(Path.GetDirectoryName(Path.GetDirectoryName(RuntimeEnvironment.GetRuntimeDirectory())));
string dotnetPath = string.IsNullOrEmptry(dotnetDir) ? null : Path.Join(dotnetDir, "dotnet"); |
First, I would recommend invoking the .exe without going through If they really want to go through
|
Yes, 'want to do this' would be want to actually go through
I think relative to the runtime directory should handle any framework-dependent app (local SDK via global.json, local runtime via DOTNET_ROOT, custom search via publish-time |
We would like to start NAOT-compiling parts of the SDK. If SDK has a need to find |
In this particular situation the launching exe is the host, so presumably it could pass this information directly into the AOT binary when it does the pinvoke. I agree that this would be an sdk-specific contract, though. |
I was referring to the self-contained scenario as a self-contained regular app trying to launch another app using its own self-contained runtime. Native AOT SDK would be trying to launch another app using another (specific) runtime - not its own. And I do still think that is not a generic scenario we would try to enable. #88754 (comment) also acknowledged that native AOT SDK would need something different. None of |
DOTNET_ROOT would cover that case in the SDK scenario. |
How? Running SDK commands does not require a user setting |
You are right that you do not need to set DOTNET_ROOT when executing SDK commands. SDK sets the DOTNET_ROOT for you as needed to make things work. For example, try Why is it not enough to just set |
Since environment variables just propagate, I don't know that the host/runtime should assume that because you are running a .NET binary, all child processes should use the same runtime root. It generally feels off to me to have the host/runtime set environment variables, particularly with broad effects. |
I am not suggesting host/runtime to set DOTNET_ROOT. I agree that it is not be desirable for the reasons you have mentioned. I am suggesting SDK to set DOTNET_ROOT for all child processes. SDK sets DOTNET_ROOT for some child processes today. Can it set DOTNET_ROOT for all child processes? |
I think the SDK could do that (maybe It would still have the current problem of determining what the value should be though. That computation could be 'use |
Is |
I think so - as in the entrypoint being the With local SDKs via global.json, the current |
Is the proposal that the SDK inside of program.cs set DOTNET_ROOT if it's not already set or are you looking for us to find all places we launch child processes to specifically add this there? |
Yes, it would be the idea. The higher level goal is to make the extra logic required to support local runtime and SDK scenarios as simple as possible. The original reason behind this PR was to simplify it, but #118249 (comment) comment suggested that it would make it even more complicated than it is today. |
Note: I think this API is still useful for users who want to start a managed app from a managed app (completely separate from the SDK). For apps without an apphost they would use the apphsot path directly, while apphost apps would use |
I seem to remember we had a long very similar conversation a maybe five years ago. @jaredpar was a key part of that. The conclusion then was that we could only design a system that had no side-effects and didn't require special documentation and warnings. We found that very hard to design and I think gave up. |
I agree with this in principle, but aside from the SDK, who is doing this? Do we have telemetry or non-SDK customer feedback that indicates this mechanism is needed? It appears like a cheap/easy decision, but given the duration and complexity of this conversation I'm suspicious the answer is all that clear. |
It sounds like xoofx was running into this, and presumably not in an SDK scenario. I’ve hit this in MSBuild tasks. I’m not sure whether MSBuild tasks count as an SDK scenario. |
I could see this being used in C#DK, and we also found a use for an internal project (can't mention that here), though this is also managed by the SDK Team, it's not the SDK 😁 |
If we can convince ourselves that this is worth having public API for, I would rather build it as a proper API. Python has API like that:
Could you please share a link where you would use it in C# SDK? |
Sure, but I mean C# DevKit. https://github.com/dotnet/vscode-dotnet-runtime/blob/6805569d16253ec4535be1666c6caf5e4759031a/vscode-dotnet-runtime-library/src/Acquisition/DotnetPathFinder.ts#L403 is in node.js but @JakeRadMSFT is thinking of having some of this logic in a separate .NET application for VS and VS Code that can find .NET runtimes if the PATH is not set and a user is using the global.json paths feature. In this use case, we rely on the fact that dotnet --list-runtimes will always use the real assembly path to output the runtime folder, and we could instead leverage an API like this. We needed to do this because a path such as '/usr/bin/snap' |
How is the separate .NET application going to find the runtime to run on? It would be useful to see the actual code for all this. |
Client applications using the MSBuild API to load and/or build projects are also an "SDK" entrypoint IMO, so we should extend the
I'd say "yes" in general, and include Framework MSBuild-from-VS launching a .NET application in the scenario list (though we hope to reduce the need for those with .NET TaskHosts). |
I don't think I have strong feelings about this PR, but as I read it it doesn't address the main scenario I am concerned about, which is IDEs. An IDE
With SDK resolver changes we've gotten 3 worked out pretty well, but quickly ran into problems with 4 since it's decoupled from build. What should IDEs do when they want to launch the application-under-development? |
If they want to launch the application on a local runtime, they need to set DOTNET_ROOT environment variable to point to that runtime before launching the application. I do not think there is any magic how an application can find the runtime in a custom location. The custom location has to be communicated to the app somehow. |
Add
DOTNET_HOST_PATH
runtime property to represent the path to thedotnet
executable corresponding to the runtime being used to run the application. Applications will be able to retrieve this value withAppContext.GetData("DOTNET_HOST_PATH")
.This goes through the properties passed directly at initialization, instead of just the callback on the host-runtime contract. Currently,
AppContext
is populated with the runtime properties from initialization and does not flow through to calling the host-runtime callback. Long-term, I think it should, but that is a much more impactful change.cc @dotnet/appmodel @AaronRobinsonMSFT @baronfel @jaredpar @richlander
See #88754 (comment)