-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Consider global.json for workload* commands #39644
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
Consider global.json for workload* commands #39644
Conversation
| if (_workloadSetVersionFromConstructor != null) | ||
| { | ||
| if (!availableWorkloadSets.TryGetValue(_workloadSetVersionFromConstructor, out _workloadSet)) | ||
| if (!availableWorkloadSets.TryGetValue(_workloadSetVersionFromConstructor, out _workloadSet) && error) |
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 is the part that I was poking at most recently, and I mentioned I'm not totally satisfied with its current state, but it sounded like it should mostly not error; this is just a bit more generous than that...
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.
After some thought, here's how I think it should work:
The scenario where it currently fails but we don't want it to fail is when we are going to install the requested workload set version. In that case, I think that most of the manifest provider and workload resolver functionality probably isn't used until we've downloaded and installed the requested workload set version and corresponding manifests, and then we call RefreshWorkloadManifests.
So what we should do is to delay throwing the error until the GetManifests method is called. Then we'll never actually end up throwing it in the case where we immediately install the needed workload set.
It looks like WorkloadResolver loads the manifests during its construction, so we would also need to modify that class to enable it to delay the initialization. It's possible that might cascade into other code that uses it too.
We may want to only delay the initialization when we are running commands such as install or update and possibly only when there is a global.json applicable. That would be to avoid changing existing behavior around when exceptions are thrown and what the stack trace is. It may not really matter though.
…consider-globaljson-in-sets
| <value>Updating workload version from {0} to {1}.</value> | ||
| </data> | ||
| <data name="CannotSpecifyVersionOnCommandLineAndInGlobalJson" xml:space="preserve"> | ||
| <value>Cannot specify a particular workload version on the command line via --version or --from-history when there is already a version specified in the global.json file.</value> |
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.
It would be good to show the path to the global.json file in this message, and maybe to recommend what to do to. For example:
Cannot specify a workload version on the command line via --version or --from-history when there is already a version specified in the global.json file {0}. To update the globally installed workload version, run the command outside of the path containing the global.json file. Otherwise update the version specified in the global.json file and run "dotnet workload update".
| var installStateContents = InstallStateContents.FromPath(path); | ||
| installStateContents.UseWorkloadSets = newMode; | ||
| File.WriteAllText(path, installStateContents.ToString()); | ||
| _reporter.WriteLine(string.Format(LocalizableStrings.UpdatedWorkloadMode, newMode ? "workload sets" : "loose manifests")); |
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.
I'm not sure whether we want to print this message every time the install mode is updated. I think if you run dotnet workload update --version 8.0.303, then we don't necessarily need to print this message. So probably this message should be generated from InstallingWorkloadCommand where we probably have that context.
Also, "workload sets" and "loose manifests" won't be localized, so we should probably use the exact strings that we use for the setting values, which are available via WorkloadConfigCommandParser.UpdateMode_WorkloadSet and WorkloadConfigCommandParser.UpdateMode_Manifests.
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.
If the user runs dotnet workload update --version 8.0.303, it's clear that they do want the workload set, but I feel like changing it without saying anything would be an unexpected side-effect. I could imagine someone expecting to update to that workload set but remain in loose manifest mode, as that's the 'minimal' change. I guess what I'm really saying is that although I agree it probably isn't necessary, I don't think it really hurts.
You're right that it won't be localized, but looking at it now, I'm wondering if that might actually be a good thing since the parameter the user will set on the command line isn't localized. I didn't make that here (extra space), but I'm wondering if the best answer might be to make it align with the parameter the user has to pass?
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.
You're right that it won't be localized, but looking at it now, I'm wondering if that might actually be a good thing since the parameter the user will set on the command line isn't localized. I didn't make that here (extra space), but I'm wondering if the best answer might be to make it align with the parameter the user has to pass?
Yes, that's what I was trying to say. It's OK if it's not localized, but it should match the actual command-line string, and you can use the WorkloadConfigCommandParser constants I mentioned to make sure that it's actually the right string.
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.
Ah, perfect 🙂 In that case, then, I'm just going to leave it as-is and let you fix it in your PR. The constants don't exist yet, so it'd just error if I did it now.
| { | ||
| if (string.IsNullOrWhiteSpace(_workloadSetVersion)) | ||
| { | ||
| _workloadSetVersion = workloadVersion; |
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.
If you run dotnet workload update with a workload set specified in global.json, it shouldn't be exactly the same as dotnet workload update --version <version>. With the explicit --version option we will write that version to the global install state, while if it's coming from global.json, it shouldn't affect the global install state at all.
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.
To be clear, that means that the version can differ between the global.json and what's "pinned" in the install state? I'm wondering how confusing that is for users...but I guess that's how the resolver does it, so I guess that makes sense.
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.
Yes, that's correct. You might have two different local repos with a different workload set version specified in global.json. Neither of those should affect the version of workloads used on projects outside of those repos.
(Technically running a command such as dotnet workload update to install the right workloads for one of the repos might install a newer workload set on the computer, and then that updated version would be used outside of the repo that needed the update. I think that's OK though.)
| var installStateFilePath = Path.Combine(WorkloadInstallType.GetInstallStateFolder(_sdkFeatureBand, _dotnetPath), "default.json"); | ||
| if (File.Exists(installStateFilePath)) | ||
| { | ||
| var installStateContents = InstallStateContents.FromPath(installStateFilePath); | ||
| _workloadSetVersion = installStateContents.WorkloadVersion; | ||
| } |
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.
I seem to think that this case is already handled elsewhere. But it's possible I'm mixing this up with what I've done in my refactoring PR.
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.
You were right! And more importantly, with how this case was handled (in the constructor), it would interpret something from the install state as a command line argument early, which means it would error if the install state and global.json disagreed. Good catch!
| var globaljsonPath = SdkDirectoryWorkloadManifestProvider.GetGlobalJsonPath(Directory.GetCurrentDirectory()); | ||
| var workloadVersion = SdkDirectoryWorkloadManifestProvider.GlobalJsonReader.GetWorkloadVersionFromGlobalJson(globaljsonPath); |
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.
Does this need to be done here? If we don't pass this to the WorkloadInstallCommand, won't the WorkloadInstallCommand find the global.json and handle it on its own anyway? And if so, we can probably also remove the new workloadSetVersion parameter to its constructor.
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.
Fair point; deleted.
| } | ||
| else if (!workloadVersion.Equals(_workloadSetVersion)) | ||
| { | ||
| // Consider setting this in the global.json? Or logging a warning? |
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.
I think this should behave the same as the install command and throw an error. Does that sound right?
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.
Yeah; that sounds good.
| if (_workloadSetVersionFromConstructor != null) | ||
| { | ||
| if (!availableWorkloadSets.TryGetValue(_workloadSetVersionFromConstructor, out _workloadSet)) | ||
| if (!availableWorkloadSets.TryGetValue(_workloadSetVersionFromConstructor, out _workloadSet) && error) |
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.
After some thought, here's how I think it should work:
The scenario where it currently fails but we don't want it to fail is when we are going to install the requested workload set version. In that case, I think that most of the manifest provider and workload resolver functionality probably isn't used until we've downloaded and installed the requested workload set version and corresponding manifests, and then we call RefreshWorkloadManifests.
So what we should do is to delay throwing the error until the GetManifests method is called. Then we'll never actually end up throwing it in the case where we immediately install the needed workload set.
It looks like WorkloadResolver loads the manifests during its construction, so we would also need to modify that class to enable it to delay the initialization. It's possible that might cascade into other code that uses it too.
We may want to only delay the initialization when we are running commands such as install or update and possibly only when there is a global.json applicable. That would be to avoid changing existing behavior around when exceptions are thrown and what the stack trace is. It may not really matter though.
| void IInstaller.UpdateInstallMode(SdkFeatureBand sdkFeatureBand, bool newMode) | ||
| { | ||
| UpdateInstallMode(sdkFeatureBand, newMode); | ||
| Reporter.WriteLine(string.Format(LocalizableStrings.UpdatedWorkloadMode, newMode ? "workload sets" : "loose manifests")); |
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.
As in the other installer, I think this should probably move to InstallingWorkloadCommand.
| } | ||
|
|
||
| [Fact] | ||
| public void ItFailsIfWorkloadSetFromGlobalJsonIsNotInstalled() |
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.
I think we should continue to fail builds if the version specified in global.json isn't available, so these two tests should be kept.
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.
Looks good. Can you also fix the file not found issue where it gives an error if you try to update in workload set mode but it doesn't find any workload sets for the feature band? My refactoring PR fixes that but at this point I think it's best if we delay that PR to 8.0.400.
| var versionInUse = resolver.GetWorkloadVersion(); | ||
| if (installedWorkloadSets.ContainsKey(versionInUse)) | ||
| { | ||
| WorkloadSetsToKeep.Add(versionInUse); | ||
| } |
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.
I think the thing to do for now since we don't garbage collect workload sets is to add all of the installed workload sets to the list of workload sets to keep. That way if we do a workload install in the global context, it won't collect workload manifests from a workload set that is referred to by global.json in another repo.
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.
Sounds good. I was very iffy about this bit because on the one hand, what I have here may GC a workload set you need for another repo, and I don't think we have a precise way to know that without basically scanning your whole drive for global.json files and combining them all. That's bad. On the other hand, your solution means we never properly GC workload sets, which means that if someone uses them for a long time and updates regularly, they'll take up an unbounded space and eventually overflow your computer. I don't like either solution, but I don't have a better option in mind...
I'll leave the install state logic for now; it should be relevant after this change, but it won't hurt, and we may want to move back to it at some point.
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.
When we do garbage collection for workload sets, we will keep track of what global.json files we've used, so we will be able to find them without scanning the whole disk.
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 is the method that does garbage collection for workload sets, and it identified the workload set specified in the global.json file in the current directory as one we didn't need and scheduled it for deletion...
| if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) && string.IsNullOrWhiteSpace(_workloadSetVersion) && string.IsNullOrWhiteSpace(_workloadSetVersionFromGlobalJson) && | ||
| (InstallStateContents.FromPath(installStateFilePath)?.Manifests is not null) || InstallStateContents.FromPath(installStateFilePath)?.WorkloadVersion is not null) |
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.
I think you can use pattern matching here to avoid calling InstallStateContents.FromPath twice:
| if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) && string.IsNullOrWhiteSpace(_workloadSetVersion) && string.IsNullOrWhiteSpace(_workloadSetVersionFromGlobalJson) && | |
| (InstallStateContents.FromPath(installStateFilePath)?.Manifests is not null) || InstallStateContents.FromPath(installStateFilePath)?.WorkloadVersion is not null) | |
| if (string.IsNullOrWhiteSpace(_fromRollbackDefinition) && string.IsNullOrWhiteSpace(_workloadSetVersion) && string.IsNullOrWhiteSpace(_workloadSetVersionFromGlobalJson) && | |
| InstallStateContents.FromPath(installStateFilePath) is var installState && (installState.Manifests is not null || installState.WorkloadVersion is not null)) |
| { | ||
| var correctedVersion = WorkloadSetVersionToWorkloadSetPackageVersion(version); | ||
| await UpdateManifestWithVersionAsync("Microsoft.NET.Workloads", includePreviews: true, _sdkFeatureBand, new NuGetVersion(correctedVersion), offlineCache); | ||
| Task.Run(() => UpdateManifestWithVersionAsync("Microsoft.NET.Workloads", includePreviews: true, _sdkFeatureBand, new NuGetVersion(correctedVersion), offlineCache)).Wait(); |
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 definitely shouldn't have been async void. It could be async Task instead but then you need to call a wait further up in the call stack.
Is there any reason not to do this instead though? It seems simpler:
| Task.Run(() => UpdateManifestWithVersionAsync("Microsoft.NET.Workloads", includePreviews: true, _sdkFeatureBand, new NuGetVersion(correctedVersion), offlineCache)).Wait(); | |
| UpdateManifestWithVersionAsync("Microsoft.NET.Workloads", includePreviews: true, _sdkFeatureBand, new NuGetVersion(correctedVersion), offlineCache).Wait(); |
I'm not an async expert though, maybe running it on another thread with Task.Run would help prevent deadlocks (it could also cause them for all I know though).
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.
I am definitely not an async expert either, but I was reading this:
https://grantwinney.com/call-an-async-method-from-a-synchronous-one/
and he said that just calling .Wait() afterwards would make it deadlock; putting it in Task.Run() first would mean that the waiting thread and the thread with work are separate, which prevents deadlocks.
This is obviously very weak evidence, but I ran it several times, and it didn't deadlock 🤷
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.
To my knowledge, if the program is running on a single thread, then calling .Wait without Task.Run can deadlock.
Wait is designed to be used for when you want to block the calling thread, without Task.Run this could end up being the same thread and cause a deadlock.
See: https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.wait?view=net-8.0#:~:text=//%20%20%20%20%20%20%20N%3A%20%20%20%20%20%20%201%2C000%2C000-,Remarks,-Wait%20is%20a
Is there a reason you can't use await instead? This will simply pause the method and add it back to the threadpool to be executed later, whenever theawaited thing finishes. Either way, I think this should be fine, but dont get of the Run if you dont use await.
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.
I'm sure you already know this, but never take running it a few times or even a few thousand times as evidence 😉
It is always worth fully understanding async code before committing it. It's very easy to create silent killers over time that slowly erode at a codebase's reliability and create bug reports that are a real headache. Eventually the bugs will compound and exponentially decay the software's ability to function at all. By that point, enough code will probably be written on top of the deadlocks / race conditions that it will be impossible to decouple, unravel, and synchronize the logic properly, so you'll end up having to dump the whole thing out
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.
To use await, you have to be in an async method, which means the caller (or somewhere up the stack) will need to do something like Wait(), or you'll need to make all your methods up the stack to the entry point async.
|
|
||
| public IEnumerable<ManifestVersionUpdate> ParseRollbackDefinitionFiles(IEnumerable<string> rollbackFilePaths) | ||
| { | ||
| var zeroVersion = new ManifestVersion("0.0.0"); |
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.
It would be good to add a comment explaining that the previous version isn't actually used anymore so we just set it to 0 to avoid having to resolve the actual manifest versions (which can fail with global.json scenarios).
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.
I was mulling this over last night and realized that we do actually use this in the MSI-based installer in two ways: to plan the package (upgrade, downgrade, or 'none'?) and to roll back if we hit an exception (manifestUpdate.Reverse()...)
I'm not sure what the ideal answer is for that scenario. We'd need some way to figure out what's currently installed without erroring...
/cc: @joeloff
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.
I think we can fix the MSI installer code. We don't upgrade or downgrade from an MSI perspective anymore, with side-by-side manifests the MSIs for different versions of the same manifest are not MSI upgrades, they're not related.
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.
I'm not sure what your suggestion is?
| sdkVersion, | ||
| Environment.GetEnvironmentVariable, | ||
| userProfileDir, | ||
| globalJsonPath: workloadSetVersion is null ? GetGlobalJsonPath(Environment.CurrentDirectory) : null, |
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.
| globalJsonPath: workloadSetVersion is null ? GetGlobalJsonPath(Environment.CurrentDirectory) : null, | |
| globalJsonPath: null, |
I don't think we should be getting the global.json path here if the passed in value is null. If we do garbage collection within the context of a global.json file, we need to be able to create a resolver in the non-global.json global install state context, so we know which workload sets and manifests to keep.
If we resolve the global.json path here, then I think we'll never have access to that global state when running a garbage collection from a folder that has a global.json.
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 is true, but conversely, we need to make sure we have a way to notice the workload set in a global.json because we don't want to uninstall a workload set that we know is in use by the repository we're currently in so that we can keep a global workload set we might not even be using.
I was thinking we'd keep the one that matters for our current case since we know we can't be perfect regardless. I can make it find both, but I don't want to just revert this part as you're suggesting. If the GC does its thing properly, reverting this part would break pinning to an older version via global.json.
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.
I guess it doesn't matter in terms of this PR anyway because it just doesn't GC at all, so it shouldn't matter if I just revert this, but when we properly implement GC for workload sets, we should remember to make a separate resolver for the current directory's global.json if necessary.
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.
When we do GC we will have a list of all the global.json files that we know about, so we will look at all of the workload sets that they refer to and then create resolvers for those workload sets. But we will probably pass the workload set version in directly instead of having it being read from global.json in that case (no need to create a bunch of different resolvers for a bunch of different global.json files that all refer to the same workload set).
| return m.Version + "/" + manifestInfo.ManifestFeatureBand; | ||
| }); | ||
| table.AddColumn(InformationStrings.WorkloadSourceColumn, workload => workload.Value); | ||
| var manifestInfoDict = _workloadListHelper.WorkloadResolver.GetInstalledManifests().ToDictionary(info => info.Id, StringComparer.OrdinalIgnoreCase); |
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.
It looks like this won't ever list the installed workloads if there's a global.json specifying the workload set version. We should still print out the table if there is a workload set version from global.json and that workload set is installed.
| // This file isn't created in tests. | ||
| PrintWorkloadSetTransition(File.ReadAllText(Path.Combine(advertisingPackagePath, Constants.workloadSetVersionFileName))); | ||
| } | ||
| else if (_workloadInstaller is FileBasedInstaller || _workloadInstaller is NetSdkMsiInstallerClient) |
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.
What's the reason for checking the types here? Is it because this is supposed to behave differently in tests?
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.
Yeah; the tests don't write and read real advertising manifests, and that means the file should never exist, but we shouldn't fail because of that.
|
|
||
| void AdjustWorkloadSetInInstallState(SdkFeatureBand sdkFeatureBand, string workloadVersion); | ||
|
|
||
| void NotifyInstallComplete(); |
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.
Per our discussion, consider getting rid of this method and moving the garbage collection outside of the transaction.
Ask mode template
Customer impact
This will let us start testing workload set support internally. Workload sets are a feature we’re targeting for broad adoption in 8.0.400, but we’d like to have our internal partners start testing the scenarios to help get feedback and find bugs. This PR fixes some bugs which will unblock testing and improves the experience when a workload set is specified in a global.json file.
Regression
No
Testing
Manual and automated testing
Risk
Low – Code changes apply mostly to workload set scenarios, which is a new mode that you need to explicitly opt in to
Original description
This should look for a global.json when using dotnet workload install/update/restore. If it finds one, and that global.json has a workloadVersion specified, it uses it as if it had been specified on the command line as long as a second version isn't specified on the command line. If a second version is specified on the command line for dotnet workload install, it errors.
I haven't really tested this yet, so I'm leaving it as a draft for now.I verified that, for Install and Update, it properly takes the global.json into account, i.e., it treats it the same as if you'd done
dotnet workload [install/update] --version <fromGlobalJson>This PR also adds some error checking and removes most of the errors from the workload resolver if we think we should be able to resolve a workload set from a particular source (global.json, install state file) and but fail to find the specified workload set already installed. We should ideally error at least when building (dotnet build) but plausibly other scenarios...