(#1109) Fix issue on upgrading prerelease packages#1118
Open
gep13 wants to merge 1 commit intochocolatey:developfrom
Open
(#1109) Fix issue on upgrading prerelease packages#1118gep13 wants to merge 1 commit intochocolatey:developfrom
gep13 wants to merge 1 commit intochocolatey:developfrom
Conversation
11 tasks
AdmiringWorm
requested changes
Mar 19, 2026
Member
AdmiringWorm
left a comment
AdmiringWorm
left a comment
There was a problem hiding this comment.
Not had a chance to take this for a spin yet, but figured I'd leave a few comments first.
| private string _localAppDataPath = string.Empty; | ||
| private const string ErrorRegex = "^\\s*(ERROR|FATAL|WARN)"; | ||
|
|
||
| private static readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim(); |
Member
There was a problem hiding this comment.
Weren't it decided to not use this Lock, and instead use the async lock instead?
I also couldn't see this lock being used anywhere in the file.
| config.RegularOutput = false; | ||
| config.QuietOutput = true; | ||
| config.Prerelease = false; | ||
| var configuration = choco.GetConfiguration(); |
Member
There was a problem hiding this comment.
Doesn't this have the same problem as other places (like list/search), since it isn't wrapped inside of an async lock, and since the Configuration object is shared, wouldn't it change the configuration object in other locations?
Comment on lines
+495
to
+496
| _chocolateyGuiCacheService.PurgeOutdatedPackages(source: null, includePrerelease: false); | ||
| await _eventAggregator.PublishOnUIThreadAsync(new PackageInstalledMessage(Id, Version, ChocolateySource)); |
Member
There was a problem hiding this comment.
Not saying to change anything right now, but we may consider an optimization in the future to verify that the cache file actually includes the package id that we reinstalled/installed/upgraded, instead of blindly removing it.
Comment on lines
+808
to
826
| public void Handle(PackagePinnedMessage message) | ||
| { | ||
| if (!string.Equals(message.Id, Id, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| IsPinned = true; | ||
| } | ||
|
|
||
| public void Handle(PackageUnpinnedMessage message) | ||
| { | ||
| if (!string.Equals(message.Id, Id, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| LatestVersion = message.Version; | ||
| IsOutdated = true; | ||
| IsPinned = false; | ||
| } |
Member
There was a problem hiding this comment.
Instead of having two seperate handlers on this, it would maybe be a bit better to just have a single message, which have a property that say explicitly whether it should be pinned or not?
gep13
force-pushed
the
issue-1109
branch
2 times, most recently
from
7677a94 to
7be1b96
Compare
Member
Author
|
@st3phhays I have some things to respond to @AdmiringWorm about on this one, but can I get your eyes on this from a Code Owners perspective? Thanks! |
st3phhays
previously approved these changes
Mar 24, 2026
gep13
force-pushed
the
issue-1109
branch
2 times, most recently
from
4b1c9d3 to
5c25793
Compare
This is a fairly significant overhaul of how the Chocolatey GUI internal messaging is done, in order to work around an issue that was idenitifed with the process of upgrading packages, when there is a prerelease version of the package available from a remote source. There were a combination of issues are play here, and I think it is safe to say that this _never_ worked. What follows is a summary of the changes that had to be made in order to get things to work, along with some other minor tweaks that had to be made in order to get things to work cleanly. The root of the problem was essentially that the RemoteSourceViews and some of the methods that are executed in the ChocolateyService, simply didn't know anything about prerelease packages. As a result, the ability to use the Upgrade Context menu action was never presented, and the Outdated/Installed status for a package was never updated correctly. - In order to keep information about the remote package that was located for a RemoteSource, we need to keep information about it around, so that this information can be used at later times. To that end, the ChocolateySource and RemoteVersion properties were added to the IPackageViewModel interface. - On the IChocolateyService, two new methods were added that can take into account what package version is being acted on, and also which RemoteSource made the request. This latter part is important when it comes to updating information about packages. In addition, the packageName argument was removed from the GetOutdatedPackages method, as this is no longer required. The old methods were marked as Obsolete, so that they aren't used again by mistake. - On the IChocoalteyGuiCacheService, a new method was added for purging the outdated packages. This needed to be more granular, and only delete the file(s) associated with a particular RemoteSource. - The new PurgeOutdatedPackages takes into account the fact that a persisted file is now kept around on a per RemoteSource basis. This was required due to the fact that a package could be outdated from more than one RemoteSource, with different package versions. As such, we need to be able to either delete _all_ files when purging, or only specific ones. - New Resource Strings were added to cover some new areas of functionality, and a single string was updated to make it clearer about what version is being shown. - Several new discrete message classes were added, and ultimately replaced the single PackageChangedMessage. This was the biggest part of the refactoring here, since it proved to be too complicated trying to update things based on a single message, with different types. By having a message for each change that can happen to a packge, it became much easier to update the necessary portions of the UI when things happened. - The RemoteSourceView was updated to include the information about whether a package was outdated or not. This had always been shown when using the Grid View layout, but it was never included in the list view for the remote source. On top of this, the binding for the list of Packages was switched to use an ICollectionView, similar to what is done on the LocalSourceView, there was no good reason for having a different approach here. - On the PackageView, an additional TextBlock was added to show the Available Version, in addition to essentially the installed version of a package, when the package is marked as IsOutdated. - The RemoteSourceViewModel was updated to record the Source that is used when querying for packages, as well as the RemoteVersion that was located. This information is very important when processing the update messages that are being sent out. It was also found that the information about whether a locally installed package was IsPinned or not wasn't being added to the PackageViewModel, so this was corrected as part of the overall work here. - In the LocalSrouceViewModel, the handle method for the PackageChangedMessage was completely removed, and replaced with smaller message handlers, to only deal with the install and uninstall methods, which are responsible for adding/removing a package from the overall package list. Important change here is to mark the ChocolateySource as null for this ViewModel, so it is clear that these are packages on the local computer. - Several changes in the PackageViewModel... It was found that some methods, Uninstall and Reinstall were available when a package was pinned, which isn't allowed. The Can methods were updated to include information about the package IsPinned property. All the _eventAggregator.PublishOnUIThreadAsync methods were updated to use the smaller messages, rather than the larger PackageChangedMessage message, which has beenn deleted. New handle methods were added for each of the new small messages, to correctly update the package inforamtion when an action was taken. Care was taken to only do this when it is known to be required, for example, uninstalling when on the LocalSource is not required. The biggest check here is about only marking a package as outdated when it is known to be being shown on the RemoteSource associated with where the outdated information is coming from. - In the ChocolateyService, a race condition was discovered, where the ChocolateyConfiguration was being overwritten before the intended execution could be completed. To guard against this, Lock.WriteLockAsync was added before each invocation. This approach was already being used in some areas, but not all of them, so the decision was taken to added it to all methods. When storing the information about what packages are outdated, a file is now created per RemoteSource, and also whether it includes prerelease packages or not. The actual call for outdated packages was updated to use the same approach that is used in Chocolatey Agent, rather than calling UpgradeDryRun. These are _essentially_ the same thing, but it was felt that it is better to keep things consistent across products. The UpdatePackage method was updated to include the version number, which now allows the installation of prerelease package versions.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description Of Changes
This is a fairly significant overhaul of how the Chocolatey GUI internal
messaging is done, in order to work around an issue that was idenitifed
with the process of upgrading packages, when there is a prerelease
version of the package available from a remote source. There were a
combination of issues are play here, and I think it is safe to say that
this never worked. What follows is a summary of the changes that had
to be made in order to get things to work, along with some other minor
tweaks that had to be made in order to get things to work cleanly. The
root of the problem was essentially that the RemoteSourceViews and some
of the methods that are executed in the ChocolateyService, simply didn't
know anything about prerelease packages. As a result, the ability to
use the Upgrade Context menu action was never presented, and the
Outdated/Installed status for a package was never updated correctly.
for a RemoteSource, we need to keep information about it around, so
that this information can be used at later times. To that end, the
ChocolateySource and RemoteVersion properties were added to the
IPackageViewModel interface.
into account what package version is being acted on, and also which
RemoteSource made the request. This latter part is important when it
comes to updating information about packages. In addition, the
packageName argument was removed from the GetOutdatedPackages method,
as this is no longer required. The old methods were marked as
Obsolete, so that they aren't used again by mistake.
the outdated packages. This needed to be more granular, and only
delete the file(s) associated with a particular RemoteSource.
persisted file is now kept around on a per RemoteSource basis. This
was required due to the fact that a package could be outdated from
more than one RemoteSource, with different package versions. As such,
we need to be able to either delete all files when purging, or only
specific ones.
functionality, and a single string was updated to make it clearer
about what version is being shown.
replaced the single PackageChangedMessage. This was the biggest part
of the refactoring here, since it proved to be too complicated trying
to update things based on a single message, with different types. By
having a message for each change that can happen to a packge, it
became much easier to update the necessary portions of the UI when
things happened.
whether a package was outdated or not. This had always been shown when
using the Grid View layout, but it was never included in the list view
for the remote source. On top of this, the binding for the list of
Packages was switched to use an ICollectionView, similar to what is
done on the LocalSourceView, there was no good reason for having a
different approach here.
Available Version, in addition to essentially the installed version of
a package, when the package is marked as IsOutdated.
used when querying for packages, as well as the RemoteVersion that was
located. This information is very important when processing the update
messages that are being sent out. It was also found that the
information about whether a locally installed package was IsPinned or
not wasn't being added to the PackageViewModel, so this was corrected
as part of the overall work here.
PackageChangedMessage was completely removed, and replaced with
smaller message handlers, to only deal with the install and uninstall
methods, which are responsible for adding/removing a package from the
overall package list. Important change here is to mark the
ChocolateySource as null for this ViewModel, so it is clear that these
are packages on the local computer.
methods, Uninstall and Reinstall were available when a package was
pinned, which isn't allowed. The Can methods were updated to include
information about the package IsPinned property. All the
_eventAggregator.PublishOnUIThreadAsync methods were updated to use
the smaller messages, rather than the larger PackageChangedMessage
message, which has beenn deleted. New handle methods were added for
each of the new small messages, to correctly update the package
inforamtion when an action was taken. Care was taken to only do this
when it is known to be required, for example, uninstalling when on the
LocalSource is not required. The biggest check here is about only
marking a package as outdated when it is known to be being shown on
the RemoteSource associated with where the outdated information is
coming from.
ChocolateyConfiguration was being overwritten before the intended
execution could be completed. To guard against this,
Lock.WriteLockAsync was added before each invocation. This approach
was already being used in some areas, but not all of them, so the
decision was taken to added it to all methods. When storing the
information about what packages are outdated, a file is now created
per RemoteSource, and also whether it includes prerelease packages or
not. The actual call for outdated packages was updated to use the same
approach that is used in Chocolatey Agent, rather than calling
UpgradeDryRun. These are essentially the same thing, but it was
felt that it is better to keep things consistent across products. The
UpdatePackage method was updated to include the version number, which
now allows the installation of prerelease package versions.
Motivation and Context
A bug was raised showing how it wasn't possible to upgrade to a prerelease package.
Testing
chocolatey-aupackage already installedchocolateysource, and search for thechocolatey-aupackage (best done using theMatch Word Exactlycheckbox)ops-choco-packagessource, and search for thechocolatey-aupackage, including theInclude PrereleasecheckboxInclude Prereleasecheckboxchoco-internal-testingsource, and search for thechocolatey-aupackage, including theInclude PrereleasecheckboxInclude PrereleasecheckboxAll Sourcessource, and search for thechocolatey-aupackage (best done using theMatch Word Exactlycheckbox), including theInclude PrereleasecheckboxInclude PrereleasecheckboxC:\Users\<user>\AppData\Local\Chocolatey GUIfolder, you should see xml files for all the sources, including-re.xml. You won't seeoutdatedPackages-pre.xmlsince you can't search for prerelease on theThis PCsourceInclude Prereleasecheckbox each time, and you should see the following for the chocolatey-au package:Include PrereleasecheckedInclude PrereleasecheckedInclude PrereleasecheckedInclude prereleasecheckbox checked as you leave each sourcechocolateysource and install the chocolatey-au packagechocolatey-aupackage is marked as installed, and also as a tile in theThis PCsource. In addition, it should be marked as:choco-internal-testingsource, and choose to upgrade the packagechocolatey-aupackage is marked as:This PCsource, right click on thechocolatey-aupackage and choose to pin the packagechocolatey-aupackage and choose to unpin the packageThat is it for testing, but feel free to play around with some additional tests as you see fit.
Operating Systems Testing
Dev VM 4
Change Types Made
Change Checklist
Related Issue
Fixes #1109