-
Notifications
You must be signed in to change notification settings - Fork 1.2k
dnup code review #50988
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
Open
dsplaisted
wants to merge
145
commits into
main
Choose a base branch
from
dnup
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
dnup code review #50988
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Copilot prompt: Implement the GetConfiguredInstallType method in #file:'DotnetInstaler.cs' . This method should look in the PATH and resolve "dotnet" or "dotnet.exe", depending on the current OS. If it is not found, the return value should be SdkInstallType.None. If it is found in Program Files, the install type should be SdkInstallType.Admin. If it is found in another folder, the install type should be SdkInsntallType.User. However, the method should also check the value of DOTNET_ROOT environment variable. For a user install, DOTNET_ROOT should be set to the folder where the dotnet executable is found. For an admin install, DOTNET_ROOT should not be set, but if it is set to the install path under program files, that is OK. If the DOTNET_ROOT value doesn't match the install location, the method should return SdkInstallType.Inconsistent.
Copilot prompt: In #method:'Microsoft.DotNet.Tools.Bootstrapper.DotnetInstaler.GetConfiguredInstallType':301-2793 , use #method:'Microsoft.DotNet.Cli.Utils.EnvironmentProvider.GetCommandPath':2399-2843 to search for dotnet on the path.
Copilot prompts: Create a class to represent the contents of a global.json file, and create a corresponding JsonSerializerContext type. Can you refer to https://learn.microsoft.com/en-us/dotnet/core/tools/global-json to update the schema you are using for the global.json file? AllowPrerelease should be a booleann, and instead of Path there should be a Paths property which is an array of strings.
Copilot prompts: Implement #method:'Microsoft.DotNet.Tools.Bootstrapper.DotnetInstaller.GetDefaultDotnetInstallPath':2814-2986 . It look for a global.json file starting in the specified directory and walking up the directory chain. If it finds one, it should deserialize its contents using System.Text.Json APIs and #class:'Microsoft.DotNet.Tools.Bootstrapper.GlobalJsonContentsJsonContext':413-650 , and use that for the return value. Sorry, I meant to ask you to implement #method:'Microsoft.DotNet.Tools.Bootstrapper.DotnetInstaller.GetGlobalJsonInfo':4307-4411 instead of #method:'Microsoft.DotNet.Tools.Bootstrapper.DotnetInstaller.GetDefaultDotnetInstallPath':2839-2947 . Can you revert your changes and then implement #method:'Microsoft.DotNet.Tools.Bootstrapper.DotnetInstaller.GetGlobalJsonInfo':4307-4411 using the instructions I previously provided?
Copilot prompts: Can you remove the try catch block around reading the global.json file in #method:'Microsoft.DotNet.Tools.Bootstrapper.DotnetInstaller.GetGlobalJsonInfo':3019-4216 ? Also remove the convenience properties you added to GlobalJsonInfo for now. OK, go ahead and add those convencience properties back.
Copilot prompt: Implement the #method:'Microsoft.DotNet.Tools.Bootstrapper.DotnetInstaller.ConfigureInstallType':4479-4606 method. If the install type is user, remove any other folder with dotnet in it from the PATH and add the dotnetRoot to the PATH. Also set the DOTNET_ROOT environment variable to dotnetRoot. If the install type is Admin, unset DOTNET_ROOT, and add dotnetRoot to the path (removing any other dotnet folder from the path). If the install type is None, unset DOTNET_ROOT and remove any dotnet folder from the path. For any other install type, throw an ArgumentException.
Copilot prompt: In #method:'Microsoft.DotNet.Tools.Bootstrapper.DotnetInstaller.ConfigureInstallType':4481-6459 what I meant by removing a folder from the path if it has dotnet in it was that you should check the contents of each folder and if it is a dotnet installation folder then it should be removed from the path. A simple way to check if it's a dotnet installation folder is if it has a dotnet executable in it.
I renamed the InstallType as the Runtime installs would similarly hold the same properties.
im not sure if I really like this - chose a fortress to represent 'guarding' the code by running tests.... is it really helpful? It might be if / when we have other test steps to differentiate and find the dnup test checks.
there is no concept of a global\ mutex prefix on unix which could be incorrect. On windows, this makes it so the mutex is not for the local user session but instead across all users. I'm not sure if this is needed but since we edit the system path, it might be. I will have to think about whether there will be issues with a higher-level admin user that has held onto a mutex and got stuck, and whether a lower level user could undo that.
10 seconds may fail - concurrently the tests could take a while. lets not fail the test because other tests are running.
OpenExisting may run into challenges with security policies changing - let's not try to navigate that when we can track it ourselves.
this would likely cause concurrency issues in tests if we just rely on the environment variable.
tests total runtime is 200 s on my machine - let's see if this helps. this would ideally be configurable because most users probably won't run multiple installs, though also they may have a much slower machine where this would cause concurrent requests to fail.
may refactor this later...
we probably shouldnt hard code debug amongst other things. Im sure theres a better approach but just want to get it green first.
This implements validation of installs using hostfxr apis to ensure the install actually works and not just that the manifest is tracking the installs correctly in e2e tests. It also adds a test to show that we can do multiple installs in the same directory without failing. It also improves the existing test logic to not assume a hard-coded debug value for the dnup process.
This reverts commit 0fc7172.
we can chat if we want this to be internal - though it might be helpful for consumers to be able to marshal the architecture from the runtime into our architecture type that we use in our install objects, considering it's part of dotnet install root, which is also public
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We're not planning to merge this to main yet, but we want to start reviewing the code.
Context: dotnet/designs#339