Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
Aug 11, 2022
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5a81f14
use effectiveInstallertype
Aug 3, 2022
871ae24
finish changes and enable test
Aug 3, 2022
6c27563
revert to base installerType for 1.1 schema
Aug 3, 2022
bc8bb21
resolve merge conflicts
Aug 4, 2022
f58bda9
add displayName
Aug 4, 2022
96c42b9
address PR feedback
Aug 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/AppInstallerCLICore/Workflows/ArchiveFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ namespace AppInstaller::CLI::Workflow
{
auto installer = context.Get<Execution::Data::Installer>().value();

if (IsArchiveType(installer.InstallerType))
if (IsArchiveType(installer.BaseInstallerType))
{
auto const& nestedInstallerFiles = installer.NestedInstallerFiles;
if (nestedInstallerFiles.empty())
Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ namespace AppInstaller::CLI::Workflow
Logging::Telemetry().LogSelectedInstaller(
static_cast<int>(itr->second.Installer.Arch),
itr->second.Installer.Url,
Manifest::InstallerTypeToString(itr->second.Installer.InstallerType),
Manifest::InstallerTypeToString(itr->second.Installer.EffectiveInstallerType()),
Manifest::ScopeToString(itr->second.Installer.Scope),
itr->second.Installer.Locale);

Expand Down
10 changes: 5 additions & 5 deletions src/AppInstallerCLICore/Workflows/DownloadFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ namespace AppInstaller::CLI::Workflow
std::wstring_view GetInstallerFileExtension(Execution::Context& context)
{
const auto& installer = context.Get<Execution::Data::Installer>();
switch (installer->InstallerType)
switch (installer->BaseInstallerType)
{
case InstallerTypeEnum::Burn:
case InstallerTypeEnum::Exe:
Expand Down Expand Up @@ -143,7 +143,7 @@ namespace AppInstaller::CLI::Workflow
if (!context.Contains(Execution::Data::InstallerPath))
{
const auto& installer = context.Get<Execution::Data::Installer>().value();
switch (installer.InstallerType)
switch (installer.BaseInstallerType)
{
case InstallerTypeEnum::Exe:
case InstallerTypeEnum::Burn:
Expand Down Expand Up @@ -183,7 +183,7 @@ namespace AppInstaller::CLI::Workflow
void CheckForExistingInstaller(Execution::Context& context)
{
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.

{
// No installer is downloaded in this case
return;
Expand Down Expand Up @@ -415,12 +415,12 @@ namespace AppInstaller::CLI::Workflow
auto existingFileHash = SHA256::ComputeHash(inStream);
context.Add<Execution::Data::HashPair>(std::make_pair(installer.Sha256, existingFileHash));
}
else if (installer.InstallerType == InstallerTypeEnum::MSStore)
else if (installer.EffectiveInstallerType() == InstallerTypeEnum::MSStore)
{
// No installer file in this case
return;
}
else if (installer.InstallerType == InstallerTypeEnum::Msix && !installer.SignatureSha256.empty())
else if (installer.EffectiveInstallerType() == InstallerTypeEnum::Msix && !installer.SignatureSha256.empty())
{
// We didn't download the installer file before. Just verify the signature hash again.
context << GetMsixSignatureHash;
Expand Down
8 changes: 4 additions & 4 deletions src/AppInstallerCLICore/Workflows/InstallFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ namespace AppInstaller::CLI::Workflow
{
auto installer = context.Get<Execution::Data::Installer>().value();

if (IsArchiveType(installer.InstallerType))
if (IsArchiveType(installer.BaseInstallerType))
{
context <<
Workflow::EnsureFeatureEnabled(Settings::ExperimentalFeature::Feature::ZipInstall);
Expand Down Expand Up @@ -217,7 +217,7 @@ namespace AppInstaller::CLI::Workflow

void ShowInstallationDisclaimer(Execution::Context& context)
{
auto installerType = context.Get<Execution::Data::Installer>().value().InstallerType;
auto installerType = context.Get<Execution::Data::Installer>().value().EffectiveInstallerType();

if (installerType == InstallerTypeEnum::MSStore)
{
Expand Down Expand Up @@ -387,7 +387,7 @@ namespace AppInstaller::CLI::Workflow

void ExecuteInstaller(Execution::Context& context)
{
context << Workflow::ExecuteInstallerForType(context.Get<Execution::Data::Installer>().value().InstallerType);
context << Workflow::ExecuteInstallerForType(context.Get<Execution::Data::Installer>().value().BaseInstallerType);
}

void ArchiveInstall(Execution::Context& context)
Expand Down Expand Up @@ -645,7 +645,7 @@ namespace AppInstaller::CLI::Workflow
// Ensure that installer type might actually write to ARP, otherwise this is a waste of time
auto installer = context.Get<Execution::Data::Installer>();

if (installer && MightWriteToARP(installer->InstallerType))
if (installer && MightWriteToARP(installer->EffectiveInstallerType()))
{
Repository::Correlation::ARPCorrelationData data;
data.CapturePreInstallSnapshot();
Expand Down
14 changes: 7 additions & 7 deletions src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ std::ostream& operator<<(std::ostream& out, const AppInstaller::Manifest::Manife
{
return out << '[' <<
AppInstaller::Utility::ToString(installer.Arch) << ',' <<
AppInstaller::Manifest::InstallerTypeToString(installer.InstallerType) << ',' <<
AppInstaller::Manifest::InstallerTypeToString(installer.EffectiveInstallerType()) << ',' <<
AppInstaller::Manifest::ScopeToString(installer.Scope) << ',' <<
installer.Locale << ']';
}
Expand All @@ -30,7 +30,7 @@ namespace AppInstaller::CLI::Workflow
{
// Unvirtualized resources restricted capability is only supported for >= 10.0.18362
// TODO: Add support for OS versions that don't support virtualization.
if (installer.InstallerType == InstallerTypeEnum::Portable && !Runtime::IsCurrentOSVersionGreaterThanOrEqual(Utility::Version("10.0.18362")))
if (installer.EffectiveInstallerType() == InstallerTypeEnum::Portable && !Runtime::IsCurrentOSVersionGreaterThanOrEqual(Utility::Version("10.0.18362")))
{
return InapplicabilityFlags::OSVersion;
}
Expand Down Expand Up @@ -218,7 +218,7 @@ namespace AppInstaller::CLI::Workflow
InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override
{
// The installer is applicable if it's type or any of its ARP entries' type matches the installed type
if (Manifest::IsInstallerTypeCompatible(installer.InstallerType, m_installedType))
if (Manifest::IsInstallerTypeCompatible(installer.EffectiveInstallerType(), m_installedType))
{
return InapplicabilityFlags::None;
}
Expand All @@ -238,7 +238,7 @@ namespace AppInstaller::CLI::Workflow
std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override
{
std::string result = "Installed package type '" + std::string{ Manifest::InstallerTypeToString(m_installedType) } +
"' is not compatible with installer type " + std::string{ Manifest::InstallerTypeToString(installer.InstallerType) };
"' is not compatible with installer type " + std::string{ Manifest::InstallerTypeToString(installer.EffectiveInstallerType()) };

std::string arpInstallerTypes;
for (const auto& entry : installer.AppsAndFeaturesEntries)
Expand All @@ -256,7 +256,7 @@ namespace AppInstaller::CLI::Workflow

bool IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override
{
return (first.InstallerType == m_installedType && second.InstallerType != m_installedType);
return (first.EffectiveInstallerType() == m_installedType && second.EffectiveInstallerType() != m_installedType);
}

private:
Expand Down Expand Up @@ -287,7 +287,7 @@ namespace AppInstaller::CLI::Workflow
InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override
{
// We have to assume the unknown scope will match our required scope, or the entire catalog would stop working for upgrade.
if (installer.Scope == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement || DoesInstallerIgnoreScopeFromManifest(installer))
if (installer.Scope == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement || DoesInstallerTypeIgnoreScopeFromManifest(installer.EffectiveInstallerType()))
{
return InapplicabilityFlags::None;
}
Expand Down Expand Up @@ -363,7 +363,7 @@ namespace AppInstaller::CLI::Workflow
if (m_requirement == Manifest::ScopeEnum::Unknown ||
installer.Scope == m_requirement ||
(installer.Scope == Manifest::ScopeEnum::Unknown && m_allowUnknownInAdditionToRequired) ||
DoesInstallerIgnoreScopeFromManifest(installer))
DoesInstallerTypeIgnoreScopeFromManifest(installer.EffectiveInstallerType()))
{
return InapplicabilityFlags::None;
}
Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCLICore/Workflows/PortableFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ namespace AppInstaller::CLI::Workflow

void EnsureSupportForPortableInstall(Execution::Context& context)
{
auto installerType = context.Get<Execution::Data::Installer>().value().InstallerType;
auto installerType = context.Get<Execution::Data::Installer>().value().EffectiveInstallerType();

if (installerType == InstallerTypeEnum::Portable)
{
Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCLICore/Workflows/ShowFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ namespace AppInstaller::CLI::Workflow
info << Execution::ManifestInfoEmphasis << Resource::String::ShowLabelInstaller << std::endl;
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

if (!installer->Locale.empty())
{
info << " "_liv << Execution::ManifestInfoEmphasis << Resource::String::ShowLabelInstallerLocale << ' ' << installer->Locale << std::endl;
Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCLICore/Workflows/UpdateFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ namespace AppInstaller::CLI::Workflow
Logging::Telemetry().LogSelectedInstaller(
static_cast<int>(installer->Arch),
installer->Url,
Manifest::InstallerTypeToString(installer->InstallerType),
Manifest::InstallerTypeToString(installer->EffectiveInstallerType()),
Manifest::ScopeToString(installer->Scope),
installer->Locale);

Expand Down
4 changes: 2 additions & 2 deletions src/AppInstallerCLICore/Workflows/WorkflowBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,7 +1078,7 @@ namespace AppInstaller::CLI::Workflow
Logging::Telemetry().LogSelectedInstaller(
static_cast<int>(installer->Arch),
installer->Url,
Manifest::InstallerTypeToString(installer->InstallerType),
Manifest::InstallerTypeToString(installer->EffectiveInstallerType()),
Manifest::ScopeToString(installer->Scope),
installer->Locale);
}
Expand Down Expand Up @@ -1123,7 +1123,7 @@ namespace AppInstaller::CLI::Workflow
{
searchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::ProductCode, MatchType::Exact, installer.ProductCode));
}
else if (installer.InstallerType == Manifest::InstallerTypeEnum::Portable)
else if (installer.EffectiveInstallerType() == Manifest::InstallerTypeEnum::Portable)
{
const auto& productCode = Utility::MakeSuitablePathPart(manifest.Id + "_" + source.GetIdentifier());
searchRequest.Inclusions.emplace_back(PackageMatchFilter(PackageMatchField::ProductCode, MatchType::CaseInsensitive, Utility::Normalize(productCode)));
Expand Down
12 changes: 1 addition & 11 deletions src/AppInstallerCLIE2ETests/InstallCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,6 @@ public void InstallNullSoft()
[Test]
public void InstallMSI()
{
if (string.IsNullOrEmpty(TestCommon.MsiInstallerPath))
{
Assert.Ignore("MSI installer not available");
}

var installDir = TestCommon.GetRandomTestDir();
var result = TestCommon.RunAICLICommand("install", $"TestMsiInstaller --silent -l {installDir}");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Expand Down Expand Up @@ -355,14 +350,9 @@ public void InstallZipWithInvalidRelativeFilePath()
Assert.True(result.StdOut.Contains("Invalid relative file path to the nested installer; path points to a location outside of the install directory"));
}

[Test, Ignore("Re-enable as part of fixing #2392")]
[Test]
public void InstallZipWithMsi()
{
if (string.IsNullOrEmpty(TestCommon.MsiInstallerPath))
{
Assert.Ignore("MSI installer not available");
}

var installDir = TestCommon.GetRandomTestDir();
var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.TestZipInstallerWithMsi --silent -l {installDir}");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Expand Down
4 changes: 3 additions & 1 deletion src/AppInstallerCLIE2ETests/ListCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,16 @@ public void ListAfterInstall()
string productCode = guid.ToString();
var installDir = TestCommon.GetRandomTestDir();

// DisplayName must be set to avoid conflicts with other packages that use the same exe installer.
string displayName = "TestExeInstaller";
string localAppDataPath = System.Environment.GetEnvironmentVariable(Constants.LocalAppData);
string logFilePath = System.IO.Path.Combine(localAppDataPath, Constants.E2ETestLogsPath);
logFilePath = System.IO.Path.Combine(logFilePath, "ListAfterInstall-" + System.IO.Path.GetRandomFileName() + ".log");

var result = TestCommon.RunAICLICommand("list", productCode);
Assert.AreEqual(Constants.ErrorCode.ERROR_NO_APPLICATIONS_FOUND, result.ExitCode);

result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.TestExeInstaller --override \"/InstallDir {installDir} /ProductID {productCode} /LogFile {logFilePath}\"");
result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.TestExeInstaller --override \"/InstallDir {installDir} /ProductID {productCode} /LogFile {logFilePath} /DisplayName {displayName}\"");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);

result = TestCommon.RunAICLICommand("list", productCode);
Expand Down
4 changes: 3 additions & 1 deletion src/AppInstallerCLIE2ETests/Test.runsettings
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
InvokeCommandInDesktopPackage: Bool to indicate using Invoke-CommandInDesktopPackage for test execution.
This is used when AppExecutionAlias is not available, or disabled.
StaticFileRootPath: Path to the set of static test files that will be served as the source for testing purposes.
MsixTestInstallerPath : The MSIX (or APPX) Installer executable under test.
MsiTestInstallerPath: The Msi Installer executable under test.
MsixTestInstallerPath: The MSIX (or APPX) Installer executable under test.
ExeTestInstallerPath: The Exe Installer executable under test.
PackageCertificatePath: Signing Certificate Path used to certify Index Source Package
-->
Expand All @@ -27,6 +28,7 @@
<Parameter name="LooseFileRegistration" value="false" />
<Parameter name="InvokeCommandInDesktopPackage" value="false" />
<Parameter name="StaticFileRootPath" value="\TestLocalIndex" />
<Parameter name="MsiTestInstallerPath" value="MsiTestInstaller.msi" />
<Parameter name="MsixTestInstallerPath" value="MsixTestInstaller.msix" />
<Parameter name="ExeTestInstallerPath" value="ExeTestInstaller.exe" />
<Parameter name="PackageCertificatePath" value="certificate.pfx"/>
Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCLITests/ARPChanges.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ struct TestContext : public Context
{
// Put installer in to control whether arp change code cares to run
Manifest::ManifestInstaller installer;
installer.InstallerType = installerType;
installer.BaseInstallerType = installerType;
Add<Data::Installer>(std::move(installer));

// Put in an empty manifest by default
Expand Down
4 changes: 2 additions & 2 deletions src/AppInstallerCLITests/ManifestComparator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const ManifestInstaller& AddInstaller(
{
ManifestInstaller toAdd;
toAdd.Arch = architecture;
toAdd.InstallerType = installerType;
toAdd.BaseInstallerType = installerType;
toAdd.Scope = scope;
toAdd.MinOSVersion = minOSVersion;
toAdd.Locale = locale;
Expand All @@ -63,7 +63,7 @@ void RequireInstaller(const std::optional<ManifestInstaller>& actual, const Mani
{
REQUIRE(actual);
REQUIRE(actual->Arch == expected.Arch);
REQUIRE(actual->InstallerType == expected.InstallerType);
REQUIRE(actual->EffectiveInstallerType() == expected.EffectiveInstallerType());
REQUIRE(actual->Scope == expected.Scope);
REQUIRE(actual->MinOSVersion == expected.MinOSVersion);
REQUIRE(actual->Locale == expected.Locale);
Expand Down
6 changes: 3 additions & 3 deletions src/AppInstallerCLITests/RestInterface_1_0.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ namespace
REQUIRE(actualInstaller.Platform.size() == 1);
REQUIRE(actualInstaller.Platform[0] == PlatformEnum::Desktop);
REQUIRE(actualInstaller.MinOSVersion == "1078");
REQUIRE(actualInstaller.InstallerType == InstallerTypeEnum::Msix);
REQUIRE(actualInstaller.BaseInstallerType == InstallerTypeEnum::Msix);
REQUIRE(actualInstaller.Scope == ScopeEnum::User);
REQUIRE(actualInstaller.SignatureSha256 == AppInstaller::Utility::SHA256::ConvertToBytes("011048877dfaef109801b3f3ab2b60afc74f3fc4f7b3430e0c897f5da1df84b6"));
REQUIRE(actualInstaller.InstallModes.size() == 1);
Expand Down Expand Up @@ -425,7 +425,7 @@ TEST_CASE("Search_Optimized_ManifestResponse", "[RestSource][Interface_1_0]")
REQUIRE(manifest.Installers.size() == 1);
REQUIRE(manifest.Installers[0].Arch == Architecture::X64);
REQUIRE(manifest.Installers[0].Sha256 == AppInstaller::Utility::SHA256::ConvertToBytes("011048877dfaef109801b3f3ab2b60afc74f3fc4f7b3430e0c897f5da1df84b6"));
REQUIRE(manifest.Installers[0].InstallerType == InstallerTypeEnum::Exe);
REQUIRE(manifest.Installers[0].BaseInstallerType == InstallerTypeEnum::Exe);
REQUIRE(manifest.Installers[0].Url == "https://installer.example.com/foobar.exe");
}

Expand Down Expand Up @@ -520,6 +520,6 @@ TEST_CASE("GetManifests_GoodResponse_UnknownInstaller", "[RestSource][Interface_
// Verify manifest is populated and manifest validation passed
Manifest manifest = manifests[0];
REQUIRE(manifest.Installers.size() == 1);
REQUIRE(manifest.Installers.at(0).InstallerType == InstallerTypeEnum::Unknown);
REQUIRE(manifest.Installers.at(0).BaseInstallerType == InstallerTypeEnum::Unknown);
REQUIRE(manifest.Installers.at(0).ProductId.empty());
}
Loading