Implement PackageValidator - IDL, velocity, and impl stubs#5864
Implement PackageValidator - IDL, velocity, and impl stubs#5864letao-msft merged 3 commits intomainfrom
Conversation
letao-msft
force-pushed
the
user/letwang/packagevalidatorimpl
branch
from
1f8ef3a to
e29c732
Compare
dev/Common/IsWindowsVersion.h
Outdated
| #define __ISWINDOWSVERSION_H | ||
|
|
||
| #include <VersionHelpers.h> | ||
| #include <wil/com.h> |
There was a problem hiding this comment.
line 6.5
#include <wil/cppwinrt.h>
Ensures WIL & C++/WinRT handle each others' exceptions
wil/cppwinrt.h must be included before any other WIL header. Since you're #include'ing wil/com here and VersionHelpers.h is an OS header best to add wil/cppwinrt before it #Resolved
dev/Common/IsWindowsVersion.h
Outdated
| return IsExportPresent(L"appxdeploymentclient.dll", "MsixIsPackageFeatureSupported"); | ||
| } | ||
|
|
||
| inline bool SupportsIAppxFactory4() |
There was a problem hiding this comment.
As this isn't an OS detection function it shouldn't be in this header
Feature functions belong with their feature.
RECOMMENDATION: Move this to dev/PackageManager/API/AppxPackagingObject.h #Resolved
| <feature> | ||
| <name>Feature_PackageValidation</name> | ||
| <description>PackageValidation support in PackageDeploymentManager</description> | ||
| <state>AlwaysEnabled</state> |
There was a problem hiding this comment.
If it's AlwaysEnabled then there's no need to have the feature defined
Unless you expect this to go into a stable or preview release in an experimental state you don't need this feature. As long as this is in main it'll go into the future 2.0 release and you're good. If you expect to backport to e.g. 1.8 then containment would be needed, but (a) you'd add that in the 1.8 backport and (b) that's probably handled via A/B containment and not TerminalVelocity (ie different mechanism).
TL;DR I think you can drop this.
There was a problem hiding this comment.
OK, dropping.
What is Feature_PackageManager above doing?
| public: | ||
| PackageValidationEventSource() = default; | ||
|
|
||
| bool Run(winrt::Windows::Foundation::Uri const& packageUri, winrt::Windows::Foundation::Collections::IMap<winrt::Windows::Foundation::Uri, hstring>& expectedDigests); |
There was a problem hiding this comment.
Purpose of Run is to evaluate the validators?
SUGGESTION: Validate(...) #Resolved
There was a problem hiding this comment.
Run() is part of the events contract. It's not a custom name.
| auto deferral = args.GetDeferral(); | ||
|
|
||
| if (m_validator && !m_validator.IsPackageValid(args.AppxPackagingObject())) | ||
| { |
There was a problem hiding this comment.
std::ignore = LOG_HR(E_CANCEL); ? #Resolved
There was a problem hiding this comment.
There is logging of specific reason inside individual validators. "Cancel" here is an implementation detail of the event pattern, logging this doesn't add any value.
| { | ||
| THROW_HR_IF(E_NOTIMPL, !::Microsoft::Windows::Management::Deployment::Feature_PackageValidation::IsEnabled()); | ||
|
|
||
| if (!m_packageValidators) |
There was a problem hiding this comment.
Same comment as in AddPackageOptions::PackageValidators() #Resolved
| <PublicHeaders Include="$(MSBuildThisFileDirectory)MsixPackageManager.h" /> | ||
| </ItemGroup> | ||
| </Project> | ||
| </Project> No newline at end of file |
There was a problem hiding this comment.
Add newline at end of file to keep GitHub happy #Resolved
| <Midl Include="$(MSBuildThisFileDirectory)PackageManager.idl" /> | ||
| </ItemGroup> | ||
| </Project> | ||
| </Project> No newline at end of file |
There was a problem hiding this comment.
Add newline at end of file to keep github happy #Resolved
DrusTheAxe
added
area-PackageManagement
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 participants
Added a basic skeleton for implementing the PackageValidator feature. This includes all changes to PackageManager.idl, adding a corresponding TerminalVelocity feature, and populating auto-generated .h and .cpp stubs.