winget-cli
winget-cli copied to clipboard
microsoft
Add AdvancedInstaller (exe) type and Default Switches
- [x] Have you signed the Contributor License Agreement?
- [ ] Are you working against an Issue?
- No. I noticed that several (~100+) packages use exe's built with Advanced Installer and that they all had the same switches. I felt that having a dedicated installer type for this would save contributors from having to add installer switches and expected return codes for this type of executable. Additionally, it would show good faith and reciprocity towards Advanced Installer as they mention winget on their page about Finding Silent Install Switches
- This PR does not consider all changes required for REST support
- This PR does not consider any learn.microsoft.com documentation that would need to be added. For example - https://www.advancedinstaller.com/silent-install-exe-msi-applications.html being linked in Installer Switches
- This PR does not consider wingetcreate support for the installer type
Microsoft Reviewers: Open in CodeFlow
- There's an issue: https://github.com/microsoft/winget-cli/issues/4429
@denelon, do we want to take this in as part of v1.4 or we'll include it in v1.5? Seems like it'd be better to do some e2e validations before we take in a new InstallerType support.
For adding support of a new type, it'd be better to add the basic install, upgrade, uninstall workflow tests in Workflow.cpp in unit tests to exercise the whole flow, and 1 basic install, uninstall tests in e2e tests.
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
For adding support of a new type, it'd be better to add the basic install, upgrade, uninstall workflow tests in Workflow.cpp in unit tests to exercise the whole flow, and 1 basic install, uninstall tests in e2e tests.
I completely agree; However - I have no clue what I'm doing when it comes to writing (or running) the tests. I took what I saw from other unit tests and think I kind of understood them, but I really have no idea. Any guidance that you can provide here would be appreciated.
Regarding the e2e tests - I think I understand the code portion of it. I see where the manifests are written, I see how the test cases are running the install/uninstall, What I'm not exactly sure on is how to generate / build / include the actual exe file for testing. I presume this would have to be included as either a) A separate project with a build output, or b) Taking the Taking the AppInstallerTestExeInstaller.exe and re-packaging it with AdvancedInstaller, then placing it in the TestData folder. Again, this is something I don't really know how to do myself, but if there's someone who can guide me through it, I'd be happy to learn.
Sorry I should provide more details in my previous comments.
Regarding unit tests, they should be added to WorkFlow.cpp under AppInstallerCliTests peoject, the project builds as an exe, you can just run the exe to run the tests. For Workflow tests specifically, we create a fake source with predefined query like here , then we create the command directly and override some workflows as necessary like here, finally we verify the installer is successfully invoked with expected args. You can maybe write something similar to this test Same for upgrade and uninstall flow
Regarding e2e tests, it would be p2 as I believe most advancedInstaller should execute like an exe And e2e tests is quite complex to setup. But if there's time to add one , please just use the existing AppInstallerTestExeInstaller.exe, winget wants to test the installer is invoked as expected(not the installer itself is working as expected or not). If you add a new manifest to the Manifests folder where other test manifests sit, it should be picked up and included in a test source automatically.
By the way, I just saw Com api needs to be updated and hooked up accordingly here too.
Regarding unit tests, they should be added to WorkFlow.cpp under AppInstallerCliTests peoject, the project builds as an exe, you can just run the exe to run the tests. For Workflow tests specifically, we create a fake source with predefined query like here , then we create the command directly and override some workflows as necessary like here, finally we verify the installer is successfully invoked with expected args. You can maybe write something similar to this test Same for upgrade and uninstall flow
Got it; I think my understanding was correct, I'll wait for the code review to see how badly I messed up.
Regarding e2e tests, it would be p2 as I believe most advancedInstaller should execute like an exe And e2e tests is quite complex to setup. But if there's time to add one , please just use the existing AppInstallerTestExeInstaller.exe, winget wants to test the installer is invoked as expected(not the installer itself is working as expected or not). If you add a new manifest to the Manifests folder where other test manifests sit, it should be picked up and included in a test source automatically.
Got it. I'll take a look and see what I can do
By the way, I just saw Com api needs to be updated and hooked up accordingly here too.
I think I got those as part of my initial commit?
Regarding
I think I got those as part of my initial commit?
That should be good I think, maybe I overlooked this one..
Advanced Installer returns the same exit code as MSI's except -1 and 1, according to https://www.advancedinstaller.com/user-guide/exe-setup-file.html.
Advanced Installer returns the same exit code as MSI's except -1 and 1, according to https://www.advancedinstaller.com/user-guide/exe-setup-file.html.
I must have missed that; Will update
Azure Pipelines successfully started running 1 pipeline(s).
I cri. For some reason it won't let me run the tests locally, seems like its related to my setup though. It's complaining the VS2019 build tools aren't installed when they are.
Would you be able to help me identify where the error is?
Would you be able to help me identify where the error is?
The pipeline shows 10 tests failed, some seems not directly related to this change. Do you mind if I clone your branch and directly push the fix while testing?
Would you be able to help me identify where the error is?
The pipeline shows 10 tests failed, some seems not directly related to this change. Do you mind if I clone your branch and directly push the fix while testing?
I don't mind at all; If you have any suggestions on how I can improve too, I'm happy to learn Edit: I'd like to rebase this on master first to ensure the additional expected return codes can be used
Edit: I'd like to rebase this on master first to ensure the additional expected return codes can be used
Sure, thanks
Edit: I'd like to rebase this on master first to ensure the additional expected return codes can be used
Sure, thanks
Rebased and pushed
/azp run
Thank you, I think I see why it was erroring now with the commits you added
Though the tests were fixed, I just realized AdvancedInstaller is an installer authoring tool that supports exe, msi, msix, appv, etc. We may need to reevaluate how we approach this installer type, if some advanced installer exe was actually just a bootstrapper for some msi(I may just be making things up based on my initial very basic exploration), the upgrade will be broken. Since we are going to have a release candidate for v1.4 soon, we'll have to revisit and take this change in v1.5 or future after we have time to think this through.
Though the tests were fixed, I just realized AdvancedInstaller is an installer authoring tool that supports exe, msi, msix, appv, etc. We may need to reevaluate how we approach this installer type, if some advanced installer exe was actually just a bootstrapper for some msi(I may just be making things up based on my initial very basic exploration), the upgrade will be broken. Since we are going to have a release candidate for v1.4 soon, we'll have to revisit and take this change in v1.5 or future after we have time to think this through.
Fair enough; I'll convert to draft for now then
I would add onto this, though, that msi and msix already have their own installer types (and appv isn't a supported type anyways). It wouldn't be insurmountable to document that the advancedInstaller type is specifically for exe's. I do understand the concern that some advancedInstaller exe's are just bootstrappers, but this can easily be solved with AppsAndFeaturesEntries putting the InstallerType as msi.
While some AI installers such as Nutstore.Nutstore write WindowsInstaller = 1 to their ARP entries and would be identified as msi installers by WinGet, there are also some AI installers such as Cocos.CocosDashboard doesn't do that and would be identified as normal exe installers.
MSI installers built by Advanced Installer uses APPDIR as installation directory instead of TARGETDIR and INSTALLDIR, so it may be necessary to add advanced installer (msi) to installer types.
In that case, would changing this to advInstallerExe suffice, which would allow for advInstallerMsi as well?
In that case, would changing this to advInstallerExe suffice, which would allow for advInstallerMsi as well?
@yao-msft Thoughts on this approach?
In that case, would changing this to advInstallerExe suffice, which would allow for advInstallerMsi as well?
@yao-msft Thoughts on this approach?
More granular definitely works. If winget is to support AdvancedInstaller, we need some investigation and testing to be done internally to see how much we need to support them. From the discussion above, I think we need at least advInstallerExe and advInstalleMsi, it does not look very good if winget only supports "partial" of an installer technology. Mainly, the team may need a bit more time to try and understand what this installer technology is doing and decide how much we will support.