Fix list/upgrade table for packages with multiple ARP entries. #2137

jedieaston · 2022-05-03T18:21:56Z

Have you signed the Contributor License Agreement?
Are you working against an Issue?

Hooray! Packages with multiple ARP entries are being matched! However, there's a problem, which is that with the current way ReportListResult() works, the package's ARP name(s) are overwritten with the one from the manifest, which puts duplicate looking results in the list:

PS C:\Users\WDAGUtilityAccount> wingetdev list Mozilla
Name            Id              Version Available Source
--------------------------------------------------------
Mozilla Firefox Mozilla.Firefox 99.0.1  100.0     winget
Mozilla Firefox Mozilla.Firefox 99.0.1  100.0     winget
PS C:\Users\WDAGUtilityAccount> # Two Firefoxes at the same time? Highly dubious!

This PR changes ReportListResult() to only output a single row for every package ID by default (which means we only show one entry in the list per "winget package", even if multiple pieces of software were installed via that package). It also adds a new argument to winget list, --all, which will show all of the software installed (but now using the original name in the ARP table instead of the name of the package in the repo, to cause less confusion). Here's what it looks like:

PS C:\Users\easton> wingetdev list Mozilla Name Id Version Available Source -------------------------------------------------------- Mozilla Firefox Mozilla.Firefox 99.0.1 100.0 winget PS C:\Users\easton> wingetdev list Mozilla --all Name Id Version Available Source -------------------------------------------------------------------- Mozilla Firefox (x64 en-US) Mozilla.Firefox 99.0.1 100.0 winget Mozilla Maintenance Service Mozilla.Firefox 99.0.1 100.0 winget PS C:\Users\easton>

The table printed in winget upgrade is always the simpler table (since we can only upgrade things we matched anyway), and the count of "upgrades available" is mapped to the number of packages instead of the number of ARP entries.

~~I know I did this without opening an issue to discuss it, so if there's a better solution I'm happy to fix this (or close it).~~

Edit: If you're just joining us, @JohnMcPMS brought up some great points with my original solution, and as such this PR doesn't do as much as it did. Now, winget will always use the PackageName from the Add and Remove Programs table, instead of the one in the manifest:

PS C:\Users\easton> wingetdev list Mozilla
Name                        Id              Version Available Source
--------------------------------------------------------------------
Mozilla Firefox (x64 en-US) Mozilla.Firefox 99.0.1  100.0     winget
Mozilla Maintenance Service Mozilla.Firefox 99.0.1  100.0     winget
PS C:\Users\easton>

There is no argument to control this anymore, since if we tried to deduplicate the list with the currently available data we'd be hiding info (since winget isn't smart enough yet to uninstall all of the ARP entries associated with a certain package). Hopefully we'll be able to come back to that solution once the correlation logic is a bit more mature.

Tested: manually, although I'm happy to write tests if necessary.

Microsoft Reviewers: Open in CodeFlow

Trenly · 2022-05-03T19:43:50Z

src/AppInstallerCLICore/ExecutionArgs.h

@@ -81,6 +81,7 @@ namespace AppInstaller::CLI::Execution

            // Other
            All, // Used in Update command to update all installed packages to latest
+        	ListAll, // Used in List command to list all installed apps associated with a package


Why do you need to add a new arg here? Couldn't you reuse AppInstaller::CLI::Execution::Args::Type::All ?

AppInstaller::CLI::Execution::Args::Type::All is used in winget upgrade --all, where I wanted the simplified table to print out. Also, there's a different help string for this new argument, which would mean I'd have to have the Argument defined only in ListCommand instead of in the Arguments.cpp file with the rest of them...

(If we check for AppInstaller::CLI::Execution::Args::Type::All in ReportListResult(), we wouldn't be able to tell whether it was coming from winget upgrade --all or winget list --all. If others think we should be printing the full table on upgrade --all, then I can use the same argument in both places.)

Trenly · 2022-05-03T19:45:14Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp


                    if (latestVersion)
                    {
                        if (updateAvailable)
                        {
                            availableVersion = latestVersion->GetProperty(PackageVersionProperty::Version);
-                            availableUpgradesCount++;
+                            if (context.Args.Contains(Execution::Args::Type::ListAll) || !packageIdsPrinted.count(packageId))


Is there any possibility that there could be two packages with the same ID from different sources?

There is a possibility, yes. Should the set be a pair of the package ID and source?

I think that might prevent false positives

yes, PackageId + SourceId will ensure a unique "winget package"

yao-msft · 2022-05-03T20:10:16Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

+                    if (context.Args.Contains(Execution::Args::Type::ListAll))
+                    {
+                       table.OutputLine({
+							installedVersion->GetProperty(PackageVersionProperty::Name),


nit: a bunch of tabs should be replaced with spaces across the change

I think I caught all of them (sorry, ReSharper thought it was being helpful).

yao-msft · 2022-05-04T02:37:22Z

src/AppInstallerCLICore/Argument.cpp

@@ -63,6 +63,8 @@ namespace AppInstaller::CLI
            return Argument{ "msix", 'm', Args::Type::Msix, Resource::String::MsixArgumentDescription, ArgumentType::Flag };
        case Args::Type::ListVersions:
            return Argument{ "versions", NoAlias, Args::Type::ListVersions, Resource::String::VersionsArgumentDescription, ArgumentType::Flag };
+        case Args::Type::ListAll:


nit: since this is for list command specifically, this could be moved to ListCommand.cpp, just like the UpgradeCommand. This Argument::ForType was initially including every argument, then we switched to be "Argument::ForType is for common args, and command specific ones could just be defined in the specific command.cpp". Btw, upgrade all does not have an alias, should this be the same for consistency?

Fixed, and I removed the alias for consistency :)

yao-msft · 2022-05-04T02:50:24Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

@@ -710,6 +710,7 @@ namespace AppInstaller::CLI::Workflow
        int unknownPackagesCount = 0;
        auto &source = context.Get<Execution::Data::Source>();
        bool shouldShowSource = source.IsComposite() && source.GetAvailableSources().size() > 1;
+        std::set<Utility::LocIndString> packageIdsPrinted = std::set<Utility::LocIndString>();


Suggested change

std::set<Utility::LocIndString> packageIdsPrinted = std::set<Utility::LocIndString>();

std::set<Utility::LocIndString> packageIdsPrinted;

yao-msft · 2022-05-04T03:01:07Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

+                    }
+                    else
+                    {
+	                    // we need to only list once per package


nit: one more tab here :)

yao-msft · 2022-05-04T03:11:49Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -1308,4 +1308,7 @@ Please specify one of them using the `--source` option to proceed.</value>
  <data name="NoPackageSelectionArgumentProvided" xml:space="preserve">
    <value>No package selection argument was provided; see the help for details about finding a package.</value>
  </data>
+  <data name="ListAllArgumentDescription" xml:space="preserve">
+    <value>List all software installed by a package</value>


nit: we try to make "package" the terminology for software across winget but I can see it's weird to say "all packages installed by a package"

Maybe "List all package components installed by a package" or "List all sub-packages installed by a package"

Open to any suggestions.

@denelon, thoughts? I think "List all components installed by a package." makes the most sense.

I refer to these as "system artifacts" internally, but that probably isn't a great name for them publicly. I do like "components" or "items".

yao-msft · 2022-05-04T19:59:04Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

                        if (updateAvailable)
                        {
                            availableVersion = latestVersion->GetProperty(PackageVersionProperty::Version);
-                            availableUpgradesCount++;
+                            if (context.Args.Contains(Execution::Args::Type::ListAll) || !packageIdsPrinted.count({ packageId, sourceName }))


Sorry, I may not make clear in previous comment. SourceName is for display. SourceIdentifier should be used to identify a source. We should use value from PackageVersionProperty::SourceIdentifier for the duplicate check.

Resolved in latest commit!

yao-msft · 2022-05-04T21:44:26Z

/azp run

azure-pipelines · 2022-05-04T21:44:37Z

Azure Pipelines successfully started running 1 pipeline(s).

JohnMcPMS

I gather from the example case that multiple AppsAndFeaturesEntries are being used to associate multiple components of some software together. This change is then leveraging that to make it appear that we are being smarter than we are about handling that. The problem that I see is that targeting these installed items becomes even more confusing, as the default list behavior will look like there is just one "Mozilla Firefox".

However, if I want to uninstall it, this falls into the "we don't handle side by side versions well". I would expect that I cannot currently uninstall Mozilla.Firefox if both the maintenance service and primary application are correlated.

Ultimately, I think we need to solve the side by side and "multi-component package" issues by having the correlation engine actually output a single Package that has all of the details on its:

Multiple versions
Multiple components

Only then will the problem actually be solved. But that might also require updates to the manifest to be able to mark one of the components as "primary" so that we know what to uninstall (assuming that it handles removing the other components). And then of course all of the interaction models will need to be updated to handle targeting versions and components within as desired.

All of this isn't to say that your change is bad, just that I think it might end up hiding the problem that is created by the way the manifests are being constructed. I think it points to a good way to deal with things at the surface, but we really need the internals to be in agreement or we will end up with a mess.

JohnMcPMS · 2022-05-04T23:22:33Z

src/AppInstallerCLICore/Commands/ListCommand.cpp

@@ -24,6 +24,7 @@ namespace AppInstaller::CLI
            Argument::ForType(Execution::Args::Type::Exact),
            Argument::ForType(Execution::Args::Type::CustomHeader),
            Argument::ForType(Execution::Args::Type::AcceptSourceAgreements),
+            Argument{ "all", Argument::NoAlias, Execution::Args::Type::ListAll, Resource::String::ListAllArgumentDescription, ArgumentType::Flag},


I agree with @Trenly that this could just use Args::Type::All (especially now that its not in ForType [which existed pre-localization, but now I don't think we should use]).

Ultimately it's just another internal enum value, so it is more about how its makes us feel than anything else.

JohnMcPMS · 2022-05-04T23:27:36Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -1308,4 +1308,7 @@ Please specify one of them using the `--source` option to proceed.</value>
  <data name="NoPackageSelectionArgumentProvided" xml:space="preserve">
    <value>No package selection argument was provided; see the help for details about finding a package.</value>
  </data>
+  <data name="ListAllArgumentDescription" xml:space="preserve">
+    <value>List all software installed by a package</value>


I refer to these as "system artifacts" internally, but that probably isn't a great name for them publicly. I do like "components" or "items".

JohnMcPMS · 2022-05-04T23:40:01Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

@@ -730,27 +731,52 @@ namespace AppInstaller::CLI::Workflow
                // The only time we don't want to output a line is when filtering and no update is available.
                if (updateAvailable || !m_onlyShowUpgrades)
                {
-                    Utility::LocIndString availableVersion, sourceName;
+                    Utility::LocIndString availableVersion, sourceName, sourceIdentifier;
+                    Utility::LocIndString packageId = match.Package->GetProperty(PackageProperty::Id);


I would suggest that rather than storing the packageId and sourceIdentifier separately, just store the pair directly. That would remove 3 times where the strings will be copied to create a temporary pair.

JohnMcPMS · 2022-05-04T23:42:03Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

+                        // we need to only list once per package
+                        if (!packageIdsPrinted.count({ packageId, sourceIdentifier }))
+                        {
+                            packageIdsPrinted.insert({ packageId, sourceIdentifier });


Suggested change

packageIdsPrinted.insert({ packageId, sourceIdentifier });

packageIdsPrinted.emplace(std::move(thePair));

Assuming that you also took my other suggestion above about keeping these as a pair rather than separate. And you named it thePair, which I'm sure you would come up with a better name than that 😉

jedieaston · 2022-05-05T03:13:46Z

But that might also require updates to the manifest to be able to mark one of the components as "primary" so that we know what to uninstall (assuming that it handles removing the other components).

I completely spaced on this, as Firefox (the case I found the bug on) cleans up the maintenance service when you run the Firefox uninstaller. You're right though, winget needs to know if there's multiple uninstallers to run (and run them all). There's definitely other software (Autodesk stuff comes to mind) that make you uninstall each and every thing they add to the ARP table separately, so winget would need to know to execute all of the uninstallers associated with the ARP entries in the manifest.

Should we table this for now then until the side-by-side/correlation work is done? I don't know if that's scheduled to happen in 1.3 or not, but if it's post 1.3 then I think there needs to be some fix for this table, since currently it's super confusing. (Maybe just no matter what print the ARP name for now, so the user can always know what name to tell winget uninstall?

JohnMcPMS · 2022-05-05T17:56:58Z

Scheduled and "can fit in with all the random stuff going on" can be quite different 😄 I do like the idea of outputting a disambiguation and the name does seem like the best place for that. The issue with removing the 2nd through nth items will be that maybe one of those was the "real" one that you should uninstall. Until we know which one that is, I think we do want to just output all of them.

JohnMcPMS · 2022-05-11T20:40:28Z

@jedieaston, are you planning to convert this PR based on our discussion? Given the current situation I think we need to at least switch to using the installed version's name value for list. If you aren't I can make a PR for that purpose.

jedieaston · 2022-05-12T17:13:11Z

I am, let me try to get to it today. Sorry for the delay, I had to graduate from college over the weekend :)

JohnMcPMS · 2022-05-12T17:31:15Z

I am, let me try to get to it today. Sorry for the delay, I had to graduate from college over the weekend :)

🎊 Congratulations! 🎊

Oh no, I've dropped this https://careers.microsoft.com/us/en

Not trying to get you to do it, just didn't want to duplicate work if you were already doing it.

github-actions · 2022-05-13T05:31:55Z

@check-spelling-bot Report

Unrecognized words, please review:

Deduplicate

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 [email protected]:jedieaston/winget-cli.git repository
on the this-is-getting-out-of-hand-now-there-are-n-of-them 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/1125671967" > "$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

jedieaston · 2022-05-13T05:39:48Z

@JohnMcPMS, I removed all of the code regarding the argument (wonderful thing about Git is that I can get that work back later), and now all this PR does is use the name in the ARP table in the list/upgrade table (the easiest solution to the current problem). Once this merges, I can write up an issue to talk about the deduplication stuff.

JohnMcPMS · 2022-05-13T22:24:39Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

@@ -731,26 +731,32 @@ namespace AppInstaller::CLI::Workflow
                if (updateAvailable || !m_onlyShowUpgrades)
                {
                    Utility::LocIndString availableVersion, sourceName;
+                    Utility::LocIndString packageId = match.Package->GetProperty(PackageProperty::Id);


Getting value into packageId but not using it.

Resolved in latest commit. Oops!

JohnMcPMS · 2022-05-13T22:24:56Z

/azp run

azure-pipelines · 2022-05-13T22:25:05Z

Azure Pipelines successfully started running 1 pipeline(s).

…ut-of-hand-now-there-are-n-of-them

JohnMcPMS · 2022-05-16T23:23:59Z

/azp run

azure-pipelines · 2022-05-16T23:24:08Z

Azure Pipelines successfully started running 1 pipeline(s).

Fix list/upgrade table for packages with multiple ARP entries.

ca93379

jedieaston requested a review from a team as a code owner May 3, 2022 18:21

Trenly reviewed May 3, 2022

View reviewed changes

yao-msft reviewed May 3, 2022

View reviewed changes

Replaced tabs with spaces, sorry.

07521bf

yao-msft reviewed May 4, 2022

View reviewed changes

Now using a pair of the ID and source for deduping.

499c008

jedieaston force-pushed the this-is-getting-out-of-hand-now-there-are-n-of-them branch from d25e17e to 0646145 Compare May 4, 2022 12:52

Fixed issues from @yao-msft's code review.

2959a36

jedieaston force-pushed the this-is-getting-out-of-hand-now-there-are-n-of-them branch from 0646145 to 2959a36 Compare May 4, 2022 13:06

yao-msft reviewed May 4, 2022

View reviewed changes

Source identifier, not source name!

589b6d8

JohnMcPMS reviewed May 4, 2022

View reviewed changes

Trenly mentioned this pull request May 11, 2022

Multiple same name packages appeared right after the package upgrade #2152

Closed

Removed argument. Now we always output the ARP table name.

51b20ec

jedieaston force-pushed the this-is-getting-out-of-hand-now-there-are-n-of-them branch from 4268674 to 51b20ec Compare May 13, 2022 05:36

JohnMcPMS reviewed May 13, 2022

View reviewed changes

jedieaston added 2 commits May 15, 2022 16:17

Removed unused variable (oops!)

8543c1d

Merge remote-tracking branch 'upstream/master' into this-is-getting-o…

7089b6b

…ut-of-hand-now-there-are-n-of-them

JohnMcPMS approved these changes May 18, 2022

View reviewed changes

JohnMcPMS merged commit 8ded03a into microsoft:master May 18, 2022

jedieaston deleted the this-is-getting-out-of-hand-now-there-are-n-of-them branch May 18, 2022 18:32

jedieaston mentioned this pull request May 24, 2022

Track "primary" AppsAndFeaturesEntry for better matching #2186

Open

	std::set<Utility::LocIndString> packageIdsPrinted = std::set<Utility::LocIndString>();
	std::set<Utility::LocIndString> packageIdsPrinted;

	packageIdsPrinted.insert({ packageId, sourceIdentifier });
	packageIdsPrinted.emplace(std::move(thePair));

Fix list/upgrade table for packages with multiple ARP entries. #2137

Fix list/upgrade table for packages with multiple ARP entries. #2137

Conversation

jedieaston commented May 3, 2022 • edited Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

jedieaston May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedieaston May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yao-msft commented May 4, 2022

azure-pipelines bot commented May 4, 2022

JohnMcPMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedieaston commented May 5, 2022

JohnMcPMS commented May 5, 2022

JohnMcPMS commented May 11, 2022

jedieaston commented May 12, 2022

JohnMcPMS commented May 12, 2022

github-actions bot commented May 13, 2022 • edited Loading

@check-spelling-bot Report

Unrecognized words, please review:

jedieaston commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnMcPMS commented May 13, 2022

azure-pipelines bot commented May 13, 2022

JohnMcPMS commented May 16, 2022

azure-pipelines bot commented May 16, 2022

jedieaston commented May 3, 2022 •

edited

Loading

jedieaston May 3, 2022 •

edited

Loading

jedieaston May 4, 2022 •

edited

Loading

github-actions bot commented May 13, 2022 •

edited

Loading