From ef4e619350c4fb9afad051b83c224341a38c030f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 25 Aug 2025 01:56:15 -0500 Subject: [PATCH] CmdPal: Fix winget in release builds (#41183) Seems as though that doing `.ToArray` on the `IIterable<>` things we get back from the winget API were not trim-safe. I'm not entirely sure why. But we're totally okay just manually iterating over these things. That works like a charm. Closes #41172 As a drive-by, I wrapped the line from #41025 in a try-catch. If that doesn't fix it, hopefully it at least gives us more logging. --- .../CommandItemViewModel.cs | 22 ++++--- .../Microsoft.CmdPal.UI.csproj | 6 ++ .../Pages/InstallPackageListItem.cs | 43 +++++++++---- .../Pages/WinGetExtensionPage.cs | 63 ++++++++++++------- 4 files changed, 91 insertions(+), 43 deletions(-) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/CommandItemViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/CommandItemViewModel.cs index 22dad16504..ea7b8fb516 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/CommandItemViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.Core.ViewModels/CommandItemViewModel.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System.Diagnostics.CodeAnalysis; +using ManagedCommon; using Microsoft.CmdPal.Core.ViewModels.Messages; using Microsoft.CmdPal.Core.ViewModels.Models; using Microsoft.CommandPalette.Extensions; @@ -181,15 +182,15 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa if (more is not null) { MoreCommands = more - .Select(item => + .Select(item => { if (item is ICommandContextItem contextItem) { - return new CommandContextItemViewModel(contextItem, PageContext) as IContextItemViewModel; + return new CommandContextItemViewModel(contextItem, PageContext); } else { - return new SeparatorViewModel() as IContextItemViewModel; + return new SeparatorViewModel(); } }) .ToList(); @@ -237,8 +238,9 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa FastInitializeProperties(); return true; } - catch (Exception) + catch (Exception ex) { + Logger.LogError("error fast initializing CommandItemViewModel", ex); Command = new(null, PageContext); _itemTitle = "Error"; Subtitle = "Item failed to load"; @@ -257,9 +259,10 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa SlowInitializeProperties(); return true; } - catch (Exception) + catch (Exception ex) { Initialized |= InitializedState.Error; + Logger.LogError("error slow initializing CommandItemViewModel", ex); } return false; @@ -272,8 +275,9 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa InitializeProperties(); return true; } - catch (Exception) + catch (Exception ex) { + Logger.LogError("error initializing CommandItemViewModel", ex); Command = new(null, PageContext); _itemTitle = "Error"; Subtitle = "Item failed to load"; @@ -342,15 +346,15 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa if (more is not null) { var newContextMenu = more - .Select(item => + .Select(item => { if (item is ICommandContextItem contextItem) { - return new CommandContextItemViewModel(contextItem, PageContext) as IContextItemViewModel; + return new CommandContextItemViewModel(contextItem, PageContext); } else { - return new SeparatorViewModel() as IContextItemViewModel; + return new SeparatorViewModel(); } }) .ToList(); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/Microsoft.CmdPal.UI.csproj b/src/modules/cmdpal/Microsoft.CmdPal.UI/Microsoft.CmdPal.UI.csproj index 97ad65dddd..14689413c1 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/Microsoft.CmdPal.UI.csproj +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/Microsoft.CmdPal.UI.csproj @@ -23,6 +23,12 @@ false false + + + true diff --git a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/InstallPackageListItem.cs b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/InstallPackageListItem.cs index e2a3d2e4b4..a8eda1bae9 100644 --- a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/InstallPackageListItem.cs +++ b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/InstallPackageListItem.cs @@ -32,7 +32,16 @@ public partial class InstallPackageListItem : ListItem { _package = package; - var version = _package.DefaultInstallVersion ?? _package.InstalledVersion; + PackageVersionInfo? version = null; + try + { + version = _package.DefaultInstallVersion ?? _package.InstalledVersion; + } + catch (Exception e) + { + Logger.LogError("Could not get package version", e); + } + var versionTagText = "Unknown"; if (version is not null) { @@ -57,20 +66,27 @@ public partial class InstallPackageListItem : ListItem } catch (COMException ex) { - Logger.LogWarning($"{ex.ErrorCode}"); + Logger.LogWarning($"GetCatalogPackageMetadata error {ex.ErrorCode}"); } if (metadata is not null) { - if (metadata.Tags.Where(t => t.Equals(WinGetExtensionPage.ExtensionsTag, StringComparison.OrdinalIgnoreCase)).Any()) + for (var i = 0; i < metadata.Tags.Count; i++) { - if (_installCommand is not null) + if (metadata.Tags[i].Equals(WinGetExtensionPage.ExtensionsTag, StringComparison.OrdinalIgnoreCase)) { - _installCommand.SkipDependencies = true; + if (_installCommand is not null) + { + _installCommand.SkipDependencies = true; + } + + break; } } - var description = string.IsNullOrEmpty(metadata.Description) ? metadata.ShortDescription : metadata.Description; + var description = string.IsNullOrEmpty(metadata.Description) ? + metadata.ShortDescription : + metadata.Description; var detailsBody = $""" {description} @@ -116,9 +132,11 @@ public partial class InstallPackageListItem : ListItem // These can be l o n g { Properties.Resources.winget_release_notes, (metadata.ReleaseNotes, string.Empty) }, }; - var docs = metadata.Documentations.ToArray(); - foreach (var item in docs) + var docs = metadata.Documentations; + var count = docs.Count; + for (var i = 0; i < count; i++) { + var item = docs[i]; simpleData.Add(item.DocumentLabel, (string.Empty, item.DocumentUrl)); } @@ -141,7 +159,7 @@ public partial class InstallPackageListItem : ListItem } } - if (metadata.Tags.Any()) + if (metadata.Tags.Count > 0) { DetailsElement pair = new() { @@ -169,6 +187,7 @@ public partial class InstallPackageListItem : ListItem { // Handle other exceptions ExtensionHost.LogMessage($"[WinGet] UpdatedInstalledStatus throw exception: {ex.Message}"); + Logger.LogError($"[WinGet] UpdatedInstalledStatus throw exception", ex); return; } @@ -233,10 +252,10 @@ public partial class InstallPackageListItem : ListItem Stopwatch s = new(); Logger.LogDebug($"Starting RefreshPackageCatalogAsync"); s.Start(); - var refs = WinGetStatics.AvailableCatalogs.ToArray(); - - foreach (var catalog in refs) + var refs = WinGetStatics.AvailableCatalogs; + for (var i = 0; i < refs.Count; i++) { + var catalog = refs[i]; var operation = catalog.RefreshPackageCatalogAsync(); operation.Wait(); } diff --git a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/WinGetExtensionPage.cs b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/WinGetExtensionPage.cs index c348d209d4..155478cb52 100644 --- a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/WinGetExtensionPage.cs +++ b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.WinGet/Pages/WinGetExtensionPage.cs @@ -32,7 +32,7 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable private CancellationTokenSource? _cancellationTokenSource; private Task>? _currentSearchTask; - private IEnumerable? _results; + private List? _results; public static string ExtensionsTag => "windows-commandpalette-extension"; @@ -48,7 +48,6 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable public override IListItem[] GetItems() { - IListItem[] items = []; lock (_resultsLock) { // emptySearchForTag === @@ -61,12 +60,28 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable { IsLoading = true; DoUpdateSearchText(string.Empty); - return items; + return []; } - if (_results is not null && _results.Any()) + if (_results is not null && _results.Count != 0) { - ListItem[] results = _results.Select(PackageToListItem).ToArray(); + var count = _results.Count; + var results = new ListItem[count]; + var next = 0; + for (var i = 0; i < count; i++) + { + try + { + var li = PackageToListItem(_results[i]); + results[next] = li; + next++; + } + catch (Exception ex) + { + Logger.LogError("error converting result to listitem", ex); + } + } + IsLoading = false; return results; } @@ -82,7 +97,7 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable IsLoading = false; - return items; + return []; } private static ListItem PackageToListItem(CatalogPackage p) => new InstallPackageListItem(p); @@ -108,7 +123,7 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable _cancellationTokenSource = new CancellationTokenSource(); - CancellationToken cancellationToken = _cancellationTokenSource.Token; + var cancellationToken = _cancellationTokenSource.Token; IsLoading = true; @@ -139,7 +154,7 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable { try { - IEnumerable results = await searchTask; + var results = await searchTask; // Ensure this is still the latest task if (_currentSearchTask == searchTask) @@ -156,7 +171,7 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable catch (Exception ex) { // Handle other exceptions - Logger.LogError(ex.Message); + Logger.LogError("Unexpected error while processing results", ex); } } @@ -165,10 +180,10 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable Logger.LogDebug($"Completed search for '{query}'"); lock (_resultsLock) { - this._results = results; + this._results = results.ToList(); } - RaiseItemsChanged(this._results.Count()); + RaiseItemsChanged(); } private async Task> DoSearchAsync(string query, CancellationToken ct) @@ -190,12 +205,12 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable HashSet results = new(new PackageIdCompare()); // Default selector: this is the way to do a `winget search ` - PackageMatchFilter selector = WinGetStatics.WinGetFactory.CreatePackageMatchFilter(); + var selector = WinGetStatics.WinGetFactory.CreatePackageMatchFilter(); selector.Field = Microsoft.Management.Deployment.PackageMatchField.CatalogDefault; selector.Value = query; selector.Option = PackageFieldMatchOption.ContainsCaseInsensitive; - FindPackagesOptions opts = WinGetStatics.WinGetFactory.CreateFindPackagesOptions(); + var opts = WinGetStatics.WinGetFactory.CreateFindPackagesOptions(); opts.Selectors.Add(selector); // testing @@ -204,7 +219,7 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable // Selectors is "OR", Filters is "AND" if (HasTag) { - PackageMatchFilter tagFilter = WinGetStatics.WinGetFactory.CreatePackageMatchFilter(); + var tagFilter = WinGetStatics.WinGetFactory.CreatePackageMatchFilter(); tagFilter.Field = Microsoft.Management.Deployment.PackageMatchField.Tag; tagFilter.Value = _tag; tagFilter.Option = PackageFieldMatchOption.ContainsCaseInsensitive; @@ -215,11 +230,11 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable // Clean up here, then... ct.ThrowIfCancellationRequested(); - Lazy> catalogTask = HasTag ? WinGetStatics.CompositeWingetCatalog : WinGetStatics.CompositeAllCatalog; + var catalogTask = HasTag ? WinGetStatics.CompositeWingetCatalog : WinGetStatics.CompositeAllCatalog; // Both these catalogs should have been instantiated by the // WinGetStatics static ctor when we were created. - PackageCatalog catalog = await catalogTask.Value; + var catalog = await catalogTask.Value; if (catalog is null) { @@ -235,8 +250,8 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable // BODGY, re: microsoft/winget-cli#5151 // FindPackagesAsync isn't actually async. - Task internalSearchTask = Task.Run(() => catalog.FindPackages(opts), ct); - FindPackagesResult searchResults = await internalSearchTask; + var internalSearchTask = Task.Run(() => catalog.FindPackages(opts), ct); + var searchResults = await internalSearchTask; // TODO more error handling like this: if (searchResults.Status != FindPackagesResultStatus.Ok) @@ -247,13 +262,17 @@ internal sealed partial class WinGetExtensionPage : DynamicListPage, IDisposable } Logger.LogDebug($" got results for ({query})", memberName: nameof(DoSearchAsync)); - foreach (Management.Deployment.MatchResult? match in searchResults.Matches.ToArray()) + + // FYI Using .ToArray or any other kind of enumerable loop + // on arrays returned by the winget API are NOT trim safe + var count = searchResults.Matches.Count; + for (var i = 0; i < count; i++) { + var match = searchResults.Matches[i]; + ct.ThrowIfCancellationRequested(); - // Print the packages - CatalogPackage package = match.CatalogPackage; - + var package = match.CatalogPackage; results.Add(package); }