Skip to content

Comments

Ensure default values are passed to nested installers within zip#2413

Merged
ryfu-msft merged 6 commits intomicrosoft:masterfrom
ryfu-msft:msiZipBug
Aug 11, 2022
Merged

Ensure default values are passed to nested installers within zip#2413
ryfu-msft merged 6 commits intomicrosoft:masterfrom
ryfu-msft:msiZipBug

Conversation

@ryfu-msft
Copy link
Contributor

@ryfu-msft ryfu-msft commented Aug 3, 2022

Fixes #2392

Changes:

  • Changes installer.InstallerType -> installer.BaseInstallerType.
  • Created a method called EffectiveInstallerType() that returns the actual installerType that will be executed (i.e. the NestedInstallerType if it is a zip installer)

Tests:

  • Reenabled ZipWithMsi test
  • All existing tests should pass
Microsoft Reviewers: Open in CodeFlow

@ryfu-msft ryfu-msft requested a review from a team as a code owner August 3, 2022 22:15
@ghost ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Aug 3, 2022
{
const auto& installer = context.Get<Execution::Data::Installer>().value();
if (installer.InstallerType == InstallerTypeEnum::MSStore)
if (installer.EffectiveInstallerType() == InstallerTypeEnum::MSStore)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[This is the correct usage here just calling this out if it isn't already done...]
As MSStore is not an installer type that actually has an installer file (to us), it should not be a valid nested type. Is that already handled elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only check I had was from manifest validation. I added another check prior to the install flow to verify that if the NestedInstallertype is valid if the BaseInstallertype is an archive. Outputs the appropriate error message if NestedInstallertype is not supported.

if (installer)
{
info << " "_liv << Execution::ManifestInfoEmphasis << Resource::String::ShowLabelInstallerType << ' ' << Manifest::InstallerTypeToString(installer->InstallerType) << std::endl;
info << " "_liv << Execution::ManifestInfoEmphasis << Resource::String::ShowLabelInstallerType << ' ' << Manifest::InstallerTypeToString(installer->BaseInstallerType) << std::endl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should be something like:

Effective (Base)

When effective and base are not the same. The important part is the effective type, but also showing the base is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to suggested.

If effective and base are not the same, will show both "Effective (Base)". Added tests to verify output


// Populate installer default return codes if not present in ExpectedReturnCodes and InstallerSuccessCodes
auto defaultReturnCodes = GetDefaultKnownReturnCodes(installer.InstallerType);
auto defaultReturnCodes = GetDefaultKnownReturnCodes(installer.BaseInstallerType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effective, unless it is not yet know, then some more effort will be required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to effective


// Installer Switches
installer.Switches = Manifest::GetDefaultKnownSwitches(installer.InstallerType);
installer.Switches = Manifest::GetDefaultKnownSwitches(installer.BaseInstallerType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effective, unless it is not yet know, then some more effort will be required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to effective

@ghost ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Aug 10, 2022
@ghost ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Aug 11, 2022
@ryfu-msft ryfu-msft requested a review from JohnMcPMS August 11, 2022 21:54
@ryfu-msft ryfu-msft merged commit 5390e1c into microsoft:master Aug 11, 2022
@ryfu-msft ryfu-msft deleted the msiZipBug branch August 11, 2022 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue-Bug It either shouldn't be doing this or needs an investigation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Zip installers not getting nested installer type defaults

2 participants