-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not attempt post install ARP correlation if PackageFamilyName is provided and present for the user #3391
Conversation
Url: https://localhost:5001/TestKit/AppInstallerTestExeInstaller/AppInstallerTestExeInstaller.exe | ||
Sha256: <EXEHASH> | ||
InstallerType: exe | ||
PackageFamilyName: 6c6338fe-41b7-46ca-8ba6-b5ad5312bb0e_8wekyb3d8bbwe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the intended way of doing the correlation? I thought that's what the AppsAndFeaturesEntries were for, so that the different InstallerType could be specified. In fact, this isn't the first time I've seen the issue where an exe installs MSIX packages -
- Add way to specify MSIX details such as Package Family Name for AppsAndFeatures Entries #2804
- https://github.com/Trenly/winget-pkgs/blob/9cf38f7be8b2f18cab84cefed19b00d957c5e9db/manifests/m/Microsoft/WindowsAppRuntime/1/2/1.2.2/Microsoft.WindowsAppRuntime.1.2.installer.yaml#L12-L15
If this is the intended way, that's great, I would only ask that it be documented somewhere that the PackageFamilyName may be used for correlation even though it is outside of the AppsAndFeaturesEntries, since based on the current manifest documentation, I would have thought that it wouldn't be used. And then, since the package behaves differently based on the OS Version, that there would be an installer node for each MinimumOSVersion
that has a unique AppsAndFeaturesEntries
to make the correlation unambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AppsAndFeaturesEntries
is for the Add/Remove Programs data in the Uninstall registry key. It doesn't contain the package family name:
winget-cli/schemas/JSON/manifests/v1.5.0/manifest.singleton.1.5.0.json
Lines 568 to 600 in 531d2f1
"AppsAndFeaturesEntry": { | |
"type": "object", | |
"properties": { | |
"DisplayName": { | |
"type": [ "string", "null" ], | |
"minLength": 1, | |
"maxLength": 256, | |
"description": "The DisplayName registry value" | |
}, | |
"Publisher": { | |
"type": [ "string", "null" ], | |
"minLength": 1, | |
"maxLength": 256, | |
"description": "The Publisher registry value" | |
}, | |
"DisplayVersion": { | |
"type": [ "string", "null" ], | |
"minLength": 1, | |
"maxLength": 128, | |
"description": "The DisplayVersion registry value" | |
}, | |
"ProductCode": { | |
"$ref": "#/definitions/ProductCode" | |
}, | |
"UpgradeCode": { | |
"$ref": "#/definitions/ProductCode" | |
}, | |
"InstallerType": { | |
"$ref": "#/definitions/InstallerType" | |
} | |
}, | |
"description": "Various key values under installer's ARP entry" | |
}, |
Yes, the WinApp SDK / Runtime is what caused us to open that value up to installer types that were not definitely MSIX based. Where would you suggest that it be documented that Installer > PackageFamilyName
be used when an MSIX is installed?
The OS version targeting portion is relevant to the real manifest in question, not the test one. I don't think it should be necessary if we can help it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, that clarifies things. I think it can be documented at pkgs in the manifest docs; I can do that after this is merged.
Thank you for the explanation!
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -691,6 +691,15 @@ namespace AppInstaller::CLI::Workflow | |||
return; | |||
} | |||
|
|||
// If the installer claims to have a PackageFamilyName, and that family name is currently registered for the user, | |||
// let that be the correlated item and skip any attempt at further ARP correlation. | |||
auto installer = context.Get<Execution::Data::Installer>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit const auto& to avoid a copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that auto
doesn't preserve the reference in the type constantly escapes my brain, no matter how many times I relearn it...
…rovided and present for the user (#3391) The simple change is to not attempt the confidence interval-based correlation after installing an installer that has a `PackageFamilyName` value present, when that family name is found to be registered for the user.
…rovided and present for the user (#3391) The simple change is to not attempt the confidence interval-based correlation after installing an installer that has a `PackageFamilyName` value present, when that family name is found to be registered for the user.
Change
We had the case of an EXE installer that would install an MSI on lower OS versions and an MSIX on higher ones. This change attempts to improve the correlation experience as best as we can, although given the current greedy correlation strategy this requires some careful work in the manifest construction. Even then you can't get the best case results, but this allows more options. By changing the package name slightly, one can give up matching pre-existing installs of the MSI in exchange for detecting that the MSIX has been uninstalled outside of winget.
The simple change is to not attempt the confidence interval-based correlation after installing an installer that has a
PackageFamilyName
value present, when that family name is found to be registered for the user.Validation
A new E2E test is added that recreates the targeted flow.
Microsoft Reviewers: codeflow:open?pullrequest=#3391