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

Make exe missing silent switch a warning and add tool info #93

Merged
merged 4 commits into from
Apr 30, 2020
Merged

Make exe missing silent switch a warning and add tool info #93

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee7987d
exe silent switch
yao-msft Apr 29, 2020
d4974ce
Add tool info
yao-msft Apr 29, 2020
beec8ee
Merge remote-tracking branch 'upstream/master' into skipexesilentswitch
yao-msft Apr 29, 2020
75ce91e
PR feedback
yao-msft Apr 29, 2020
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
5 changes: 3 additions & 2 deletions src/AppInstallerCLICore/Command.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ namespace AppInstaller::CLI
void Command::OutputIntroHeader(Execution::Reporter& reporter) const
{
reporter.Info() <<
"AppInstaller Command Line v" << Runtime::GetClientVersion() << std::endl <<
"Copyright (c) Microsoft Corporation" << std::endl;
"Windows Package Manager" << std::endl <<
Copy link
Contributor

@mapill-msft mapill-msft Apr 29, 2020 •
edited by yao-msft
Loading

Choose a reason for hiding this comment

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

Shouldn't this be WinGet? #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the string from the bug description. I guess our official name is still Windows Package Manager?

In reply to: 417084682 [](ancestors = 417084682)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll follow up with Kevin to confirm

In reply to: 417589933 [](ancestors = 417589933,417084682)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kevin confirmed Windows Package Manager is the correct name

In reply to: 417590437 [](ancestors = 417590437,417589933,417084682)

" Version: " << Runtime::GetClientVersion() << std::endl <<
" Copyright: Copyright (c) Microsoft Corporation. All rights reserved." << std::endl;
Copy link
Member

@JohnMcPMS JohnMcPMS Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't like the change to this format. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, the goal of the change was not to affect this, but to add --info. Please revert your format changes to this part.

In reply to: 417634511 [](ancestors = 417634511)

}

void Command::OutputHelp(Execution::Reporter& reporter, const CommandException* exception) const
Expand Down
16 changes: 14 additions & 2 deletions src/AppInstallerCLICore/Commands/RootCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ namespace AppInstaller::CLI
return
{
Argument{ "version", 'v', Execution::Args::Type::ListVersions, LOCME("Display the version of the tool"), ArgumentType::Flag, Visibility::Help },
Argument{ "info", APPINSTALLER_CLI_ARGUMENT_NO_SHORT_VER, Execution::Args::Type::Info, LOCME("Display general info of the tool"), ArgumentType::Flag, Visibility::Help },
};
}

Expand All @@ -40,9 +41,20 @@ namespace AppInstaller::CLI

void RootCommand::ExecuteInternal(Execution::Context& context) const
{
if (context.Args.Contains(Execution::Args::Type::ListVersions))
if (context.Args.Contains(Execution::Args::Type::Info))
{
context.Reporter.Info() << 'v' << Runtime::GetClientVersion() << std::endl;
OutputIntroHeader(context.Reporter);

context.Reporter.Info() << std::endl <<
"Links:" << std::endl <<
" Privacy Statement: https://aka.ms/winget-privacy" << std::endl <<
Copy link
Member

@JohnMcPMS JohnMcPMS Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

[](start = 17, length = 4)

We typically only use 2 spaces for indentation. I would stick with that here. #Closed

" License agreement: https://aka.ms/winget-license" << std::endl <<
" 3rd Party Notices: https://aka.ms/winget-3rdPartyNotice" << std::endl <<
" Homepage: https://aka.ms/mspm" << std::endl;
Copy link
Contributor Author

@yao-msft yao-msft Apr 29, 2020 •
edited by JohnMcPMS
Loading

Choose a reason for hiding this comment

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

Homepage: https://aka.ms/mspm" [](start = 21, length = 31)

Kevin mentioned we should use aka.ms/winget #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is not correct.

In reply to: 417598643 [](ancestors = 417598643)

Copy link
Member

Choose a reason for hiding this comment

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

Also, given that the others line up nicely, I think it would look better if this URL also lined up.

In reply to: 417636108 [](ancestors = 417636108,417598643)

}
else if (context.Args.Contains(Execution::Args::Type::ListVersions))
{
context.Reporter.Info() << "Version: " << Runtime::GetClientVersion() << std::endl;
Copy link
Member

@JohnMcPMS JohnMcPMS Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

"Version: " [](start = 39, length = 11)

Don't change this either. #Closed

}
else
{
Expand Down
12 changes: 10 additions & 2 deletions src/AppInstallerCLICore/Commands/ValidateCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,20 @@ namespace AppInstaller::CLI

try
{
(void)Manifest::Manifest::CreateFromPath(inputFile, true);
(void)Manifest::Manifest::CreateFromPath(inputFile, true, true);
context.Reporter.Info() << "Manifest validation succeeded." << std::endl;
}
catch (const Manifest::ManifestException& e)
{
context.Reporter.Warn() << "Manifest validation failed." << std::endl;
if (e.IsWarningOnly())
{
context.Reporter.Info() << "Manifest validation succeeded with warnings." << std::endl;
Copy link
Member

@JohnMcPMS JohnMcPMS Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Info [](start = 37, length = 4)

Warn #Closed

}
else
{
context.Reporter.Warn() << "Manifest validation failed." << std::endl;
Copy link
Member

@JohnMcPMS JohnMcPMS Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Warn [](start = 37, length = 4)

Error #Closed

}

context.Reporter.Warn() << e.GetManifestErrorMessage() << std::endl;
Copy link
Member

@JohnMcPMS JohnMcPMS Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Warn [](start = 33, length = 4)

Either match the above, or just use Info. #Closed

}
};
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/ExecutionArgs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ namespace AppInstaller::CLI::Execution
PlainStyle, // Makes progress display as plain
RainbowStyle, // Makes progress display as a rainbow
Help, // Show command usage
Info, // Show general info about WinGet
};

bool Contains(Type arg) const { return (m_parsedArgs.count(arg) != 0); }
Expand Down
130 changes: 80 additions & 50 deletions src/AppInstallerCLITests/YamlManifest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,80 +122,110 @@ TEST_CASE("ReadGoodManifestWithSpaces", "[ManifestValidation]")
REQUIRE(manifest.FileExtensions == MultiValue{ "appx", "appxbundle", "msix", "msixbundle" });
}

void TestManifest(const std::filesystem::path& manifestPath, const std::string& expectedError = {})
struct ManifestExceptionMatcher : public Catch::MatcherBase<ManifestException>
{
if (expectedError.empty())
ManifestExceptionMatcher(std::string expectedMessage, bool expectedWarningOnly) :
m_expectedMessage(expectedMessage), m_expectedWarningOnly(expectedWarningOnly) {}

// Performs the test for this matcher
bool match(ManifestException const& e) const override
{
return e.GetManifestErrorMessage().find(m_expectedMessage) != std::string::npos &&
e.IsWarningOnly() == m_expectedWarningOnly;
}

virtual std::string describe() const override {
std::ostringstream ss;
ss << std::boolalpha << "Expected exception message: " << m_expectedMessage << "Expected IsWarningOnly: " << m_expectedWarningOnly;
return ss.str();
}

private:
std::string m_expectedMessage;
bool m_expectedWarningOnly;
};

void TestManifest(const std::filesystem::path& manifestPath, const std::string& expectedMessage = {}, bool expectedWarningOnly = false)
{
if (expectedMessage.empty())
{
CHECK_NOTHROW(Manifest::CreateFromPath(TestDataFile(manifestPath), true));
CHECK_NOTHROW(Manifest::CreateFromPath(TestDataFile(manifestPath), true, true));
}
else
{
CHECK_THROWS_WITH(Manifest::CreateFromPath(TestDataFile(manifestPath), true), Catch::Contains(expectedError));
CHECK_THROWS_MATCHES(Manifest::CreateFromPath(TestDataFile(manifestPath), true, true), ManifestException, ManifestExceptionMatcher(expectedMessage, expectedWarningOnly));
}
}

struct ManifestTestCase
{
std::string TestFile;
std::string ExpectedMessage = {};
bool IsWarningOnly = false;
};

TEST_CASE("ReadGoodManifests", "[ManifestValidation]")
{
std::string TestCases[] =
ManifestTestCase TestCases[] =
{
"Manifest-Good-InstallerTypeExeRoot-Silent.yaml",
"Manifest-Good-InstallerTypeExeRoot-SilentRoot.yaml",
"Manifest-Good-InstallerTypeExe-Silent.yaml",
"Manifest-Good-InstallerTypeExe-SilentRoot.yaml",
"Manifest-Good-Installeruniqueness-DefaultLang.yaml",
"Manifest-Good-Installeruniqueness-DiffLangs.yaml",
"Manifest-Good-InstallerUniqueness-DiffScope.yaml",
"Manifest-Good-Minimum.yaml",
"Manifest-Good-Minimum-InstallerType.yaml",
"Manifest-Good-Switches.yaml",
{ "Manifest-Good-InstallerTypeExeRoot-Silent.yaml" },
{ "Manifest-Good-InstallerTypeExeRoot-SilentRoot.yaml" },
{ "Manifest-Good-InstallerTypeExe-Silent.yaml" },
{ "Manifest-Good-InstallerTypeExe-SilentRoot.yaml" },
{ "Manifest-Good-Installeruniqueness-DefaultLang.yaml" },
{ "Manifest-Good-Installeruniqueness-DiffLangs.yaml" },
{ "Manifest-Good-InstallerUniqueness-DiffScope.yaml" },
{ "Manifest-Good-Minimum.yaml" },
{ "Manifest-Good-Minimum-InstallerType.yaml" },
{ "Manifest-Good-Switches.yaml" },
};

for (auto const& testCase : TestCases)
{
TestManifest(testCase);
TestManifest(testCase.TestFile);
}
}

TEST_CASE("ReadBadManifests", "[ManifestValidation]")
{
std::pair<std::string, std::string> TestCases[] =
ManifestTestCase TestCases[] =
{
{ "Manifest-Bad-ArchInvalid.yaml", "Manifest: Invalid field value. Field: Arch" },
{ "Manifest-Bad-ArchMissing.yaml", "Manifest: Required field missing. Field: Arch" },
{ "Manifest-Bad-Channel-NotSupported.yaml", "Manifest: Field is not supported. Field: Channel" },
{ "Manifest-Bad-DifferentCase-camelCase.yaml", "Manifest: All field names should be PascalCased. Field: installerType" },
{ "Manifest-Bad-DifferentCase-lower.yaml", "Manifest: All field names should be PascalCased. Field: installertype" },
{ "Manifest-Bad-DifferentCase-UPPER.yaml", "Manifest: All field names should be PascalCased. Field: INSTALLERTYPE" },
{ "Manifest-Bad-DuplicateKey.yaml", "Manifest: Duplicate field found in the manifest." },
{ "Manifest-Bad-DuplicateKey-DifferentCase.yaml", "Manifest: Duplicate field found in the manifest." },
{ "Manifest-Bad-DuplicateKey-DifferentCase-lower.yaml", "Manifest: Duplicate field found in the manifest." },
{ "Manifest-Bad-IdInvalid.yaml", "Manifest: Invalid field value. Field: Id" },
{ "Manifest-Bad-IdMissing.yaml", "Manifest: Required field missing. Field: Id" },
{ "Manifest-Bad-InstallersMissing.yaml", "Manifest: Required field missing. Field: Installers" },
{ "Manifest-Bad-InstallerTypeExe-NoSilent.yaml", "Manifest: Silent switches are required for InstallerType exe." },
{ "Manifest-Bad-InstallerTypeExe-NoSilentRoot.yaml", "Manifest: Silent switches are required for InstallerType exe." },
{ "Manifest-Bad-InstallerTypeExeRoot-NoSilent.yaml", "Manifest: Silent switches are required for InstallerType exe." },
{ "Manifest-Bad-InstallerTypeExeRoot-NoSilentRoot.yaml", "Manifest: Silent switches are required for InstallerType exe." },
{ "Manifest-Bad-InstallerTypeInvalid.yaml", "Manifest: Invalid field value. Field: InstallerType" },
{ "Manifest-Bad-InstallerTypeMissing.yaml", "Manifest: Invalid field value. Field: InstallerType" },
{ "Manifest-Bad-InstallerUniqueness.yaml", "Manifest: Duplicate installer entry found." },
{ "Manifest-Bad-InstallerUniqueness-DefaultScope.yaml", "Manifest: Duplicate installer entry found." },
{ "Manifest-Bad-InstallerUniqueness-DefaultValues.yaml", "Manifest: Duplicate installer entry found." },
{ "Manifest-Bad-InstallerUniqueness-SameLang.yaml", "Manifest: Duplicate installer entry found." },
{ "Manifest-Bad-NameMissing.yaml", "Manifest: Required field missing. Field: Name" },
{ "Manifest-Bad-PublisherMissing.yaml", "Manifest: Required field missing. Field: Publisher" },
{ "Manifest-Bad-Sha256Invalid.yaml", "Manifest: Invalid field value. Field: Sha256" },
{ "Manifest-Bad-Sha256Missing.yaml", "Manifest: Required field missing. Field: Sha256" },
{ "Manifest-Bad-SwitchInvalid.yaml", "Manifest: Unknown field. Field: NotASwitch" },
{ "Manifest-Bad-UnknownProperty.yaml", "Manifest: Unknown field. Field: Fake" },
{ "Manifest-Bad-UrlInvalid.yaml", "Manifest: Invalid field value. Field: Url" },
{ "Manifest-Bad-UrlMissing.yaml", "Manifest: Required field missing. Field: Url" },
{ "Manifest-Bad-VersionInvalid.yaml", "Manifest: Invalid field value. Field: Version" },
{ "Manifest-Bad-VersionMissing.yaml", "Manifest: Required field missing. Field: Version" },
{ "Manifest-Bad-ArchInvalid.yaml", "Invalid field value. Field: Arch" },
{ "Manifest-Bad-ArchMissing.yaml", "Required field missing. Field: Arch" },
{ "Manifest-Bad-Channel-NotSupported.yaml", "Field is not supported. Field: Channel" },
{ "Manifest-Bad-DifferentCase-camelCase.yaml", "All field names should be PascalCased. Field: installerType" },
{ "Manifest-Bad-DifferentCase-lower.yaml", "All field names should be PascalCased. Field: installertype" },
{ "Manifest-Bad-DifferentCase-UPPER.yaml", "All field names should be PascalCased. Field: INSTALLERTYPE" },
{ "Manifest-Bad-DuplicateKey.yaml", "Duplicate field found in the manifest." },
{ "Manifest-Bad-DuplicateKey-DifferentCase.yaml", "Duplicate field found in the manifest." },
{ "Manifest-Bad-DuplicateKey-DifferentCase-lower.yaml", "Duplicate field found in the manifest." },
{ "Manifest-Bad-IdInvalid.yaml", "Invalid field value. Field: Id" },
{ "Manifest-Bad-IdMissing.yaml", "Required field missing. Field: Id" },
{ "Manifest-Bad-InstallersMissing.yaml", "Required field missing. Field: Installers" },
{ "Manifest-Bad-InstallerTypeExe-NoSilent.yaml", "Silent and SilentWithProgress switches are not specified for InstallerType exe.", true },
{ "Manifest-Bad-InstallerTypeExe-NoSilentRoot.yaml", "Silent and SilentWithProgress switches are not specified for InstallerType exe.", true },
{ "Manifest-Bad-InstallerTypeExeRoot-NoSilent.yaml", "Silent and SilentWithProgress switches are not specified for InstallerType exe.", true },
{ "Manifest-Bad-InstallerTypeExeRoot-NoSilentRoot.yaml", "Silent and SilentWithProgress switches are not specified for InstallerType exe.", true },
{ "Manifest-Bad-InstallerTypeInvalid.yaml", "Invalid field value. Field: InstallerType" },
{ "Manifest-Bad-InstallerTypeMissing.yaml", "Invalid field value. Field: InstallerType" },
{ "Manifest-Bad-InstallerUniqueness.yaml", "Duplicate installer entry found." },
{ "Manifest-Bad-InstallerUniqueness-DefaultScope.yaml", "Duplicate installer entry found." },
{ "Manifest-Bad-InstallerUniqueness-DefaultValues.yaml", "Duplicate installer entry found." },
{ "Manifest-Bad-InstallerUniqueness-SameLang.yaml", "Duplicate installer entry found." },
{ "Manifest-Bad-NameMissing.yaml", "Required field missing. Field: Name" },
{ "Manifest-Bad-PublisherMissing.yaml", "Required field missing. Field: Publisher" },
{ "Manifest-Bad-Sha256Invalid.yaml", "Invalid field value. Field: Sha256" },
{ "Manifest-Bad-Sha256Missing.yaml", "Required field missing. Field: Sha256" },
{ "Manifest-Bad-SwitchInvalid.yaml", "Unknown field. Field: NotASwitch" },
{ "Manifest-Bad-UnknownProperty.yaml", "Unknown field. Field: Fake" },
{ "Manifest-Bad-UrlInvalid.yaml", "Invalid field value. Field: Url" },
{ "Manifest-Bad-UrlMissing.yaml", "Required field missing. Field: Url" },
{ "Manifest-Bad-VersionInvalid.yaml", "Invalid field value. Field: Version" },
{ "Manifest-Bad-VersionMissing.yaml", "Required field missing. Field: Version" },
};

for (auto const& testCase : TestCases)
{
TestManifest(testCase.first, testCase.second);
TestManifest(testCase.TestFile, testCase.ExpectedMessage, testCase.IsWarningOnly);
}
}
20 changes: 14 additions & 6 deletions src/AppInstallerRepositoryCore/Manifest/Manifest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ namespace AppInstaller::Manifest
return resultErrors;
}

Manifest Manifest::CreateFromPath(const std::filesystem::path& inputFile, bool fullValidation)
Manifest Manifest::CreateFromPath(const std::filesystem::path& inputFile, bool fullValidation, bool throwOnWarning)
{
Manifest manifest;
std::vector<ValidationError> errors;
Expand All @@ -170,19 +170,23 @@ namespace AppInstaller::Manifest
}
catch (const std::exception& e)
{
AICLI_LOG(YAML, Error, << "Failed to create manifest from file: " << inputFile.u8string());
THROW_EXCEPTION_MSG(ManifestException(), e.what());
}

if (!errors.empty())
{
THROW_EXCEPTION(ManifestException(std::move(errors)));
ManifestException ex{ std::move(errors) };

if (throwOnWarning || !ex.IsWarningOnly())
{
THROW_EXCEPTION(ex);
}
}

return manifest;
}

Manifest Manifest::Create(const std::string& input, bool fullValidation)
Manifest Manifest::Create(const std::string& input, bool fullValidation, bool throwOnWarning)
{
Manifest manifest;
std::vector<ValidationError> errors;
Expand All @@ -194,13 +198,17 @@ namespace AppInstaller::Manifest
}
catch (const std::exception& e)
{
AICLI_LOG(YAML, Error, << "Failed to create manifest: " << input);
THROW_EXCEPTION_MSG(ManifestException(), e.what());
}

if (!errors.empty())
{
THROW_EXCEPTION(ManifestException(std::move(errors)));
ManifestException ex{ std::move(errors) };

if (throwOnWarning || !ex.IsWarningOnly())
{
THROW_EXCEPTION(ex);
}
}

return manifest;
Expand Down
9 changes: 5 additions & 4 deletions src/AppInstallerRepositoryCore/Manifest/Manifest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,11 @@ namespace AppInstaller::Manifest

std::vector<ValidationError> PopulateManifestFields(const YAML::Node& rootNode, bool fullValidation);

// fullValidation Bool to set if manifest creation should perform extra validation that client does not need.
// e.g. Channel should be null. Client code does not need this check to work properly.
static Manifest CreateFromPath(const std::filesystem::path& inputFile, bool fullValidation = false);
// fullValidation: Bool to set if manifest creation should perform extra validation that client does not need.
// e.g. Channel should be null. Client code does not need this check to work properly.
// throwOnWarning: Bool to indicate if an exception should be thrown with only warnings detected in the manifest.
static Manifest CreateFromPath(const std::filesystem::path& inputFile, bool fullValidation = false, bool throwOnWarning = false);

static Manifest Create(const std::string& input, bool fullValidation = false);
static Manifest Create(const std::string& input, bool fullValidation = false, bool throwOnWarning = false);
};
}
2 changes: 1 addition & 1 deletion src/AppInstallerRepositoryCore/Manifest/ManifestInstaller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ namespace AppInstaller::Manifest
(Switches.find(InstallerSwitchType::SilentWithProgress) == Switches.end() ||
Switches.find(InstallerSwitchType::Silent) == Switches.end()))
{
resultErrors.emplace_back(ManifestError::ExeInstallerMissingSilentSwitches);
resultErrors.emplace_back(ManifestError::ExeInstallerMissingSilentSwitches, "", "", -1, -1, ValidationError::Level::Warning);
Copy link
Member

@JohnMcPMS JohnMcPMS Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

ManifestError::ExeInstallerMissingSilentSwitches, "", "", -1, -1, ValidationError::Level::Warning [](start = 42, length = 97)

An overload is nicer than pushing defaults out into the call sites. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Or if you do push defaults out, they should probably be through an indirection rather than the actual values.

In reply to: 417640546 [](ancestors = 417640546)

}

// Check empty string before calling IsValidUrl to avoid duplicate error reporting.
Expand Down
Loading