Skip to content

Support custom installer success exit codes#778

Merged
yao-msft merged 2 commits into
microsoft:masterfrom
yao-msft:extraexitcodes
Mar 9, 2021
Merged

Support custom installer success exit codes#778
yao-msft merged 2 commits into
microsoft:masterfrom
yao-msft:extraexitcodes

Conversation

@yao-msft

@yao-msft yao-msft commented Mar 4, 2021

Copy link
Copy Markdown
Contributor

Also added tests for custom installer success codes.

Microsoft Reviewers: Open in CodeFlow

@yao-msft yao-msft requested a review from a team as a code owner March 4, 2021 22:02
AICLI_TERMINATE_CONTEXT(E_ABORT);
}
else if (installResult.value() != 0)
else if (installResult.value() != 0 && (std::find(successCodes.begin(), successCodes.end(), static_cast<int>(installResult.value())) == successCodes.end()))

@JohnMcPMS JohnMcPMS Mar 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

installResult.value() != 0 [](start = 17, length = 26)

Should 0 be included by default if codes are provided? What if 0 is a failure for some installer? I can understand the confusion that this could create, so it might be better to have a separate flag in the manifest for making 0 a failure value. #Resolved

AICLI_TERMINATE_CONTEXT(E_ABORT);
}
else if (installResult.value() != 0)
else if (installResult.value() != 0 && (std::find(successCodes.begin(), successCodes.end(), static_cast<int>(installResult.value())) == successCodes.end()))

@JohnMcPMS JohnMcPMS Mar 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

int [](start = 112, length = 3)

Should we just store the values in the manifest as DWORD then, since we will always compare against this value type? #Resolved

AICLI_TERMINATE_CONTEXT(E_ABORT);
}
else if (installResult.value() != 0)
else if (installResult.value() != 0 && (std::find(successCodes.begin(), successCodes.end(), static_cast<int>(installResult.value())) == successCodes.end()))

@JohnMcPMS JohnMcPMS Mar 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

successCodes [](start = 58, length = 12)

I think we are making a problem for ourselves later if we simply add this support without a healthy set of defaults in place. If we take this as is, this is the problem I imagine:

  1. Installer always return some value stating "reboot required for full functionality"
  2. Manifest author puts this value in success codes
  3. We add defaults later that make this value and others success for installation
  4. Installer starts returning one of the other values in some cases (maybe reboot required for any functionality)
  5. We fail the install because it isn't one of the values in the list
    Then do we need to start treating the values in the manifest as additional ones, beyond 0 and our defaults? How could they indicate to us that our defaults are incorrect; that one of them means a failure?

If the manifest called these AdditionalSuccessCodes, we would probably be fine to treat 0 and any values we wanted (on a per-InstallerType basis) as success. But if it calls them InstallerSuccessCodes, it seems like this is the list, full stop. #Resolved

@JohnMcPMS JohnMcPMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@yao-msft

yao-msft commented Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

Thanks to @chausner for initiating the first version of the fix at #296

@yao-msft yao-msft merged commit 2f0ef1f into microsoft:master Mar 9, 2021
@yao-msft yao-msft deleted the extraexitcodes branch March 9, 2021 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants