Add AdvancedInstaller (exe) type and Default Switches #2617
Conversation
|
|
@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. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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 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. |
Got it; I think my understanding was correct, I'll wait for the code review to see how badly I messed up.
Got it. I'll take a look and see what I can do
I think I got those as part of my initial commit? |
|
Regarding
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. |
I must have missed that; Will update |
|
/azp run |
|
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? |
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 |
> I have no clue what I'm doing - Hoping this is right?
Sure, thanks |
Trenly
force-pushed
the
AdvancedInstaller
branch
from
37c3aac to
2707216
Compare
Rebased and pushed |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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. |
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 |
|
MSI installers built by Advanced Installer uses |
|
In that case, would changing this to advInstallerExe suffice, which would allow for advInstallerMsi as well? |
|
Yes. |
@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. |
|
Going to close this due to it being extremely far behind on commits, no immediate need for it, and it would be easier to redo the changes than to try and fix the merge conflicts. |
Microsoft Reviewers: Open in CodeFlow