Refactor pinning evaluation #4151
Conversation
| } | ||
|
|
||
| // Gets a value indicating whether an available version is newer than the installed version. | ||
| virtual bool IsUpdateAvailable(PinBehavior pinBehavior) const = 0; |
There was a problem hiding this comment.
I'm guessing this is removed to move the work to the caller, who should consider pins?
There was a problem hiding this comment.
Correct, it didn't seem right to leave this there to be misused. Additionally, it can't properly consider multiple installed versions, which is a major factor driving this change.
| // Determines if the given version is an update to the installed version that this object was created with. | ||
| bool IsUpdate(const std::shared_ptr<AppInstaller::Repository::IPackageVersion>& availableVersion); |
There was a problem hiding this comment.
It does not itself consider pins. It is just the new location for this method to try and force proper consideration of pins. An even larger change might be to create an entire "version selector" class that considers many variables before deciding on the most appropriate version to install.
| // Gets the latest available package that fits within the pinning restrictions. | ||
| std::shared_ptr<AppInstaller::Repository::IPackageVersion> GetLatestAvailableVersionForPins(const std::shared_ptr<AppInstaller::Repository::IPackage>& package); |
There was a problem hiding this comment.
Is the installed version in package used in any way? Do we have any expectations about it? (e.g., it is not set, or it matches what this object has)
| // Gets the latest available package that fits within the pinning restrictions. | ||
| std::shared_ptr<AppInstaller::Repository::IPackageVersion> GetLatestAvailableVersionForPins(const std::shared_ptr<AppInstaller::Repository::IPackage>& package); | ||
|
|
||
| // Determines if the given version is an update to the installed version that this object was created with. | ||
| bool IsUpdate(const std::shared_ptr<AppInstaller::Repository::IPackageVersion>& availableVersion); |
There was a problem hiding this comment.
These functions assume that the available version passed as parameters are for the same package as the installed version, right? If so, I'd mention that in a comment
| std::shared_ptr<AppInstaller::Repository::Microsoft::PinningIndex> m_database; | ||
| std::optional<Pin> m_installedPin; | ||
| std::optional<Utility::VersionAndChannel> m_installedVersion; | ||
| std::map<PinKey, std::optional<Pin>> m_availablePins; |
There was a problem hiding this comment.
It took me a moment to get that this is only used as a cache for EvaluatePinType() and I had to look for all uses to make sure. May be worth adding a comment here to make it easier to understand.
| return PinType::Unknown; | ||
| } | ||
|
|
||
| // Gets the pinned state for an available PackageVersionKey that may have a pin, |
| PortableIndex::PortableIndex(PortableIndex&&) = default; | ||
| PortableIndex& PortableIndex::operator=(PortableIndex&&) = default; | ||
|
|
||
| PortableIndex::~PortableIndex() = default; |
There was a problem hiding this comment.
Just curious, is there particular reason we move these default constructor/destructor here where in other places we just define "= default" in header? Or this is the preferred convention in future?
There was a problem hiding this comment.
It is part of pImpl; the destruction of the smart pointer has to happen in the compilation unit that has access to the destructor. Leaving it in the header requires that the caller can do that, but moving it to the .cpp moves that requirement to the implementation. This allows the IPortableIndex type to simply be a forward declaration for the public header and only a complete type in the .cpp.
This is not new convention, but application of a specific technique for this purpose.
| @@ -29,6 +29,11 @@ namespace AppInstaller::Repository::Microsoft | |||
| return { filePath, disposition, std::move(indexFile) }; | |||
| } | |||
|
|
|||
| // Opens or creates a PinningIndex database on the default path. | |||
There was a problem hiding this comment.
Will update the comment in my other change as soon as I merge them.
Change
Moves the evaluation of pinning to when it is needed, rather than doing it for every composite search. This both makes it more "pay for play" and enables more complex scenarios in the future by allowing the call sites to change behavior more easily.
Also fixes a few problems that I found along the way:
"file"for files in the project,<file>for files outside of the project)DefaultInstallVersionandIsUpdateAvailablepropertiesValidation
Existing regression tests.
Microsoft Reviewers: Open in CodeFlow