[Bootstrapper] Fetch information from dotnet feeds #391
Conversation
…ind/cli-lab into bootstr-edvilme-commandline
edvilme
force-pushed
the
bootstrapper-dotnet-feeds
branch
from
4355ff9 to
6a89e20
Compare
| private bool _allowPreviews = parseResult.ValueForOption(SearchCommandParser.AllowPreviews); | ||
| public override int Execute() | ||
| { | ||
| List<Product> productCollection = [.. ProductCollection.GetAsync().Result]; |
There was a problem hiding this comment.
The releases.json only changes about once a month. You should consider caching the file on disk and only update it if there's a new version available.
There was a problem hiding this comment.
We should be using the same etag-based cache invalidation system that @nagilson has for the VSCode extension. ETags are the way to handle cache invalidation of HTTP-delivered resources, and we shouldn't be reinventing the wheel.
There was a problem hiding this comment.
Does https://github.com/dotnet/deployment-tools cache it? I would hope it does so, but at a glance it looks like it does not. I tis what is interfacing with the web request caller API. I would also hope that it handles proxies well. As well as timeouts. If it does not, I would almost question why that should not be implemented over there instead of here. Definitely wouldn't block on this for this PR but would create another issue from it.
There was a problem hiding this comment.
From what I've seen, I don't think that it does. I do agree that it would make a lot of sense to implement it there
| // TODO: Replace with the actual installation directory. | ||
| return Path.Combine(directoryPath, ".dotnet.local"); |
There was a problem hiding this comment.
Where should we install to? Should we install to the .dotnet directory?
There was a problem hiding this comment.
Priority #1: I believe we should have a command argument where you can specify the directory
I would personally say that we should not block this PR on this decision, and this logic can be reserved for another PR/issue. I would refer to #354 for the global.json and a new issue for the file location default: #392
Priority #2: Global.Json: Based on https://github.com/dotnet/cli-lab/issues/354, in which we decided to go with the spec here, https://github.com/dotnet/designs/pull/303/files#diff-6327dc0859c24dcc1de13ab88ec74a7b00f7b0d877a12b12d1dd802b2409e2e6R11, it seems this should look at the paths key inside of sdk of global.json. I'm not sure how we prioritize which path to put it in out of the paths in paths. Perhaps the first -- cc @baronfel @agocke?
Priority 3: Default location. Perhaps somewhere in temp/home dir.
There was a problem hiding this comment.
Thank you for writing this up, I'm excited to see a lot of progress here!
In the very early days of a project, it's very hard to say what should/shouldnt go in a separate PR and what the 'bar' should be. Some of what I mention below I do think are things we don't need to block this PR on (I mostly remark when that is the case), though some of them are. I think this is a great start 🔥 and it is normal to have a lot of feedback and things to consider.
There is a balance of 'moving fast' vs carefully considering every possible edge case. Since we are not even a launched product I would say we are on the 'moving fast' side for now, to some extent. I would defer to @baronfel and @marcpopMSFT for a broader priority discussion on some of these items.
| { | ||
| // Get the nearest global.json file. | ||
| JsonElement globalJson = GlobalJsonUtilities.GetNearestGlobalJson(basePath); | ||
| string sdkVersion = globalJson |
There was a problem hiding this comment.
https://learn.microsoft.com/en-us/dotnet/api/system.text.json.jsonelement.getproperty?view=net-9.0 It looks like GetProperty will throw if the property keys are not present. I believe a global json that does not specify an SDK should be valid, same with one that has no 'tools'. I would write a test for both of these scenarios.
There was a problem hiding this comment.
With that being said, the global.json partition can be an entirely separate PR. There is already an issue for that. #354
|
|
||
| public static string GetInstallationDirectoryPath() | ||
| { | ||
| string globalJsonPath = GlobalJsonUtilities.GetNearestGlobalJsonPath(Environment.CurrentDirectory); |
There was a problem hiding this comment.
I think the current behavior of this, is that it will fail if it has no global.json, since this is called first when trying to find the install directory path. I don't think this is the behavior that we want, @baronfel?
There was a problem hiding this comment.
I think we can defer this to #354.
| releaseFile.DownloadAsync(zipPath)?.Wait(); | ||
|
|
||
| // Extract the downloaded file | ||
| ZipFile.ExtractToDirectory(zipPath, Path.ChangeExtension(zipPath, "")); |
There was a problem hiding this comment.
It would be ideal to check whether it has already been extracted to this directory before so we can skip this labor, but iff the extraction resulted in a success and valid installation.
| Console.WriteLine($"\tExtracted {component.Name} to {Path.ChangeExtension(zipPath, "")}"); | ||
|
|
||
| // Delete the downloaded file | ||
| File.Delete(zipPath); |
There was a problem hiding this comment.
We should probably have an issue w.r.t caching this zip file in case someone installs again? Not very high priority and definitely not a blocker.
| releaseFile.DownloadAsync(zipPath)?.Wait(); | ||
|
|
||
| // Extract the downloaded file | ||
| ZipFile.ExtractToDirectory(zipPath, Path.ChangeExtension(zipPath, "")); |
There was a problem hiding this comment.
I see that DownloadAsync verifies the hash. It's a bit awkward though in that it will do a download even if the dest file exists and always replace it. Ideally we could verify the hash of an existing file and re-utilize it, though I know that's not as relevant in the current context, since we delete the file every time.
| releaseFile.DownloadAsync(zipPath)?.Wait(); | ||
|
|
||
| // Extract the downloaded file | ||
| ZipFile.ExtractToDirectory(zipPath, Path.ChangeExtension(zipPath, "")); |
There was a problem hiding this comment.
One thing to consider: What if someone runs multiple bootstrapper commands at the same time? Will the directory extraction clobber over each other? Should there be some sort of lock on this operation that can be shared across processes?
What if someone pkills the bootstrapper as it is in the middle of extracting the directory? Then it will leave the directory in a bad state, I believe. We may want to consider cleaning the directory up beforehand (admittedly, this would be bad if someone incorrectly configured their path and then it wiped something else out) or handling these scenarios better with a different approach.
I also don't know if I would block on this but I do consider it higher priority after this.
| releaseFile.DownloadAsync(zipPath)?.Wait(); | ||
|
|
||
| // Extract the downloaded file | ||
| ZipFile.ExtractToDirectory(zipPath, Path.ChangeExtension(zipPath, "")); |
There was a problem hiding this comment.
It's likely worth doing a dive into each of the possible error cases here to handle them: PathTooLong (does this function respect the OS setting or rely on the default?) -> Directory Not Found - Do we need to create the directory? Unauthorized -> Should we try a fallback directory (probably not for now)
#348
Fetch information from dotnet feeds for search and install command.
Added logic to find global.json file and fetch version information.
Added logic for downloading files.
Search
Added search command to list all available sdk installs.
If no channel is provided, it will list all of them by default.
Install
Added install command to install all assets from a specified version.
If no version is provided, it will look up the nearest global.json file and install the version specified there.
Usage example