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

Expose deeper installation detection through Com #2420

Merged
merged 19 commits into from
Sep 19, 2022

Conversation

yao-msft
Copy link
Contributor

@yao-msft yao-msft commented Aug 5, 2022

null

Microsoft Reviewers: Open in CodeFlow

@yao-msft yao-msft requested a review from a team as a code owner August 5, 2022 00:06
#define WINGET_INSTALLED_STATUS_ARP_ENTRY_FOUND S_OK
#define WINGET_INSTALLED_STATUS_ARP_ENTRY_NOT_FOUND ((HRESULT)0x8A150201)
#define WINGET_INSTALLED_STATUS_INSTALL_LOCATION_FOUND S_OK
#define WINGET_INSTALLED_STATUS_INSTALL_LOCATION_NOT_APPLICABLE ((HRESULT)0x8A150202)
Copy link
Member

@JohnMcPMS JohnMcPMS Aug 11, 2022

Choose a reason for hiding this comment

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

Should these actually be success codes when it is a case of not full success? (not have the error bit set) #Resolved

@@ -347,6 +347,9 @@ namespace AppInstaller::Repository::Microsoft
// Pick up Language to enable proper selection of language for upgrade.
AddMetadataIfPresent(arpKey, Language, index, manifestId, PackageVersionMetadata::InstalledLocale);

// Set installed architecture info.
index.SetMetadataByManifestId(manifestId, PackageVersionMetadata::InstalledArchitecture, architecture);
Copy link
Member

@JohnMcPMS JohnMcPMS Aug 11, 2022

Choose a reason for hiding this comment

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

You cannot pull this out of portable; we do not know the actual architecture of the installed package based on the ARP registry location. Even for portable it is dubious, and we should record it into an ARP value (due to ARM64). #Resolved

@@ -270,6 +318,9 @@ namespace AppInstaller::Repository
// Gets a value indicating whether an available version is newer than the installed version.
virtual bool IsUpdateAvailable() const = 0;

// Checks installed status of the package.
virtual std::vector<InstallerInstalledStatus> CheckInstalledStatus(InstalledStatusType types = InstalledStatusType::AllChecks) const = 0;
Copy link
Member

@JohnMcPMS JohnMcPMS Aug 11, 2022

Choose a reason for hiding this comment

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

I'm not sure that this needs to be implemented as a member of IPackage versus a standalone function that takes in an IPackage and the other parameters. What is gained from enabling different IPackage implementations to have different implementations of this function? #Resolved


/// The package installer scope.
[contract(Microsoft.Management.Deployment.WindowsPackageManagerContract, 5)]
enum PackageInstallerScope
Copy link
Member

@JohnMcPMS JohnMcPMS Aug 11, 2022

Choose a reason for hiding this comment

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

There is already a scope enum. #ByDesign

{
bool operator()(const std::filesystem::path& a, const std::filesystem::path& b) const
{
if (std::filesystem::equivalent(a, b))
Copy link
Member

@JohnMcPMS JohnMcPMS Aug 12, 2022

Choose a reason for hiding this comment

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

This part seems unnecessary if the goal is producing a < b. #ByDesign

fileStatus = WINGET_INSTALLED_STATUS_FILE_FOUND_WITHOUT_HASH_CHECK;
if (!expectedHash.empty())
{
if (fileHashes.find(filePath) == fileHashes.end())
Copy link
Member

@JohnMcPMS JohnMcPMS Aug 12, 2022

Choose a reason for hiding this comment

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

If we are going to have a map for performance, then you should:

auto itr = find();
if (itr != end)
{
itr = emplace().first;
}

AreEqual(expected, itr->second);

To prevent any more map lookups than necessary. #Resolved

@github-actions
Copy link

github-actions bot commented Aug 17, 2022

@check-spelling-bot Report

Unrecognized words, please review:

  • IObject
  • IWin
To accept these unrecognized words as correct, run the following commands

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

update_files() {
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/1218494533" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

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

Copy link
Member

@JohnMcPMS JohnMcPMS left a comment

Choose a reason for hiding this comment

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

Ensure that consumers are going to be happy with the API design before merging.

@yao-msft yao-msft merged commit a92346d into microsoft:master Sep 19, 2022
@yao-msft yao-msft deleted the detectionimpl branch September 19, 2022 23: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
Development

Successfully merging this pull request may close these issues.

2 participants