dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.66k stars 1.06k forks source link

Developing a shared framework requires admin rights #12357

Open natemcmaster opened 5 years ago

natemcmaster commented 5 years ago

We develop the Microsoft.AspNetCore.App shared framework in https://github.com/aspnet/AspNetCore, and it is currently difficult for our contributors to actually run tests against the shared framework. Corehost only resolves shared frameworks from $DOTNET_ROOT/shared/Microsoft.AspNetCore.App/ or the global location, which means there are basically two options for our contributors:

Request Is there way we could have corehost resolve shared frameworks from a local directory? For example, could we add a shared framework probing path in runtimeconfig.dev.json?

Ideally, I'd love for Visual Studio to pick up the local builds of our shared framework without needing administrative access to a machine, and overriding a handful of environment variables.

cc @davidfowl

vitek-karas commented 5 years ago

There's dotnet/cli#10100 which will add the ability to install SDK (and frameworks) into a user specified location which will effectively replace the current ProgramFiles one. The location will be in registry (probably machine wide), so the first time setup requires admin rights, but after that if the location is user accessible, normal user can write there as well.

This would not solve the problem of "polluting" the global location with developer bits though.

I think that right now the best way to do this is through the DOTNET_ROOT variable. It makes it explicit in the sense that if I want to run against the developer bits, I have to set the env variable.

Alternatively you can run the dotnet.exe from the desired directory and it will look in its subtree (that's what our repos do when building using the toolchain). This probably won't help with the VS case though.

As for having a probing path in runtimeconfig.dev.json - this is a bit problematic. For a 100% correct solution the hostfxr lookup itself would need to be able to follow this, which would require dotnet.exe to parse the runtime config. That's a rather large change.

natemcmaster commented 5 years ago

As for having a probing path in runtimeconfig.dev.json - this is a bit problematic.

Just trying to understand better... Don't we already have code which parses the app's runtimeconfig.json to know which shared framework name and version we are looking for? I would have thought adding a probing path could just be one more input to the 'find a framework' code. I was considering sending a PR for this since it's important to my team, and was looking at putting code into this method https://github.com/dotnet/core-setup/blob/master/src/corehost/cli/runtime_config.cpp#L175. I didn't actually write it yet though. Is there sometime I'm missing about the order in which json parsing and fx lookup happens?

vitek-karas commented 5 years ago

That would work in most cases. The problem is not with the framework but with how dotnet.exe finds hostfxr.dll - this happens before any framework resolution is attempted.

Basically the problem is that if your global location only has let's say 2.2 as the latest installed, its hostfxr is also 2.2. So running the global dotnet.exe (without additional changes) would find the 2.2 hostfxr which would then parse the runtimeconfig.json and in the end try to load a 3.0 (for example) hostpolicy. While we try to keep things backward and forward compatible, it's not really a supported scenario to use lower version hostfxr in combination with higher version hostpolicy. The general design is that we always use the highest possible version of hostfxr (which can then talk to any equal or lower version of hostpolicy). But with the above change, we would not find the highest version.

I'm trying to understand why the DOTNET_ROOT env variable is not good enough. For command line tooling it should be fine. The only scenario where it is somewhat cumbersome is VS, where it would require to start VS through some script which sets the variable first and then runs VS. Are there are concerns?

natemcmaster commented 5 years ago

I understand your concern now. I think this concern mostly applies to Microsoft.NETCore.App. Other shared frameworks shouldn't be implementing their own hostfxr or policies, so I would be okay if an option like "additionalFrameworkProbingPaths" was supported but only applied to additional frameworks, not the root framework.

The primary reason I think DOTNET_ROOT is insufficient is that it's not something we can set in a .sln file or global.json. Despite all attempts to document this clearly, first thing contributors typically do with our source code is open a Visual Studio solution file. Mayhem follows. I regularly field questions from devs struggling to get started with our code. The next thing I'm considering is a dancing bear gif in our README.md holding a sign that says "for real, you should actually read me".

An alternative worth considering is the ability to specifying DOTNET_ROOT in global.json, dotnet.json, .config/dotnet.json...some kind of config file we can check in, don't care what it looks like. I figured that would required more coordination with VS, Omnisharp, and VS4Mac teams, which is why I didn't jump that that idea at first.

cc @dsplaisted

vitek-karas commented 5 years ago

I personally think that this is something which VS should have an option for, ideally as an MSBuild property (but then it's a chicken and egg problem I guess). The fact the VS forces the use of the same dotnet on all projects in it feels wrong.

I think that if the runtime would be to support this, then the right place would be the global.json as it already handles a similar concern for SDK. Unfortunately implementing this could be relatively ugly. I would have to look into this some more.

natemcmaster commented 5 years ago

Update to this thread - I'm going to workaround this limitation. I would still love to see a proper fix because what I'm doing feels just plain wrong. But, I don't have other options.

natemcmaster commented 5 years ago

I’m still running into problems getting the aspnet/AspNetCore repo to build and test the Microsoft.AspNetCore.App shared framework. The workarounds I've tried still run into this problem: using Visual Studio plus a custom shared framework is fragile.

To solve these problems, I’d like to make a pull-request to core-setup to add the option I think I need to corehost. Is this something you would accept?

Proposal

Add support to the host for listing additional folders that might contain a shared framework.

Usage

In runtimeconfig.json (or runtimeconfig.dev.json), support parsing a new option, “additionalFrameworkProbingPaths”, which an be used to list new folders that contain shared frameworks. For instance, in the aspnet/AspNetCore repo where we develop the Microsoft.AspNetCore.App shared framework, I would change the repo to generate a runtimeconfig.dev.json that looks like this:

{
  "runtimeOptions": {
    "additionalFrameworkProbingPaths": [
      "C:\\src\\aspnet\\AspNetCore\\artifacts\\bin\\Debug\\shared\\"
    ]
  }
}

This allows me to create a folder like "C:\src\aspnet\AspNetCore\artifacts\bin\Debug\shared\Microsoft.AspNetCore.App\9.9.9-dev\", and keep tools like dotnet test and Visual Studio working correctly.

Behavior

Folders listed in additionalFrameworkProbingPaths are treated as additional folders in the already-existing multi-level shared framework lookup code. These folders are simply additional locations to be searched for shared frameworks. The same rollforward versioning policies apply to these shared frameworks.

cc @vitek-karas @Pilchie @dagood

Pilchie commented 5 years ago

Also tagging @livarcocc

vitek-karas commented 5 years ago

I think this sounds reasonable. To make the behavior predictable I would add:

ViktorHofer commented 5 years ago

@natemcmaster I absolutely love your idea of fixing this common scenario. Every repository that deals with composing its own framework hit this. Just one ask, why not encode that in the global.json instead?

natemcmaster commented 5 years ago

Discussed a little in chat, but posting here for posterity. The problem I want to solve is that the AspNetCore repo currently needs to write its shared framework into the installation of dotnet in order for the host to pick it up. Because of the way MSBuild does incrementalism, switching between release and debug in VS or on command line may result in tests or build not actually using up-to-date bits. Global.json doesn't work because we don't have a way to flow the VS configuration setting into the host. Runtimeconfig.dev.json seems like a better solution because build output is already config-specific.

ViktorHofer commented 5 years ago

OK the configuration specific point makes sense.

vitek-karas commented 5 years ago

There's one other problem with global.json. Currently the host only reads this file when it invokes SDK commands. Running apps will never look at that file. Note that starting to do so would introduce several issues:

I do agree that it's not as "nice" since we can't easily do it globally for all projects in a repo. That said, we should be able to modify the build system in the repo such that it produces the right .runtimeconfig.json by default for all apps in it. Or in fact probably the .runtimeconfig.dev.json as this setting is more like the other things we write to .runtimeconfig.dev.json.

ViktorHofer commented 5 years ago

Given the argument that we want to have different shared framework folders for different configurations I'm in favor of runtimeconfig.json now.

That said, we should be able to modify the build system in the repo such that it produces the right .runtimeconfig.json by default for all apps in it.

That should easily doable with a template.runtimeconfig.json. If you think about additional support, sure.

Or in fact probably the .runtimeconfig.dev.json as this setting is more like the other things we write to .runtimeconfig.dev.json.

Agreed.

ViktorHofer commented 5 years ago

Spoke with @ericstj about this and he mentioned that the runtimeconfig.json file might not be the best place for this setting as that configuration file should be portable. The runtimeconfig.dev.json seems most appropriate here.

ViktorHofer commented 5 years ago

@vitek-karas is there support for modifying the runtimeconfig.dev.json with a template similar to what is possible with a template.runtimeconfig.json file?

vitek-karas commented 5 years ago

@nguerrera would know about the runtimeconfig.dev.json support in the SDK. That said, maybe in 3.0 we don't actually need the SDK to produce that file at all - in 3.0 build will include all dependencies in the app, so the probing paths which are normally written to the runtimeconfig.dev.json are not actually useful for anything. So we might be able to simply overwrite the file with our own.

nguerrera commented 5 years ago

Yes, we're discussing removing .dev.json from default output. You could overwrite it. There's not a template mechanism for it.

vitek-karas commented 5 years ago

/cc @blowdart @jkotas @ericstj - we are planning to introduce this setting soon... if you have concerns please let us know.

Short version: Adding additionalFrameworkProbingPaths property into .runtimeconfig.json and .runtimeconfig.dev.json (we would only recommend using it in the .dev file as it is machine specific). The paths specified would be added to the end of the list of paths considered when searching for frameworks - currently that list contains (by default) the location from where the dotnet.exe was executed, the globally registered location (on Windows that's in registry) and the global default location (on Windows that's program files).

jkotas commented 5 years ago

Update to this thread - I'm going to workaround this limitation. I would still love to see a proper fix because what I'm doing feels just plain wrong.

@natemcmaster What is your current workaround? Is the proposed fix going to move the needle enough for you?

It does not really solve the crux of the problem that you cannot point Visual Studio to the local runtime +SDK copy.

ericstj commented 5 years ago

What types of paths are allowed in there? Relative, relative with parents, absolute, absolute to different drives, unc, environment variables? You probably need to consider these to get an appropriate threat model while still satisfying customer scenarios. It was always frustrating to me in desktop the limitations of probingPaths in the config file (relative only) and codebase hints (absolute only, with very limited environment variable support). It made things like sharing code between two applications which were deployed to different locations with installers very hard without custom actions.

vitek-karas commented 5 years ago

@ericstj I expect to do the same thing we do for the existing additionalProbingPaths. Currently the code gets the value and calls realpath on it (or GetFullPathName on Windows). If I read the docs correctly this does not expand env. variables, but it does support absolute and relative paths (relative to current directory).

From a security perspective this is no different than the additionalProbingPaths, it's just used a little earlier. The .runtimeconfig.json or .runtimeconfig.dev.json is automatically picked up from the same folder as the app itself, so it's considered part of the app. (There is a command line option to specify a different file, but that is an explicit action with clear implications).

ericstj commented 5 years ago

I see, read through the rest of this thread for more context. It's still unclear to me if this will solve the larger problem. You still end up running on the machine-wide SDK. IMHO this contributes to the original problem, just in a different way.

overriding a handful of environment variables.

Now there is another option for something that can be overridden. Imagine all the contexts you might want to use the local CLI, are you sure you can hook all their runtimeconfigs?

I think we need a holistic feature for repo-acquired CLI including shared-frameworks + SDK. Until we have something like that I don't think we should be adding more knobs.

Here's a suggestion for a workaround for this scenario that doesn't require a new feature, and will hopefully steer people towards the path you want them to go down while also supporting more of the cases:

<Project InitialTargets="ValidateVSEnvironment">
  <Target Name="ValidateVSEnvironment"
                Condition="'$(BuildingInsideVisualStudio)' == 'TRUE'">
    <Error Condition="'$(DOTNET_ROOT)' == ''" Error="This repository requires a locally acquired DotNET but VS was launched without setting this.  Please see Readme.md for more details" />
  </Target>
</Target>

I bet you could generalize this enough to put it in arcade.

natemcmaster commented 5 years ago

What is your current workaround? Is the proposed fix going to move the needle enough for you?

The workaround I created requires:

  1. Devs must first run a restore.cmd/sh script which will download dotnet into $repoRoot/. dotnet
  2. Devs must set DOTNET_ROOT, DOTNET_ROOT(X86), DOTNET_MULTILEVEL_LOOKUP, and PATH env vars. I added startvs.cmd to open Visual Studio, and run activate.ps1 to set shell env vars so devs can use "dotnet build/test" or launch VS Code
  3. When we build the aspnet shared framework, we copy the local build into $repoRoot/.dotnet/shared.

Downsides:

This fix would help move the needle because I could have test projects generate a runtime config.dev.json file that probes for additional shared frameworks in $repoRoot/artifacts/$config/. As long as the global version of dotnet installed is recent enough, devs can probably go back to just opening Visual Studio and using the dotnet CLI normally.

jkotas commented 5 years ago

I think we need a holistic feature for repo-acquired CLI including shared-frameworks + SDK. Until we have something like that I don't think we should be adding more knobs.

+1

natemcmaster commented 5 years ago

I don't think we should block this on a desire to design a much bigger feature. We've had lots of conversations for years about how we might do a holistic feature for acquiring runtimes and CLIs, and even built prototypes like dnvm, but never had management buy-in on doing this for real. I would be in favor of such a feature if there were resources and a timeline for it, but delaying a small config option like this (which would immediately benefit aspnetcore) for some hypothetical, yet-to-be-designed feature is, IMO, a mistake.

ericstj commented 5 years ago

Can you use the workaround I suggested above https://github.com/dotnet/core-setup/issues/4809#issuecomment-503222883 that directs your devs to launch VS with your startvs.cmd script if you find they didn't do so on project load? That way you steer people into building the "same" way from VS as the commandline without requiring them to read the docs.

natemcmaster commented 5 years ago

Sure, we could do that, but that suggestion solves another problem, not this one.

ericstj commented 5 years ago

Can you elaborate on how it doesn't solve this problem? You mentioned that you can set environment variables in order to get VS to use your private shared framework, but you cannot enforce that folks use those. I suggested a way to enforce that they use them and get a predictable error when they don't. It also ensures some determinism around building: you can ensure that folks use the exact same version you're acquiring in your build rather than floating. Maybe I'm missing something from your scenario, if so can you share?

I'm not a fan of adding a hook that allows for a persistent asset that points to the shared framework. I feel that this is machine / environment state that shouldn't be persisted in build output. I honestly think the error and script is a better option because it opts for a more deterministic solution, and has the characteristic of applying to all processes since the environment is inherited.

davidfowl commented 5 years ago

Can you use the workaround I suggested above dotnet/core-setup#4809 (comment) that directs your devs to launch VS with your startvs.cmd script if you find they didn't do so on project load? That way you steer people into building the "same" way from VS as the commandline without requiring them to read the docs.

Your devs = any existing contributor or potential future contributor

natemcmaster commented 5 years ago

Can you elaborate on how it doesn't solve this problem?

Your suggestion was to emit an error message if DOTNET_ROOT is empty. It helps people who forget to use startvs.cmd, but the fact that startvs.cmd is required in the first place is the problem. As described above, corehost will not resolve shared frameworks from any location except $currentDotnetInstall/shared with a potential fallback to $globalDotNetInstall/dotnet. Requiring DOTNET_ROOT to be set doesn't change how the VS loads the SDK (you also have to put dotnet.exe first on PATH), help the host find a new shared framework (we still have to copy into $dotnet/shared/Microsoft.AspNetCore.App), help with switching between Debug and Release in VS (we don't have a workaround for this yet).

What I am asking for:

Runtimeconfig.dev.json is the best option I'm aware of. If you know of alternatives, please share.

natemcmaster commented 5 years ago

I'm not a fan of adding a hook that allows for a persistent asset that points to the shared framework. I feel that this is machine / environment state that shouldn't be persisted in build output.

Runtimeconfig.dev.json already exists and already does this. If your proposal is that we eliminate this file, then where do the existing config options in this file, like additionalProbingPaths? I know .NET Core 3 mostly removes the need for that config by copying-local all dependencies, but the whole point of the shared framework is that it's not copied local.

natemcmaster commented 5 years ago

I honestly think the error and script is a better option because it opts for a more deterministic solution, and has the characteristic of applying to all processes since the environment is inherited.

How is my proposal non determistic? How my proposal not going to work with child processes? I don't see either of those as problems. Please elaborate.

natemcmaster commented 5 years ago

Final comment, I AM NOT trying to solve the problems with .NET Core SDK acquisition and VS and global.json. I am just trying get the runtime which VS and dotnet-cli will use to include a shared framework that is locally built.

ericstj commented 5 years ago

Your devs = any existing contributor or potential future contributor

Completely understood. I really want a solution that gives everyone a predictable experience where building and running in VS is as close to building from command line / CI as possible. I used ‘your devs’ in reference to Nate’s mention of ‘our contributors’. Sorry if my terminology confused or came off wrong. I didn’t mean to.

So my point around child processes is that you can set all your environment (including DOTNET_ROOT) to impact the entire VS process and child processes it launches during the build. Contrast that to runtimeconfig writing which will only impact the things you build. So you end up running anything in your build / test processes on a different shared framework than you use for a command line build (which uses locally acquired CLI).

The non-determinism I was referring to was that you float to the SDK people have installed at the time they run build, and your apps can still fall back to run on machine wide shared framework if the path isn’t found. With an explicit DOTNET_ROOT along with other env vars you mentioned you can lock things down to just the dependencies you stage for your build.

The difference between additionalProbingPaths and this is that the former will likely be used more with relative paths which are likely portable, whereas this new feature is more likely to be used with absolute paths which are not portable. That said one could draw an analogy to absolute path to PDB in PE image, so it’s not without precedent.

I think I missed the distinction you have here between locally built vs locally acquired. You’re talking about the shared framework you build, that itself needs to load the locally acquired base shared framework. I had assumed you’d stage an entire shared framework set in a single root, but now I can see why you would not want to do that because it would mix the build inputs with build outputs.

I can see the value here. You essentially need two DOTNET_ROOTS. I would hope that you’d make sure your tests run on both the locally built shared framework and the locally acquired shared framework rather than relying on devs to install either. I’d also hope that you steer folks to use the script so that they don’t need to install a newer machine-wide SDK.

Did you consider multiple environment variables, or maybe permitting a list in DOTNET_ROOT? Presumably you’d know the path up front and could set this. I guess you might still hit the debug/release problem. Or even architecture problem. Reminds me of COMPLUS_InstallRoot. I’m coming around to this... I still don’t like the fact that paths aren’t portable but are persisted in the output. I’m not seeing how this would work with Helix, I guess for that we’d still use a fully staged shared framework set and ignore this setting. I guess I am ok with this proposal if the host and runtime folks are and we can’t come up with an alternative using solely env vars (to avoid leaking into output). I ask that if you do go this route you take care to ensure that both aspnet and the base shared framework are correctly located and never loaded from machine-wide.

natemcmaster commented 5 years ago

Yeah, locally built vs acquired is a key distinction. I considered env vars, but still favor the runtimeconfig.dev.json file because we could vary the probing paths based on Configuration. I’m not sure how we would do that with env vars unless we also teach the host about Debug vs. Release (which doesn’t sound like a good idea). I would expect the contents of a local build on a test project in AspNetCore to have a runtimeconfig.dev.json file such as

{
  "runtimeOptions": {
    "additionalFrameworkProbingPaths": [
      "C:\\src\\aspnet\\AspNetCore\\artifacts\\bin\\Debug\\shared\\"
    ]
  }
}

I’m not seeing how this would work with Helix, I guess for that we’d still use a fully staged shared framework set and ignore this setting

We could ignore or generate the runtimeconfig.dev.json file on the helix machine. AspNetCore is currently using the publish target to generate helix test payload, so runtimeconfig.dev.json file is already excluded from the helix payload.

The non-determinism I was referring to was that you float to the SDK people have installed at the time they run build, and your apps can still fall back to run on machine wide shared framework if the path isn’t found.

Thanks for clarifying. Floating the SDK for local development is not something I’m too concerned about. Aspnet repos almost always need some preview SDK as a minimum version, and newer ones typically still build the code in the same way. Runtime floating, on the other hand, is something we attempt to guard against in our tests by setting rollforward to “disable” and DOTNET_MULTILEVEL_LOOKUP=0. For the sanity of our contributors running tests, it’s important we are running tests against a deterministic set of dependencies.

vitek-karas commented 5 years ago

Just a couple of observations:

Going forward there's also the scenario of 3rd party shared frameworks. Those would likely not live in the same location as .NET Core runtime. So we would need a similar setting for those - but this time it would be a machine wide version of the setting (either registry or something similar). Not counting that we would need a story for how such framework is developed - which is basically the same thing as ASP.NET issues today.

I can imagine that we support multiple ways of setting this new property in the future - command line, env. variable, .runtimeconfig and potentially some config-file/registry as well.

jkotas commented 5 years ago

the scenario of 3rd party shared frameworks multiple ways of setting this new property in the future - command line, env. variable, .runtimeconfig and potentially some config-file/registry as well

Agree. It would be nice to get the whole thing figured out, and not try to implement a small part of it right before forking a release.

basically VS needs an isolation layer which separates VS from the build/test processes

I expect that once the core VS starts depending on .NET Core, they will ship with their own app-local copy of the runtime and their own hosting that works well with the VS internal package manager (Willows) and gives them full control over their destiny.

davidfowl commented 5 years ago

I expect that once the core VS starts depending on .NET Core, they will ship with their own app-local copy of the runtime and their own hosting that works well with the VS internal package manager (Willows) and gives them full control over their destiny.

That's not how we'd want to solve this issue though. The other angle to this experience would to have a way to select the relevant .NET runtime for the tools. Today that's drive via the host interfaces and global.json but you could imagine VS and other tools to explicitly use a specific dotnet.

The only problem with this approach is that it's inconsistent between various tools.

ViktorHofer commented 5 years ago

Agree. It would be nice to get the whole thing figured out, and not try to implement a small part of it right before forking a release.

Absolutely agree. Given the benefits of such a solution (ie this is the only hard blocker for corefx to enable VS Test Explorer) I think we should still strongly consider implementing this as soon as we forked-off for 3.0.

ViktorHofer commented 5 years ago

I think it's the right time now to continue our discussion here. We still need a way to associate a self-built shared framework with a locally acquired dotnet hive. The proposal was to add an entry in runtimeconfig.dev.json for the local dev experience, in combination with DOTNET_MULTILEVEL_LOOKUP=0.

On the helix side it would be nice to be able to use this feature as well but this would require relative paths / environment variables being handled by the runtimeconfig.dev.json entry correctly.

vitek-karas commented 5 years ago

Related to this discussion is dotnet/core-setup#7851 - although it's env. variable based, so probably not going to solve the issues described here.

agawasare commented 4 years ago

Please see https://dotnetresolutions.wordpress.com/2020/05/21/making-net-core-binaries-work-with-visual-studio-code