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.

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

Use AppsAndFeaturesEntries DisplayVersion info for installed package version mapping #2213

Merged
merged 20 commits into from
Jun 10, 2022

Conversation

yao-msft
Copy link
Contributor

@yao-msft yao-msft commented Jun 2, 2022

For #980

Changes

Described in spec

Validation

Added unit and e2e tests. Did a local manual validation.

Microsoft Reviewers: Open in CodeFlow

@github-actions
Copy link

github-actions bot commented Jun 2, 2022

@check-spelling-bot Report

Unrecognized words, please review:

  • messasge
Previously acknowledged words that are now absent activatable amd Archs dsc enr FWW Globals hackathon lww mytool OSVERSION Packagedx parametermap symlink Uninitialize WDAG whatif wsb
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:yao-msft/winget-cli.git repository
on the arpversion branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/1145278660" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

@yao-msft yao-msft marked this pull request as ready for review June 3, 2022 20:01
@yao-msft yao-msft requested a review from a team as a code owner June 3, 2022 20:01
@@ -16,7 +16,7 @@ namespace AppInstaller::CLI::Workflow
{
bool IsUpdateVersionApplicable(const Utility::Version& installedVersion, const Utility::Version& updateVersion)
{
return (installedVersion < updateVersion || updateVersion.IsLatest());
return (installedVersion < updateVersion || (installedVersion.IsLatest() && updateVersion.IsLatest()));
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

Why does the installed version need to be Latest in order for the update version of Latest to be applicable? #Resolved


for (auto const& installer : Installers)
{
if (DoesInstallerTypeWriteAppsAndFeaturesEntry(installer.InstallerType) && installer.InstallerType != InstallerTypeEnum::Portable)
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

I would like another semantic function name here to encode the removal of portable, possibly something like DoesInstallerTypeSupportVersionRange.

Alternately, we should probably just move to a full on installer traits type to capture this information and require that all new installer types must have had this thought through. #Resolved

bool IsApproximateVersion() const { return m_approximateComparator != ApproximateComparator::None; }

// Static methods to create an approximate version from a base version.
static Version CreateLessThanApproximateVersion(const Version& base);
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

Why not add an ApproximateComparator parameter to the Version constructor? I think specifically Version(const Version&, ApproximateComparator) is what you would need. #Resolved

if (CaseInsensitiveStartsWith(m_version, s_Approximate_Less_Than))
{
m_approximateComparator = ApproximateComparator::LessThan;
baseVersion = m_version.substr(2, m_version.length() - 2);
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

Suggested change
baseVersion = m_version.substr(2, m_version.length() - 2);
baseVersion = m_version.substr(s_Approximate_Less_Than.length(), m_version.length() - s_Approximate_Less_Than.length());
``` #Resolved

@@ -48,9 +66,23 @@ namespace AppInstaller::Utility

bool Version::operator<(const Version& other) const
{
// If base versions are same, result is based on approximate comparator
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

This seems like a very costly way of doing the comparison (copies and doing full equality comparison first). Why not just move the approximate comparison resolution to the end when we have decided that the base is equal? Yes, it means refactoring the flow a bit to not return on equality, but it seems feasible.

For performance it would probably even be good to move the latest/unknown to be determined at Assign, which would remove the need to create temporaries here. Then the refactored flow would be able to be cleaner. #Resolved

@@ -314,6 +426,8 @@ namespace AppInstaller::Repository
std::shared_ptr<IPackage> m_availablePackage;
Source m_trackingSource;
std::shared_ptr<IPackage> m_trackingPackage;
mutable std::atomic_bool m_isOverrideInstalledVersionInitialized = false;
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

Why not just initialize it in the constructor; I don't see a lot of cases where the value is going to be ignored. #Resolved

{
THROW_HR_IF(E_UNEXPECTED, manifestColumnNames.size() != 0 && manifestColumnNames.size() != columns.size());
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

Suggested change
THROW_HR_IF(E_UNEXPECTED, manifestColumnNames.size() != 0 && manifestColumnNames.size() != columns.size());
THROW_HR_IF(E_UNEXPECTED, manifestColumnNames.size() != columns.size());

I don't think this code works if manifestColumnNames is not provided along with columns. #Resolved

@@ -247,6 +255,106 @@ extern "C"
}
CATCH_RETURN()

DEFINE_ENUM_FLAG_OPERATORS(WinGetValidateManifestResult);

WINGET_UTIL_API WinGetValidateManifestV3(
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

Add result of discussion WinGetUtil.h...

Should we make this a little more complicated from the number of APIs, but less complicated for each?

Imagine:

WinGetCreateManifest(path, message, merge path, option, manifest handle);
WinGetValidateManifestV3(manifest handle, index, result, message, operation, option);
WinGetCloseManifest(manifest handle);

The create is basically V2 validate, but we return a pointer to reuse for future validations. It performs the direct manifest validations only. The returned pointer saves continually parsing the manifest and enables efficient splitting of the various validations we perform. Then the V3 validate can be "perform this one validation". #Resolved

// Check arp version ranges do not overlap
if (result && sqliteIndex->GetVersion() >= Schema::Version{ 1, 5 })
{
result = ValidateArpVersionConsistency(sqliteIndex);
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

Why is this not part of CheckConsistency? #Resolved

// Only validate against json schema
SchemaValidation = 0x1,
// Validate against schema and also perform semantic validation
SchemaAndSemanticValidation = 0x2,
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 6, 2022

Choose a reason for hiding this comment

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

Shouldn't this just be SemanticValidation and you pass both flags to get both? #Pending

@@ -51,6 +51,9 @@ namespace AppInstaller::Manifest
const char* const ExceededAppsAndFeaturesEntryLimit = "Only zero or one entry for Apps and Features may be specified for InstallerType portable.";
const char* const ExceededCommandsLimit = "Only zero or one value for Commands may be specified for InstallerType portable.";
const char* const ScopeNotSupported = "Scope is not supported for InstallerType portable.";
const char* const ApproximateVersionNotAllowed = "Approximate version not allowed.";
const char* const ArpVersionOverlapWithIndex = "DisplayVersion declared in the manifest has overlap with existing DisplayVersion range in the index. Existing DisplayVersion range in index: ";
Copy link
Contributor

@Trenly Trenly Jun 7, 2022

Choose a reason for hiding this comment

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

This got me thinking - Are version ranges architecture specific, and/or should they be? I could see a scenario where a publisher uses the following -

PackageVersion: 1.0-Alpha # This is their marketing version
Installers: 
  - Architecture: x86
    AppsAndFeaturesEntries:
      - DisplayVersion: 1.0.4.0
- Architecture: x64
  AppsAndFeaturesEntries:
    - DisplayVersion: 1.0.5.0

According to the spec, this would result in an Version Range of [1.0.4.0, 1.0.5.0]

Suppose they release a new update -

PackageVersion: 1.1-Alpha # This is their marketing version
Installers: 
  - Architecture: x86
    AppsAndFeaturesEntries:
      - DisplayVersion: 1.0.4.1
- Architecture: x64
  AppsAndFeaturesEntries:
    - DisplayVersion: 1.0.5.1

According to the logic, wouldn't 1.0.4.1 overlap with [1.0.4.0, 1.0.5.0]. since 1.0.4.0 =< 1.0.4.1 =< 1.0.5.0, thereby triggering a false overlap warning? #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.

Correct, this will be treated as unsupported case according to the spec previously written. Since there's no restriction on what is written to DisplayVersion entry, an installer could also write different values for different locale. The above example can be extended to scope, installertype, etc. Essentially this will end up being each installer is in its own set.

@@ -51,6 +51,9 @@ namespace AppInstaller::Manifest
const char* const ExceededAppsAndFeaturesEntryLimit = "Only zero or one entry for Apps and Features may be specified for InstallerType portable.";
const char* const ExceededCommandsLimit = "Only zero or one value for Commands may be specified for InstallerType portable.";
const char* const ScopeNotSupported = "Scope is not supported for InstallerType portable.";
const char* const ApproximateVersionNotAllowed = "Approximate version not allowed.";
const char* const ArpVersionOverlapWithIndex = "DisplayVersion declared in the manifest has overlap with existing DisplayVersion range in the index. Existing DisplayVersion range in index: ";
const char* const ArpVersionValidationInternalError = "Encountered internal error during DisplayVersion validation against index.";
Copy link
Contributor

@Trenly Trenly Jun 7, 2022

Choose a reason for hiding this comment

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

Suggested change
const char* const ArpVersionValidationInternalError = "Encountered internal error during DisplayVersion validation against index.";
const char* const ArpVersionValidationInternalError = "Internal error while validating DisplayVersion against index.";

Nit: Passive voice #Resolved

@@ -33,6 +46,9 @@ namespace AppInstaller::Utility
Version(std::string(version), splitChars) {}
Version(std::string&& version, std::string_view splitChars = DefaultSplitChars);

// Constructing an approximate version from a base version.
Version(const Version& baseVersion, ApproximateComparator approximateComparator);
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 9, 2022

Choose a reason for hiding this comment

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

Suggested change
Version(const Version& baseVersion, ApproximateComparator approximateComparator);
Version(Version baseVersion, ApproximateComparator approximateComparator);

And move its contents. This gives us both copies and moves. #Resolved

@@ -122,8 +130,19 @@ namespace AppInstaller::Utility
// else parts are equal, so continue to next part
}

// All parts tested were equal, so this is only less if there are more parts in other.
return m_parts.size() < other.m_parts.size();
// All parts tested were equal
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 9, 2022

Choose a reason for hiding this comment

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

You have to change the Latest and Unknown comparisons to only return in the cases that they are not equal so that they can fall down here. Currently they return false in the equal case, since this is operator<. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, and unit tests failed for this... rushing to get a commit run in the pipeline before running tests locally

for (auto const& tuple : rawVersionValues)
{
auto& [version, arpMin, arpMax] = tuple;
if (!arpMin.empty() && !arpMax.empty() && arpMin != version && arpMax != version)
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 9, 2022

Choose a reason for hiding this comment

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

Suggested change
if (!arpMin.empty() && !arpMax.empty() && arpMin != version && arpMax != version)
if (!arpMin.empty() && !arpMax.empty() && (arpMin != version || arpMax != version))

It is ok if one of them is the package version.

Also, why not perform the test in the loop while building the strings out? #Resolved

// Construct a map between manifest version and arp version range. The map is ordered in descending by package version.
std::vector<std::pair<std::string, Utility::VersionRange>> arpVersionMap;
// Stores raw versions value strings to run a preliminary check whether version mapping is needed.
std::vector<std::tuple<std::string, std::string, std::string>> rawVersionValues;
auto versionKeys = availablePackage->GetAvailableVersionKeys();
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 9, 2022

Choose a reason for hiding this comment

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

I've just realized that if the available package is a composite of multiple sources, then this is flawed. Do we do that now or not? #Pending

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 think composite available package may need more thinking, currently we stop when we find one from a source, I'll add a comment as a reminder when we enable composite available package one day

@@ -254,6 +255,12 @@ namespace AppInstaller::Repository::Microsoft

bool result = m_interface->CheckConsistency(m_dbconn, log);

// Check arp version ranges do not overlap
if ((result || log) && GetVersion() >= Schema::Version{ 1, 5 })
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 9, 2022

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 done in the versioned code. Why is it not? #Resolved

WINGET_UTIL_API WinGetCreateManifest(
WINGET_STRING inputPath,
BOOL* succeeded,
WINGET_SQLITE_MANIFEST_HANDLE* manifest,
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 9, 2022

Choose a reason for hiding this comment

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

Suggested change
WINGET_SQLITE_MANIFEST_HANDLE* manifest,
WINGET_MANIFEST_HANDLE* manifest,
``` #Resolved

if (WI_IsFlagSet(option, WinGetValidateManifestOptionV2::SchemaValidation) || WI_IsFlagSet(option, WinGetValidateManifestOptionV2::SchemaAndSemanticValidation))
Manifest* manifestPtr = reinterpret_cast<Manifest*>(manifest);
SQLiteIndex* sqliteIndex = nullptr;
if (index)
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 9, 2022

Choose a reason for hiding this comment

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

You can just reinterpret_cast without the null check. #Resolved

// a string representing validation errors if validation failed.
// If mergedManifestPath is provided, this method will write a merged manifest
// to the location specified by mergedManifestPath
WINGET_UTIL_API WinGetValidateManifestV2(
Copy link
Member

@JohnMcPMS JohnMcPMS Jun 9, 2022

Choose a reason for hiding this comment

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

Suggested change
WINGET_UTIL_API WinGetValidateManifestV2(
WINGET_UTIL_API WinGetValidateManifestV3(

And change the parameters to match. #Resolved

@ghost ghost added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Jun 9, 2022
WINGET_UTIL_API WinGetCloseManifest(
WINGET_MANIFEST_HANDLE manifest);

// Validates a given manifest. Returns a bool for validation result and
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returns a bool for validation result and

update comments

installerType == InstallerTypeEnum::Nullsoft ||
installerType == InstallerTypeEnum::Wix ||
installerType == InstallerTypeEnum::Burn
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any advantage to using ||'s here, instead of a switch statement? I would think that when compiled, a switch statement would be slightly more efficient, given that it compiles as a jump table rather than a sequential evaluation

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 just copied from above similar functions. I guess it's more readable. In the end, this function will be invoked at most once for each installed package so I think I'll leave it for consistency for now.

m_maxVersion = std::move(maxVersion);
}

bool VersionRange::HasOverlapWith(const VersionRange& other) const
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool VersionRange::HasOverlapWith(const VersionRange& other) const
bool VersionRange::Overlaps(const VersionRange& other) const

Why use many word, when few word do trick?

@@ -75,9 +91,48 @@ namespace AppInstaller::Utility
// Gets the part breakdown for a given version; used for tests.
const std::vector<Part>& GetParts() const { return m_parts; }

// Returns if the version is an approximate version.
bool IsApproximateVersion() const { return m_approximateComparator != ApproximateComparator::None; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool IsApproximateVersion() const { return m_approximateComparator != ApproximateComparator::None; }
bool IsApproximate() const { return m_approximateComparator != ApproximateComparator::None; }

Why use many word, when few word do trick?

Comment on lines +106 to +107
bool thisIsUnknown = IsBaseVersionUnknown();
bool otherIsUnknown = other.IsBaseVersionUnknown();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to change this, since IsUnknown() calls IsBaseVersionUnkown()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a previous commit, approximate to Unknown is actually supported. I disabled it after internal discussion about what we would do to Unknown. I kept it for parity. And in case we need it in the future.

return m_minVersion <= other.m_maxVersion && m_maxVersion >= other.m_minVersion;
}

bool VersionRange::IsSameAsSingleVersion(const Version& version) const
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this could be an operator overload of == instead of a separate method?

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 don't feel like doing == for different objects, which may cause confusion IMO.

@yao-msft yao-msft merged commit db2014f into microsoft:master Jun 10, 2022
@yao-msft yao-msft deleted the arpversion branch June 10, 2022 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants