Use workload sets as per user request #37681

Forgind · 2023-12-29T00:27:00Z

This PR should enable installing and updating via workload sets. In particular, if you have mode set to workload sets (and don't use a rollback file), it will now look for the latest workload set (or latest stable if you don't specify to use previews) and install/update to that rather than updating manifests individually.

It now also supports dotnet workload update --version x where it will update the workload set version x.

Finally, it now will display the workload set it is using if it's set to use workload sets in the install state file when the user runs dotnet workload list.

All functionality was manually tested at some point, and there's a new unit test.

Causes the dotnet workload update/install/repair commands to use a new GetWorkloadInstallMode method to decide whether to use workload sets or not. If they decide to use workload sets, they calculate manifest updates by reading from a dictionary that is not implemented.

src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs

joeloff · 2024-01-10T17:29:05Z

src/Cli/dotnet/commands/dotnet-workload/install/NetSdkMsiInstallerClient.cs

            _shutdown = true;
        }

        public PackageId GetManifestPackageId(ManifestId manifestId, SdkFeatureBand featureBand)


This is misleading, I'd suggest having two separate methods for manifest and workload set package, even though there similar - it will make it much easier to understand the code.

I'll admit you're right that it's a little misleading. I'm trying to balance making things clearer vs. reusing more code—since this method is called in the middle of where I need it, I'd have to either put a random if/else in the middle of that method (that is, move this up one stack frame) or copy most of the method into a separate method just for workload sets. I'd rather not lose any updates that come in the future, which is why I architected it this way. If you feel strongly, though, I can change that.

Do you still need an if/else in the calling method? I see that this method is called on line 285 with the workload set package ID hardcoded, so it could just call a different method directly there. Are there other places where you're calling it and you don't know whether it's being called for a workload set or a normal manifest?

In GetManifestPackageDownloadsAsync and UpdateManifestWithVersionAsync, we shouldn't know whether it's a workload set manifest ID or something else.

Plus more correct MSI-based workload set installation

… install state (dotnet#38117)

(and this time, build to make sure that's it)

Version is not written into the install state by default anymore, so this writes it into a temporary "versionFile" (analogous to packageVersion.txt) and verifies that it's updated to the right workload set version.

Forgind · 2024-03-04T19:10:52Z

Windows_NT Build_Release and Darwin Build_Release both failed due to CanBuildProjectWithAPackageReferenceWithMultipleAliases. I didn't bother rerunning them.

dsplaisted

It looks like this PR is feature-complete for merging, thanks.

Some feedback (which is also covered in PR comments on the code):

Terminology: We should use "workload version" instead of "workload set" or "workload set version" when possible in user-facing messages.
Resolver changes / workload set filenames: I sent an email about this. I think we want to revert most of the SdkDirectoryWorkloadManifestProvider changes here and re-generate the workload set packages with different .json filenames.
Pinning: We shouldn't write the workload set version to the install state file unless --version was specified (see my PR code comment for more details)
Transactions: The CliTransaction class isn't being used correctly, which I think means it wouldn't actually run the rollback actions.

Follow-up items (ie things to do in subsequent PRs):

Offline cache: We support a --download-to-cache parameter to download all required packages locally, and a --from-cache parameter to use that to do updates / installs without network access. It looks like those aren't supported for workload sets here. Those parameters are hidden and a lot lower priority, so we can add support for them as a follow-up item (if at all). FYI @marcpopMSFT @baronfel
Add support for dotnet workload restore with workload set version specified in global.json, as well as root tracking, garbage collection, etc.
Check for available workload set updates and notify user about them (see WriteUpdatableWorkloadsFile)
Tests: I'd like to see a lot more workload set tests. I think the VM testing infrastructure I'm working on will be great for this. I also have some ideas for file-based end-to-end workload tests, but probably that's a lot longer term.
ManifestVersionUpdate class: This isn't directly related to this PR or to workload sets, but I noticed it when reviewing the code. Do we need to represent the existing/old manifest version in ManifestVersionUpdate anymore? Manifests are now side-by-side so it's probably not needed.

We had also previously discussed how the APIs here are passing around information such as the workload set version via a path to the package in the advertising manifests. I still don't think that this is the best design. If we are worried about downloading packages twice, I took a look and for MSI-based installs we don't actually do so because the MSI installer manages its own cache of downloaded packages. If the double download is actually an issue we could do the same type of caching for the file-based installer.

However, I don't want to block on changing this design, so we can check it in as is and possibly refactor it later.

dsplaisted · 2024-03-05T14:19:01Z

src/Cli/Microsoft.DotNet.Cli.Utils/PathUtility.cs

+        /// and continues to its parent until it fails. Returns whether it succeeded
+        /// in deleting the file it was intended to delete.
+        /// </summary>
+        public static bool DeleteFileAndEmptyParents(string path)


Suggestion: I think this would feel safer if it also took a "root" path where it wouldn't delete above that. There are a few places in FileBasedInstaller (mainly in the garbage collection I think) where this could simplify the code.

Can you clarify what error case you're trying to avoid? The two possible cases I can think of are:

Deleting something you wanted

Hitting an exception at some point because we don't have access to a directory

The second could happen, though wrapping it in a try/catch would presumably be just as good. (I think this is very unlikely unless the directory with the file to delete is already protected.) The first shouldn't happen.

I think under the dotnet/metadata directory there might be directories that we don't want to delete even if they are empty. It's not a big deal though.

dsplaisted · 2024-03-05T14:22:58Z

src/Cli/dotnet/commands/InstallingWorkloadCommand.cs

                    manifestVersionUpdates.Select(update => new WorkloadManifestInfo(update.ManifestId.ToString(), update.NewVersion.ToString(), /* We don't actually use the directory here */ string.Empty, update.NewFeatureBand))
                    ).ToDictionaryForJson();

+        public static bool GetInstallStateMode(SdkFeatureBand sdkFeatureBand, string dotnetDir)


Returning a bool from a method that is named to return a "mode" doesn't make clear what the value of the boolean means. Perhaps this should be named ShouldUseWorkloadSetMode.

dsplaisted · 2024-03-05T14:33:52Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs

+                    using PackageArchiveReader packageReader = new(packagePath);
+                    var downloadedPackageVersion = packageReader.NuspecReader.GetVersion();
+                    var correctedPackageVersion = WorkloadSetPackageVersionToWorkloadSetVersion(_sdkFeatureBand, downloadedPackageVersion.ToString());
+                    File.WriteAllText(Path.Combine(adManifestPath, "packageVersion.txt"), correctedPackageVersion);


It looks like the contents of this file are the workload set version, not the package version, so the file should be named accordingly:

Suggested change

File.WriteAllText(Path.Combine(adManifestPath, "packageVersion.txt"), correctedPackageVersion);

File.WriteAllText(Path.Combine(adManifestPath, "workloadSetVersion.txt"), correctedPackageVersion);

Also, it might be good to create a constant so we don't have to hard code this file name in multiple places. There are various path components in the workload code that we don't do this for, but it might be a good idea for something with mixed case which is more likely to end up being inconsistent and only fail on case-sensitive file systems.

dsplaisted · 2024-03-05T14:40:01Z

src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs

+                    if (advertisingPackagePath is not null)
+                    {
+                        PathUtility.DeleteFileAndEmptyParents(advertisingPackagePath);
+                    }


This doesn't seem like the right rollback action here. The rollback here should undo what was done in the corresponding action, so it should delete the destination files that were copied, not the source. If the source needs to be deleted that should probably be done elsewhere.

dsplaisted · 2024-03-05T14:45:34Z

src/Cli/dotnet/commands/dotnet-workload/install/NetSdkMsiInstallerClient.cs

+                ExecutePackage(msi, plannedAction, msiPackageId);
+
+                // Update the reference count against the MSI.
+                UpdateDependent(InstallRequestType.AddDependent, msi.Manifest.ProviderKeyName, _dependent);


I think you only want to add the dependent if you installed the package, if you uninstalled it then you probably want to remove the dependent. @joeloff, does that seem right?

dsplaisted · 2024-03-07T17:21:03Z

src/Resolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs

                    {
                        WorkloadSet? workloadSet = null;
-                        foreach (var jsonFile in Directory.GetFiles(workloadSetDirectory, "*.workloadset.json"))
+                        var jsonFile = Path.Combine(workloadSetDirectory, "workloadset.json");


I sent an email about this. Probably we don't want to change this logic, as it would mean we'd have to update the MSBuild SDK Resolver in Visual Studio for it to work with workload sets. Keeping the current logic (ie reverting the change here) does probably mean the current workload set packages won't work and will have to be regenerated.

dsplaisted · 2024-03-07T17:25:49Z

...soft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.GlobalJsonReader.cs

                                        case JsonTokenType.PropertyName:
                                            var sdkPropName = reader.GetString();
-                                            if (string.Equals("workloadVersion", sdkPropName, StringComparison.OrdinalIgnoreCase))
+                                            if (string.Equals("workloadSetVersion", sdkPropName, StringComparison.OrdinalIgnoreCase))


What's the reason for this change? The spec specifies workloadVersion as the property name in the global.json file. If we were to make this change, this would be another case where we'd have to update the resolver in VS for it to work there.

My understanding was that we didn't have workload set version support for global.json yet, so I didn't think of it as a breaking change yet and preferred the more descriptive name. Both because it sounds like the current consensus is to use "workload version" instead of "workload set version" and because of what you said about updating VS, I don't think this is a good idea anymore, so I'll revert it.

dsplaisted · 2024-03-07T17:28:18Z

.../Microsoft.NET.Sdk.WorkloadManifestReader.Tests/SdkDirectoryWorkloadManifestProviderTests.cs

        }

-        [Fact]
-        public void WorkloadSetCanIncludeMultipleJsonFiles()


I don't think we should change the resolver behavior, so I think we should keep these tests.

dsplaisted · 2024-03-07T17:34:59Z

src/Tests/dotnet-workload-update.Tests/GivenDotnetWorkloadUpdate.cs

+            var resolverFactory = new MockWorkloadResolverFactory(Path.Combine(Path.GetTempPath(), "dotnetTestPath"), versionNumber, workloadResolver, "userProfileDir");
+            var updateCommand = new WorkloadUpdateCommand(Parser.Instance.Parse("dotnet workload update"), Reporter.Output, resolverFactory, workloadInstaller, nugetPackageDownloader, workloadManifestUpdater);
+
+            var installStatePath = Path.Combine(Path.GetTempPath(), "dotnetTestPath", "metadata", "workloads", versionNumber, "InstallState", "default.json");


I would avoid using the temp path for tests. I know we do have some tests that do it, but I think it's a lot better to call _testAssetsManager.CreateTestDirectory(identifier: upgrade.ToString(). Then you don't have to delete the directory and can investigate it after the test runs.

dsplaisted · 2024-03-07T18:19:10Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs

+                // this updates all the manifests 
+                var manifests = _workloadResolver.GetInstalledManifests();
+                await Task.WhenAll(manifests.Select(manifest => UpdateAdvertisingManifestAsync(manifest, includePreviews, offlineCache))).ConfigureAwait(false);
+                WriteUpdatableWorkloadsFile();


This file is used for a message we print out on dotnet commands letting people know that a workload update is available. We should do a similar thing for workload sets.

However, it doesn't need to be in this PR, we can do that as a follow-up.

dsplaisted

This is quite close. I had a few comments, the most important to change before merging is the name of the workloadVersion property in global.json.

dsplaisted · 2024-03-14T22:07:52Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadInstallCommand.cs

                        installer.SaveInstallStateManifestVersions(sdkFeatureBand, GetInstallStateContents(manifestsToUpdate));
                    }

+                    installer.AdjustWorkloadSetInInstallState(sdkFeatureBand, string.IsNullOrWhiteSpace(_workloadSetVersion) ? null : workloadSetVersion);


It seems a bit confusing that this uses two different values: _workloadSetVersion and workloadSetVersion. Is there any case where _workloadSetVersion would be set, but workloadVersion would have a different value? I think not, so I think this can use _workloadSetVersion in both cases. If not, then this should definitely have a comment explaining the difference.

Probably it would be good to have a comment anyway explaining why this uses _workloadSetVersion and not workloadSetVersion.

The only way I think _workloadSetVersion could be set, and it would be different from workloadSetVersion is if the user requests version x, and we reach out to NuGet and ask for x, and it returns version y instead. I don't think that's possible today—I think if NuGet can't find the right version, it would error rather than return the wrong version—so I think you're right that I can switch to just using _workloadSetVersion and delete workloadSetVersion. I should be able to do something similar in WorkloadUpdateCommand.

I don't think you want to delete workloadSetVersion entirely, just don't use it on this line. If there's no pinned workload set version and no version was specified on the command line, but the install state is opted into workload sets, then _workloadSetVersion would be null, but workloadSetVersion would be the latest workload set version that was found, and should be installed / used.

Right. We'd still install that version, but we don't actually need to keep track of it in Workload(Update/Install)Command.

dsplaisted · 2024-03-14T22:17:35Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs

+                            .Where(path => path.EndsWith(".nupkg") && Path.GetFileName(path).StartsWith(manifestPackageId.ToString(), StringComparison.OrdinalIgnoreCase))
+                            .Max() :


This looks like the correct logic if packageVersion is not specified. If it is specified, I think this logic needs to take the version into account to return the package version specified instead of the latest version in the cache.

That was how it was before...I'll admit I thought that was a little odd but figured it must not matter too much since we hadn't fixed it for other manifests? But I can change the logic for this.

I think the difference is that previously it would always look for the latest version of a package, it didn't have the capability of specifying a specific version.

Mmm...I think you're right. I'd thought there was something that handled versions previously, but I don't know what I was thinking of.

dsplaisted · 2024-03-14T22:24:54Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs

        }

-        private IEnumerable<(ManifestId Id, ManifestVersionWithBand ManifestWithBand)> ParseRollbackDefinitionFile(string rollbackDefinitionFilePath)
+        public IEnumerable<ManifestVersionUpdate> ParseRollbackDefinitionFiles(IEnumerable<string> rollbackFilePaths)


SdkDirectoryWorkloadManifestProvider.GetAvailableWorkloadSets has logic that does the same thing as this method. Could this be factored so there's only one implementation?

They don't look like they do the same thing. That method finds all installed workload sets and returns the ones that match this feature band. This takes in paths to workload sets and parses them into a set of manifest version updates. It's basically "what happens after you call SdkDirectoryWorkloadManifestProvider.GetAvailableWorkloadSets"

The methods don't do the same thing, but inside the loop in GetAvailableWorkloadSets it seemed like it was doing the same thing (aggregating multiple json files into a single workload set).

dsplaisted · 2024-03-14T22:28:19Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs

+            fullSet = fullSet.DistinctBy<(ManifestId, ManifestVersionWithBand), ManifestId>(update => update.Item1).ToList();
+            if (size != fullSet.Count)
+            {
+                throw new Exception();


We shouldn't throw an exception with no details here. It looks like this is probably supposed to catch the case where the same manifest ID appears in multiple json files in the workload set package. If so, ideally the message would include information about which ID was duplicated. However, since in practice we don't expect multiple json files in the package, it's not a big deal. But we still shouldn't throw an Exception without a message.

Good catch; that was supposed to have a real exception, but then I forgot to add that part 🙂

dsplaisted · 2024-03-14T22:35:39Z

src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs

+            {
+                // Rollback files are only for loose manifests. Update the mode to be loose manifests.
+                Reporter.WriteLine(LocalizableStrings.UpdateFromRollbackSwitchesModeToLooseManifests);
+                _workloadInstaller.UpdateInstallMode(_sdkFeatureBand, false);


This should probably be run inside the transaction, and rolled back if the update fails.

dsplaisted · 2024-03-14T22:38:37Z

src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs

                        _workloadInstaller.RemoveManifestsFromInstallState(sdkFeatureBand);
                    }

+                    _workloadInstaller.AdjustWorkloadSetInInstallState(sdkFeatureBand, string.IsNullOrWhiteSpace(_workloadSetVersion) ? null : workloadSetVersion);


I think maybe I got confused here by the difference between _workloadSetVersion and workloadSetVersion. I commented on the same code in WorkloadInstallCommand, I think it would be easier to understand if this line was consistent about using _workloadSetVersion, and it would be good to have a comment explaining why it does so.

dsplaisted · 2024-03-14T22:39:21Z

src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs

            return workloads;
        }
+
+        private void RunInNewTransaction(Action<ITransactionContext> a)


Is this the exact same code as in WorkloadInstallCommand? If so, should this be moved up to the base class?

I actually had the exact same thought. The only issue is that the strings are separate, so this is an annoyingly big change. I do think it would be cleaner to do it that way, though...

dsplaisted · 2024-03-14T22:49:12Z

src/Cli/dotnet/commands/dotnet-workload/InstallStateContents.cs

+        public Dictionary<string, string>? Manifests { get; set; }
+
+        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
+        public string? WorkloadSetVersion { get; set; }


I think I mentioned this when we were discussing the PR, but I may have forgotten to include it in the PR comments: The property name in the global.json file should be workloadVersion (though it looks like the tests weren't consistent with the casing, so probably we should be case-insensitive).

So this property should be named WorkloadVersion, or I think you could also use a Json attribute to change the name used when reading / writing json.

dsplaisted · 2024-03-14T22:50:46Z

.../Microsoft.NET.Sdk.WorkloadManifestReader.Tests/SdkDirectoryWorkloadManifestProviderTests.cs

                """
                {
-                    "workloadVersion": "8.0.201"
+                    "workloadSetVersion": "8.0.201"


As mentioned in InstallStateContents, the JSON property should be workloadVersion, so these changes in the tests should be reverted.

dsplaisted · 2024-03-14T22:53:22Z

src/Cli/dotnet/commands/dotnet-workload/install/WorkloadManifestUpdater.cs

+            return $"{nugetVersion.Major}.{nugetVersion.Patch}.{patch}{release}";
+        }
+
+        public static string WorkloadSetPackageVersionToWorkloadSetVersion(SdkFeatureBand sdkFeatureBand, string packageVersion)


It looks like this wasn't addressed yet, it would be good to sync the implementations with arcade, especially since there are tests covering arcade's version of this.

ghost added Area-Workloads untriaged Request triage from a team member labels Dec 29, 2023

Forgind changed the base branch from main to release/8.0.2xx December 29, 2023 00:27

Forgind commented Dec 29, 2023

View reviewed changes

src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs Outdated Show resolved Hide resolved

Forgind added 10 commits January 2, 2024 10:56

Simplify getting install mode

3a54940

Update to use workload sets

65ea81a

Update to properly install via workload set

02253d6

Undo List changes

63647b2

Get real version

347d0c9

Implement workload update --version

39f0ec7

Support rolling back workload set installation

52af35d

Don't roll back for file-based

2625649

Small touchup

198e397

First stab at MSI-based install

2a4a9f9

joeloff reviewed Jan 10, 2024

View reviewed changes

Forgind added 14 commits January 10, 2024 14:46

Reimplement rolling back

b28ca38

Plus more correct MSI-based workload set installation

Little fixups

0aca30e

Overwrite workload set if present

4d1b0fc

Fix garbage collecting workload sets

65e6c73

Find workload set before installing it

197b7f9

Support workload sets in list

324d5e0

Partial progress

d44a33d

Avoid elevation if we will noop in MSI-based operations to adjust the…

60a387f

… install state (dotnet#38117)

Finish adding to install state

402ff34

Remove InstallStateReader

5796ee5

Small fixes

4b8ab17

More small fixes

2e0d6e5

Fix rollback behavior

0e4c5f1

Update some tests

3a60e0c

Forgind requested a review from vijayrkn as a code owner March 1, 2024 20:50

Forgind changed the base branch from release/8.0.2xx to release/8.0.3xx March 1, 2024 20:50

Forgind removed request for a team, andriipatsula, ericstj, joperezr and vijayrkn March 1, 2024 20:52

Forgind added 3 commits March 1, 2024 13:51

Delete duplicate method (?)

8c8ce54

Fix method signature

5bbcb0d

(and this time, build to make sure that's it)

Fix test

5c09c2c

Version is not written into the install state by default anymore, so this writes it into a temporary "versionFile" (analogous to packageVersion.txt) and verifies that it's updated to the right workload set version.

build-analysis bot mentioned this pull request Mar 2, 2024

GivenThatWeHaveAPackageReferenceWithAliases tests failing in CI #39068

Closed

I'm dumb sometimes

b9f79ac

dsplaisted reviewed Mar 7, 2024

View reviewed changes

Forgind added 6 commits March 7, 2024 17:22

Most PR comments

e38c25a

Fix transactions and other little things

d0e0bcf

Permit multiple *.workloadset.json files

9a34b4b

Fix transaction usage + update some tests

11c0d44

Correct my test

c7ec231

Merge branch 'release/8.0.3xx' into sets-in-file-based

94255c9

dsplaisted approved these changes Mar 14, 2024

View reviewed changes

More feedback

5944045

Forgind merged commit 9927abb into dotnet:release/8.0.3xx Mar 15, 2024

Forgind deleted the sets-in-file-based branch March 15, 2024 18:04

dsplaisted mentioned this pull request Mar 29, 2024

Use System.Text.Json in Netfx build of MSBuildSdkResolver #39573

Merged

marcpopMSFT mentioned this pull request Apr 1, 2024

Fix loading of rollback installed workloads in Visual Studio #39901

Merged

KalleOlaviNiemitalo mentioned this pull request Apr 9, 2024

Updating a workload during the "patch Tuesday" release window corrupts most dotnet commands on Linux and macOS dotnet/aspnetcore#55032

Closed

1 task

	File.WriteAllText(Path.Combine(adManifestPath, "packageVersion.txt"), correctedPackageVersion);
	File.WriteAllText(Path.Combine(adManifestPath, "workloadSetVersion.txt"), correctedPackageVersion);

		.Where(path => path.EndsWith(".nupkg") && Path.GetFileName(path).StartsWith(manifestPackageId.ToString(), StringComparison.OrdinalIgnoreCase))
		.Max() :

Use workload sets as per user request #37681

Use workload sets as per user request #37681

Uh oh!

Conversation

Forgind commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Forgind commented Dec 29, 2023 •

edited

Loading

Forgind commented Mar 4, 2024 •

edited

Loading