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

Supporting local SDK deployment in global.json #303

Merged
merged 17 commits into from
May 10, 2024
Merged

Conversation

jaredpar
Copy link
Member

@jaredpar jaredpar commented Nov 8, 2023

This proposal extends global.json such that it supports locally deployed instances of the .NET SDK. This will significantly reduce friction our local deployment has with developer tools like Visual Studio, VS Code and make our own developer story much simpler.

@xoofx
Copy link
Member

xoofx commented Nov 8, 2023

Thanks for writing this proposal! Looks like it's covering our use case for using a custom dotnet SDK with Unity + MSBuild.

### Do we need additionalLocationsOnly?
The necessity of this property is questionable. The design includes it for completeness and understanding that the goal of some developers is complete isolation from machine state. As long as we're considering designs that embrace local deployment, it seemed sensible to extend the design to embrace _only_ local deployment.

At the same time the motivation for this is much smaller. It would be reasonable to cut this from the design and consider it at a future time when the motivation is higher.
Copy link
Member

Choose a reason for hiding this comment

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

I believe we can do without it, but at the same time, being able to set this makes silly onboarding mistakes much easier to catch.

Copy link
Member

Choose a reason for hiding this comment

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

When .NET workloads are involved, I definitely don't want my teammates accidentally polluting their "global" SDK locations with workloads they don't need. It's nicer to keep those isolated.

But still, I agree, not a requirement.

As a result solutions like "just use .dotnet if it exists" fall short. It will work in a lot of casse but will fail in more complex scenarios. To completely close the disconnect here we need to consider all the possible locations.

### Do we need additionalLocationsOnly?
The necessity of this property is questionable. The design includes it for completeness and understanding that the goal of some developers is complete isolation from machine state. As long as we're considering designs that embrace local deployment, it seemed sensible to extend the design to embrace _only_ local deployment.
Copy link
Member

Choose a reason for hiding this comment

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

There are things that can change a particular SDK state (namely workload installs). So in the event a user has the same SDK in both the additional paths and globally, this may be needed for that type of situation but it's also likely fairly niche.

Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly this in effect equals to DOTNET_MULTILEVEL_LOOKUP=0?

Copy link
Member

Choose a reason for hiding this comment

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

That is my understanding as well. With the difference that now it would work the same across all platforms (the env. variable only ever worked on windows, and now it's off by default anyway).

Copy link
Member

Choose a reason for hiding this comment

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

I don't like this property as much. If the goal of some developers would be isolation from the machine state, then why not just set DOTNET_ROOT after a repo-specific install?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like this property as much. If the goal of some developers would be isolation from the machine state, then why not just set DOTNET_ROOT after a repo-specific install?

How can tooling like say Visual Studio pick up such an environment variable? Yes you can do tricks like set the variable then start VS but that impacts the entire dev workflow. Actions like using the quick launch menu don't work anymore cause VS can't process your repo without extra information. Any external process that launches VS (like say a debugger attach event) don't work in the repo cause they don't set the environment variable. It's an endless series of gotchas.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Are you proposing a change to the failure case? For instance "I need 9.0.100-preview.2 exactly and it must be in .dotnet but the user hasn't run the script to make that happen".

proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
@baronfel
Copy link
Member

baronfel commented Nov 8, 2023

Just a note that if we take this we should update the official schema for global.json files that's over in SchemaStore to declare the new properties (and link to docs for them).

@jaredpar
Copy link
Member Author

jaredpar commented Nov 8, 2023

Are you proposing a change to the failure case? For instance "I need 9.0.100-preview.2 exactly and it must be in .dotnet but the user hasn't run the script to make that happen".

This proposal is narrowly focused on changing the host resolver algorithm only. This means it should be effectively a transparent change to the rest of the tooling stack. If the correct SDK was not found it would emit an error just as it does today. Maybe the error text updates a bit to list the locations considered (not sure if that is surfaced today). It would not attempt anything like auto-acquiring the correct SDK.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

Few nits

proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
### Do we need additionalPathsOnly?
The necessity of this property is questionable. The design includes it for completeness and understanding that the goal of some developers is complete isolation from machine state. As long as we're considering designs that embrace local deployment, it seemed sensible to extend the design to embrace _only_ local deployment.

At the same time the motivation for this is much smaller. It would be reasonable to cut this from the design and consider it at a future time when the motivation is higher.
Copy link
Member

Choose a reason for hiding this comment

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

I would very much like this, it's not fun when random machine state changes behavior.

proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved

At the same time the motivation for this is much smaller. It would be reasonable to cut this from the design and consider it at a future time when the motivation is higher.

### Best match or first match?
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps there could be another property to control this option (if people want it).


As a result solutions like "just use .dotnet, if it exists" fall short. It will work in a lot of casse but will fail in more complex scenarios. To completely close the disconnect here we need to consider all the possible locations.

### Do we need additionalPathsOnly?
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider that having the $machine$ means that:

"additionalPaths": [ ".dotnet", "$machine$" ],
"additionalPathsOnly": "true"

is equivalent to

"additionalPaths": [ ".dotnet" ],
"additionalPathsOnly": "false"

Would it make sense to only use additionalPaths - and maybe rename it to something like installPaths. And if specified it would the only list searched?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did think about that and I agree it's a valid approach to this. I've frankly waffled on which I think is the better approach here.

I think in most scenarios customers would want to consider machine based installations. No data here, just intuition. The $machine$ value is a bit of a magic value. In the installPaths approach that would mean that $machine$ would end up in the majority of uses of installPaths. Had a hard time convincing myself that was the best outcome which is why I ended up with the current approach.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not advocating for either way, but having $machine$ is slightly more flexible, because you can switch the order:

"additionalPaths": [ "$machine$", ".dotnet" ],

Not sure why anyone would want to do that, but I'm sure someone will come up with a use-case at some point.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why anyone would want to do that, but I'm sure someone will come up with a use-case at some point.

That's basically what the Arcade script does that acquires the .NET SDK. It first checks if a direct match (based on the sdk version) is found in the %PATH% location and only if not then downloads the SDK to the local ".dotnet" directory.

That script could honor the selection here to make that behavior more configurable.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: $machine$ doesn't mean anything to me. I'd call this $default_install$ or similar.

And yeah, if we have a way to refer to the default install I would change additionalPaths to installPaths and just search those paths precisely in order.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, we still need to know whether to check $PATH.

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 prefer not having a separate additionalPathsOnly property.

Given that the magic is much less in 7+ (we only look for the SDK relative to the running dotnet and I don't recall fallout from it), I'd expect $machine$/$default_install$ shouldn't end up in most uses.

Since the fallback logic would be to look relative to the running dotnet, I'd still consider that only local deployment and I could see always looking next to running dotnet (after additionalPaths/installPaths) being reasonable.

@agocke
Copy link
Member

agocke commented Nov 9, 2023

Confirming: this is an SDK-only feature, correct? That is, the behavior of the app host or of dotnet blah.dll would not be affected? That appears to be true, as written. However, I would change the "host resolver" to simply "SDK resolver" to remove confusion.

This proposal is designed at giving global.json more control over how SDKs are found. If the global.json asked for a specific path to be considered and it has a matching SDK but a different SDK was chosen, that seems counter intuitive. Even in the case where the chosen SDK was _better_. This is a motivating scenario for CI where certainty around SDK is often more desirable than _better_. This is why the host discovery stops at first match vs. looking at all location and choosing the best match.

Best match is a valid approach though. Can certainly see the argument for some customers wanting that. Feel like it cuts against the proposal a bit because it devalues `additionalPaths` a bit. If the resolver is switched to best match then feel like the need for `additionalPathsOnly` is much stronger. There would certainly be a customer segment that wanted to isolate from machine state in that case.

Copy link
Member

Choose a reason for hiding this comment

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

Additional things worth mentioning:

  • This will not affect the runtime resolution when executing dotnet exec app.dll
    • Maybe not a big deal - depends on the use cases - potentially we could introduce something like dotnet exec --use-sdk-locations app.dll or something like that to ask runtime resolution to also consider global.json

Copy link
Member

Choose a reason for hiding this comment

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

  • Compatibility considerations - this relies on the dotnet.exe which is on the user's path to be able to run the SDK specified in the global.json. This is not a big concern for the "Fallback to machine wide" since that will typically be the dotnet.exe used, but it is a concern for repo-local installs. This can lead to things like:
    • 8.0 dotnet.exe is used to run 9.0.0-preview... SDKs (so we have to be really strict about forward compatibility)
    • 8.0 dotnet.exe is used to run 3.1.0 SDK (so we have to be really strict about backward compatibility)
      None of this is exactly new, but specifically the forward compatibility doesn't have a common use case currently (it can happen, but it's very rare).

Copy link
Member

Choose a reason for hiding this comment

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

The forward compatibility is somewhat concerning to me. Do we have a list of API dependencies that the muxer needs? And a strategy to ensure that those APIs don't change in the future?

Copy link
Member

Choose a reason for hiding this comment

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

I could get you the list - it's VERY short (I think 2 APIs or maybe even just 1).
We are very aware of this requirement and we never touched those APIs. So I'm not super concern about this, but it does raise the importance of keeping it compatible since so far all the cases where it can happen are pretty rare, this would be basically the first real feature which makes use of it.

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 like to see the "doesn't affect dotnet exec app.dll" part called out explicitly. Naively I would have expected it to, though I think it's ok that it doesn't.

Copy link
Member

@MarcoRossignoli MarcoRossignoli Feb 5, 2024

Choose a reason for hiding this comment

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

Correct we set VSTEST_WINAPPHOST_* vars and we use it in case of apphost usage(testhost.exe) https://github.com/microsoft/vstest/blob/main/src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs#L488

Before the --arch support we used the default naming DOTNET_ROOT_* but architecture switch uses these and so we had to "pass" the sdk root information in a different one.

Architecture switch uses the DOTNET_ROOT_* to look for the correct muxer that can be set by users and can be different than the sdk one https://github.com/microsoft/vstest/blob/main/src/Microsoft.TestPlatform.CoreUtilities/Helpers/DotnetHostHelper.cs#L153

@nohwnd something to add?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to see the "doesn't affect dotnet exec app.dll" part called out explicitly. Naively I would have expected it to, though I think it's ok that it doesn't.

I agree that tripped me up to so I will call it out. It materially impacts the scenarios here cause it is common in builds to effectively build a .NET core based tool then launch it with dotnet exec. That is done many times in the Roslyn build.

At the same time it seems like we can continue this with a small tweak. Just need to flip to using dotnet run instead of dotnet exec in our scripts. That fixes other problems like having to hard code in the location of the built binary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Other SDK commands set DOTNET_HOST_PATH based on the running dotnet. Tools running as part of an SDK command look at its value.

Want to make sure that when we run dotnet build that DOTNET_HOST_PATH gets set to the resolved runtime / SDK. Example it would get set to the app local .NET SDK. Pretty sure based on reading that is the case but wanted to double check.

Copy link
Member Author

@jaredpar jaredpar Feb 6, 2024

Choose a reason for hiding this comment

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

Want to elaborate on DOTNET_HOST_PATH a bit. Let's assume I have the following:

  • dotnet.exe is installed at C:\Program Files\dotnet\dotnet.exe and is on %PATH%. This location includes a 7.0.400 .NET SDK install
  • The current directory is c:\source and has the following:
    • .dotnet which is a directory that has 8.0.100 .NET SDK installed
    • global.json that requires 8.0.100 .NET SDK and has path: ["$host$", ".dotnet" ].

Now in the current directory I run dotnet build inside c:\source. That will resolve to c:\Program Files\dotnet\dotnet.exe since it's on %PATH% but it will use the use the 8.0.100 SDK from c:\source\.dotnet because the machine wide installation does not work.

What will the value of %DOTNET_HOST_PATH% be inside of msbuild here? That needs to be c:\source\.dotnet\dotnet.exe otherwise this proposal falls apart. The msbuild environment depends on this to dotnet exec tools inside of it so it must be a dotnet.exe that will have access to the 8.0.100 runtime.

@elinor-fung, @agocke, @rainersigwald, @baronfel

Copy link
Member

Choose a reason for hiding this comment

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

What will the value of %DOTNET_HOST_PATH% be inside of msbuild here? That needs to be c:\source\.dotnet\dotnet.exe otherwise this proposal falls apart.

As part of the implementation of this proposal, I think it should become c:\source\.dotnet\dotnet.exe. I think the SDK commands that currently set some variable to 'the running dotnet' intend that value to represent 'the dotnet for the running SDK`, so they would need to be updated.

```

In this configuration the host resolver would find a compatible SDK, if it exists in `.dotnet` or a machine wide location. The host resolver will consider the `additionalPaths` in the order they are defined and will stop at the first match. If none of the locations have a matching SDK then
will fall back to the existing SDK resolution strategy: `%PATH%` followed by machine wide installations.
Copy link
Member

Choose a reason for hiding this comment

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

I think this will need more detailed specification. Currently the SDK resolution has two "modes":

  • Multi-level lookup disabled (the default for .NET 8) - in this case only the location of the dotnet.exe used to invoke the command.
  • Multi-level lookup enabled (opt-in via DOTNET_MULTILEVEL_LOOKUP=1, which is considered "obsolete" really) - in this case the location of the dotnet.exe is considered first, followed by the global location

Note that the resolver doesn't look at PATH. Frequently the dotnet.exe will come from PATH, but if I run C:\myfolder\dotnet.exe build then PATH is actually completely ignored.

We need to define what fallback is considered - if it means the location of the dotnet.exe used to invoke the command, or the actual "machine-wide" install, or both.

@sharwell
Copy link
Member

We (Roslyn team) are desperately in need of this fix.

@CyrusNajmabadi
Copy link
Member

FYI, this is probably my biggest pain point being a dev on hte roslyn team. Constant breakages here are pretty frustrating :)

"[markdown]": {
"editor.rulers": [80],
"editor.wordWrap": "bounded",
"editor.wordWrapColumn": 80
Copy link
Member Author

Choose a reason for hiding this comment

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

These settings match with the linting rules in this repo. I'd prefer the linting rules be changed (why 80 characters hard wrap? this is 2023, not 1980 😉 ) but at the moment leaning into previous decisions.

{
"sdk": {
"paths": [ ".dotnet", "$host" ],
"errorMessage": "The .NET SDK could not be found, please run ./install.sh."
Copy link
Member Author

@jaredpar jaredpar Feb 1, 2024

Choose a reason for hiding this comment

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

@agocke chose this name, blame him. 😉

Copy link
Member

Choose a reason for hiding this comment

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

Go explicit with resolutionFailureMessage, maybe? Not a strong opinion, errorMessage is ok by me.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

Two minor corrections, otherwise LGTM.

Just want to confirm that our editors/dev tools use this entry point.

The values in the `paths` property can be a relative path, absolute path or
`$host$`. When a relative path is used it will be resolved relative to the
location of the containing global.json. The value `$host$` is a special value
that represents the machine wide installation path of .NET SDK for the
Copy link
Member

Choose a reason for hiding this comment

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

Actually $host$ is the path of the current dotnet.exe that was the entry point. This exe can be anywhere -- globally installed, locally installed, current directory, etc.

Copy link
Member

Choose a reason for hiding this comment

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

$host$ would mean "the .NET 8 and earlier behavior", right?

Copy link
Member

Choose a reason for hiding this comment

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

From .NET 7 and later, the host only looks next to the running dotnet.exe. I don't think we want anything representing the previous (.NET 6 and below, only different on Windows) behaviour.

hence discovery stops there.

This design requires us to only change the host resolver. That means other
tooling like Visual Studio, VS Code, MSBuild, etc ... would transparently
Copy link
Member

Choose a reason for hiding this comment

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

I don't know whether or not this is true. @elinor-fung do you happen to know what entry points this change would effect and whether our dev tools would use the same entry points?

Copy link
Member

Choose a reason for hiding this comment

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

I'll leave the detailed analysis to @elinor-fung , but I think it will work out that way. The changes will be in hostfxr and we've been advocating for all of the other tools to use hostfxr to locate the SDK. Specifically MSBuildLocator does use hostfxr. That said, I think it would be good to include testing of some of the obvious use cases as part of the feature work on this (we've been surprised in the past unfortunately).

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, the fact this lives in hostfxr will mean that it's versioning behavior will be somewhat "unexpected". As long as one has the .NET 9 version with this change installed, it will work regardless of the SDK version chosen in the global.json because we always use the latest hostfxr available in a given installation. This includes previews, so just installing a preview of .NET 9 will make this work basically everywhere on the machine.

Copy link
Member

Choose a reason for hiding this comment

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

MSBuildLocator using hostfxr will give this behavior for VS Code/DevKit scenarios.

For VS proper it's more complicated: VS will only ever use the copy of MSBuild that it distributes, but the .NET SDK is located by calling hostfxr, so this proposal should work fine there too--it's just a nicer way to get the same mix of components you can get today (with a global.json + global SDK install, or setting environment variables to tell the resolver to use a private SDK).

MSBuild.exe is the same as VS in this respect (you pick an MSBuild.exe to run, then it loads its own copies of things, but calls the .NET SDK resolver to find the SDK).

There are surely some build-ish tools that exist that don't call hostfxr (forks from older Locators or reimplementations), but I can't see this making anything worse for them, and I don't know of any offhand.

graph LR

subgraph entrypoints
    MSBuild.exe[<pre>MSBuild.exe</pre>]
    VS[Visual Studio]
    DevKit
    cli[<pre>dotnet</pre> CLI]
    rando[other apps]
end

hostfxr

sdk[.NET SDK Resolver]

VS --> sdk

MSBuild.exe --> msbenv[/<pre>MSBuildSDKsPath</pre> env var/]
MSBuild.exe --> sdk

sdk --> hostfxr
sdk --> env[/<pre>DOTNET_MSBUILD_SDK_RESOLVER_SDKS_DIR</pre> env var/]

cli --> hostfxr

DevKit --> MSBuildLocator --> hostfxr

rando --> MSBuildLocator

rando --> oldLocator[MSBuildLocator vOld] --> info[<pre>dotnet --info</pre>] --> hostfxr

rando --> info
Loading

Copy link
Member

Choose a reason for hiding this comment

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

I will take this to mean VS will partially work, VS Code will work fully. As a VS Code user, I am fine with this 😄

Copy link
Member

Choose a reason for hiding this comment

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

VS will work the way it always was - even today if you set PATH and DOTNET_ROOT which is what the various scripts in repos do, before starting VS, VS will still run the .NET Framework version of MSBuild it ships with for design builds and the other one for build. But it should still use hostfxr to find the SDK to use for the given app. So this change should make it unnecessary to use the special scripts - which is probably 80% of the value of this feature :-)
That is the one scenario we definitely need to test when developing this feature.

Copy link
Member

Choose a reason for hiding this comment

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

...including the out-of-proc scenarios (e.g., the Windows Forms designers). /cc: @merriemcgaw

Copy link
Member

Choose a reason for hiding this comment

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

Yes, "the way it always has" is key here. All of the friction with a VS/SDK mismatch that you can get with this proposal you can get today with environment variables or a standalone SDK install + global.json.

VS will still run the .NET Framework version of MSBuild it ships with for design builds and the other one for build.

VS uses the .NET Framework version of MSBuild it ships with, period, for evaluation and for all builds including design time and F5 builds.

Copy link
Member

Choose a reason for hiding this comment

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

VS uses the .NET Framework version of MSBuild it ships with, period, for evaluation and for all builds including design time and F5 builds.

Thanks for the explanation!

[This is a proposal][designs-other] similar in nature to this one. There are a
few differences:

1. This proposal is more configurable and supports all standard local
Copy link
Member

Choose a reason for hiding this comment

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

True, although those installations won't explicitly be checked -- the muxer will simply look for the host relative to itself, which will work if the correct entry point is chosen (e.g., the user's PATH is correctly chosen to prefer the installation they want).

@jaredpar
Copy link
Member Author

jaredpar commented Feb 1, 2024

Just want to confirm that our editors/dev tools use this entry point.

@rainersigwald can you confirm this?

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

🎉

proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>
The values in the `paths` property can be a relative path, absolute path or
`$host$`. When a relative path is used it will be resolved relative to the
location of the containing global.json. The value `$host$` is a special value
that represents the machine wide installation path of .NET SDK for the
Copy link
Member

Choose a reason for hiding this comment

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

$host$ would mean "the .NET 8 and earlier behavior", right?

{
"sdk": {
"paths": [ ".dotnet", "$host" ],
"errorMessage": "The .NET SDK could not be found, please run ./install.sh."
Copy link
Member

Choose a reason for hiding this comment

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

Go explicit with resolutionFailureMessage, maybe? Not a strong opinion, errorMessage is ok by me.

hence discovery stops there.

This design requires us to only change the host resolver. That means other
tooling like Visual Studio, VS Code, MSBuild, etc ... would transparently
Copy link
Member

Choose a reason for hiding this comment

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

MSBuildLocator using hostfxr will give this behavior for VS Code/DevKit scenarios.

For VS proper it's more complicated: VS will only ever use the copy of MSBuild that it distributes, but the .NET SDK is located by calling hostfxr, so this proposal should work fine there too--it's just a nicer way to get the same mix of components you can get today (with a global.json + global SDK install, or setting environment variables to tell the resolver to use a private SDK).

MSBuild.exe is the same as VS in this respect (you pick an MSBuild.exe to run, then it loads its own copies of things, but calls the .NET SDK resolver to find the SDK).

There are surely some build-ish tools that exist that don't call hostfxr (forks from older Locators or reimplementations), but I can't see this making anything worse for them, and I don't know of any offhand.

graph LR

subgraph entrypoints
    MSBuild.exe[<pre>MSBuild.exe</pre>]
    VS[Visual Studio]
    DevKit
    cli[<pre>dotnet</pre> CLI]
    rando[other apps]
end

hostfxr

sdk[.NET SDK Resolver]

VS --> sdk

MSBuild.exe --> msbenv[/<pre>MSBuildSDKsPath</pre> env var/]
MSBuild.exe --> sdk

sdk --> hostfxr
sdk --> env[/<pre>DOTNET_MSBUILD_SDK_RESOLVER_SDKS_DIR</pre> env var/]

cli --> hostfxr

DevKit --> MSBuildLocator --> hostfxr

rando --> MSBuildLocator

rando --> oldLocator[MSBuildLocator vOld] --> info[<pre>dotnet --info</pre>] --> hostfxr

rando --> info
Loading

This proposal is designed at giving global.json more control over how SDKs are found. If the global.json asked for a specific path to be considered and it has a matching SDK but a different SDK was chosen, that seems counter intuitive. Even in the case where the chosen SDK was _better_. This is a motivating scenario for CI where certainty around SDK is often more desirable than _better_. This is why the host discovery stops at first match vs. looking at all location and choosing the best match.

Best match is a valid approach though. Can certainly see the argument for some customers wanting that. Feel like it cuts against the proposal a bit because it devalues `additionalPaths` a bit. If the resolver is switched to best match then feel like the need for `additionalPathsOnly` is much stronger. There would certainly be a customer segment that wanted to isolate from machine state in that case.

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 like to see the "doesn't affect dotnet exec app.dll" part called out explicitly. Naively I would have expected it to, though I think it's ok that it doesn't.

Comment on lines +42 to +43
default. Developers must take additional steps like manipulating `%PATH%` before
launching these editors. That reduces the usefulness of items like the quick
Copy link
Member

Choose a reason for hiding this comment

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

Does manipulating PATH actually work? I thought you had to set the special SDK resolver env vars that then bypass hostfxr.

proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
proposed/local-sdk-global-json.md Outdated Show resolved Hide resolved
This proposal is designed at giving global.json more control over how SDKs are found. If the global.json asked for a specific path to be considered and it has a matching SDK but a different SDK was chosen, that seems counter intuitive. Even in the case where the chosen SDK was _better_. This is a motivating scenario for CI where certainty around SDK is often more desirable than _better_. This is why the host discovery stops at first match vs. looking at all location and choosing the best match.

Best match is a valid approach though. Can certainly see the argument for some customers wanting that. Feel like it cuts against the proposal a bit because it devalues `additionalPaths` a bit. If the resolver is switched to best match then feel like the need for `additionalPathsOnly` is much stronger. There would certainly be a customer segment that wanted to isolate from machine state in that case.

Copy link
Member

@elinor-fung elinor-fung Feb 2, 2024

Choose a reason for hiding this comment

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

This change will affect SDK resolution only, not runtime resolution.

I think this is they key part. But with potentially confusing aspects:

  1. In order to run an SDK command, the host clearly needs to load some runtime. For this case, we should look for the runtime based on where the SDK was found. So in a sense it does affect runtime resolution, but only for running the SDK command, not for 'normal' app execution.
  2. dotnet exec looks like an SDK command, but is not. It just runs the app and the SDK is not involved.
  3. dotnet run is an SDK command that also runs the app. The SDK command launches the app, setting the DOTNET_ROOT environment variable based on the running dotnet.
  4. Other SDK commands set DOTNET_HOST_PATH based on the running dotnet. Tools running as part of an SDK command look at its value.

For 3 and 4 in their current state, it would mean that the SDK command could be from some local deployment relative to global.json, but the command would then set variables such that child processes resolve the runtime based on the running dotnet, not where the SDK is running from. This may be odd? I can see the expectation that anything started due to one SDK command would be based on where that SDK was resolved. However, that would also mean dotnet run vs dotnet app.dll would be different in terms of where the runtime could come from.

jaredpar and others added 2 commits February 5, 2024 10:26
Co-authored-by: Elinor Fung <elfung@microsoft.com>
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

the app directly using the runtime installed with `dotnet`.

It is reasonable for complex builds to build and use small tools. For example
building a tool for linting the build, running complex validation, etc ... To
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
building a tool for linting the build, running complex validation, etc ... To
building a tool for linting the build, running complex validation, etc. To

@simonferquel
Copy link

Just asking for confirmation:
if I have a custom MSBuild SDK (that extends the .net SDK) will I be able to use those paths instead of relying on nuget to resolve it ? - the spec only mentions the .net SDK, not custom ones.

@baronfel
Copy link
Member

baronfel commented May 7, 2024

@simonferquel correct - this is only about automatically using a locally-installed .NET SDK. The MSBuild SDKs feature (which has an unfortunately-similar name) is entirely different.

This proposal is designed at giving global.json more control over how SDKs are found. If the global.json asked for a specific path to be considered and it has a matching SDK but a different SDK was chosen, that seems counter intuitive. Even in the case where the chosen SDK was _better_. This is a motivating scenario for CI where certainty around SDK is often more desirable than _better_. This is why the host discovery stops at first match vs. looking at all location and choosing the best match.

Best match is a valid approach though. Can certainly see the argument for some customers wanting that. Feel like it cuts against the proposal a bit because it devalues `additionalPaths` a bit. If the resolver is switched to best match then feel like the need for `additionalPathsOnly` is much stronger. There would certainly be a customer segment that wanted to isolate from machine state in that case.

Copy link
Member

Choose a reason for hiding this comment

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

What will the value of %DOTNET_HOST_PATH% be inside of msbuild here? That needs to be c:\source\.dotnet\dotnet.exe otherwise this proposal falls apart.

As part of the implementation of this proposal, I think it should become c:\source\.dotnet\dotnet.exe. I think the SDK commands that currently set some variable to 'the running dotnet' intend that value to represent 'the dotnet for the running SDK`, so they would need to be updated.

@jaredpar jaredpar merged commit 54ddb59 into main May 10, 2024
3 checks passed
@jaredpar jaredpar deleted the dev/jaredpar/local branch May 10, 2024 16:48
@MartyIX
Copy link

MartyIX commented Jun 12, 2024

Do I understand correctly that this would allow me to: Have globally installed .NET 8 but test with .NET 9 that would be stored in a folder (not installed)?

I always want to test my .NET MAUI app on .NET 9 but it's not practical to install it and uninstall it every time as I want to use .NET 8 as my default .NET.

@agocke
Copy link
Member

agocke commented Jun 12, 2024

@MartyIX you can do this today by installing .NET 9, but using global.json to specify a .NET 8 SDK version. That will cause your build to always use .NET 8. When you want to test on .NET 9 you can temporarily bump that version to .NET 9, then revert the change when you’re done.

@MartyIX
Copy link

MartyIX commented Jun 12, 2024

@MartyIX you can do this today by installing .NET 9, but using global.json to specify a .NET 8 SDK version. That will cause your build to always use .NET 8. When you want to test on .NET 9 you can temporarily bump that version to .NET 9, then revert the change when you’re done.

I see. I would define global.json in my D:\Projects folder to be .NET 8 and all projects in subdirectories would use .NET 8 by default. Then if needed, I would bump version in my D:\Project\MyMauiProject\global.json to .NET 9 to test it.

Right?

@xoofx
Copy link
Member

xoofx commented Nov 27, 2024

@agocke do you think this work could be prioritized for .NET 10? We are currently blocked by this and it is causing internally lots of confusion and pain when building with a custom SDK that is not same used by the IDE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.