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

Draft of List, Upgrade and Uninstall. #397

Closed
wants to merge 13 commits into from

Conversation

KevinLaMS
Copy link
Contributor

@KevinLaMS KevinLaMS commented Jun 4, 2020

This includes issue 120, 121 and 119.

Microsoft Reviewers: Open in CodeFlow

@KevinLaMS KevinLaMS requested a review from a team as a code owner June 4, 2020 05:41
Apps that are free, will be downloaded directly when requested.

***Pay apps [NOT SUPPORTED]***
Apps that are for purchase, will not be allowed to be submitted. If an app from the store requires commerce, to run, we will not allow them in the Windows Package Manager repository.
Copy link

Choose a reason for hiding this comment

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

What about paid apps that have been purchased already?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the entitlement is already granted, I think we can continue with installation.

@megamorf
Copy link

megamorf commented Jun 4, 2020

@KevinLaMS Some general feedback.

Based on the contents on these drafts you haven't worked with Markdown that much so here are some potential issues that can be solved fairly quickly:

  • When using inline or fenced code formatting you don't need to escape characters (e.g. angle brackets) with a backslash `winget list [[-q] <query>] [<options>]`
  • Instead of using HTML tags for formatting try to stick with the markdown versions, e.g. **bold string** instead of <b>bold string</b>
  • All path references in markdown use a regular slash as path separator
  • lists, headings and tables should be surrounded by new lines

See: https://www.markdownguide.org/basic-syntax/

I like where this is going and will add more feedback later. 👍

Copy link

@fmeyertoens fmeyertoens left a comment

Choose a reason for hiding this comment

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

Just a few typos.

doc/specs/#117-Windows-Store-Support.md Outdated Show resolved Hide resolved
doc/specs/#117-Windows-Store-Support.md Outdated Show resolved Hide resolved
doc/specs/#120,121,119-Update-Uninstall-List.md Outdated Show resolved Hide resolved
doc/specs/#120,121,119-Update-Uninstall-List.md Outdated Show resolved Hide resolved
doc/specs/#120,121,119-Update-Uninstall-List.md Outdated Show resolved Hide resolved
doc/specs/#120,121,119-Update-Uninstall-List.md Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Jun 4, 2020
Copy link

@martinmine martinmine left a comment

Choose a reason for hiding this comment

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

Some comments from the discussion in #120


## Abstract

For our preview of the Windows Package Manager, the goal was to enable install of apps. In order to be a package manager, the Windows Package Manager must be able to list all installed packages, communicate if there is an update ane uninstall apps.

Choose a reason for hiding this comment

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

Small typo:

Suggested change
For our preview of the Windows Package Manager, the goal was to enable install of apps. In order to be a package manager, the Windows Package Manager must be able to list all installed packages, communicate if there is an update ane uninstall apps.
For our preview of the Windows Package Manager, the goal was to enable install of apps. In order to be a package manager, the Windows Package Manager must be able to list all installed packages, communicate if there is an update and uninstall apps.

| **-e,--exact** | Find the application using exact match. |


*Upgrade app when no update is available*

Choose a reason for hiding this comment

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

Another aspect to consider: What would happen if the user updates an app that is not installed? Should it then display an error or install said app?



*Upgrade app when no update is available*
If the user attempts to update an app that there is no known update for, the package manager will issue an error.

Choose a reason for hiding this comment

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

Should winget exit with a non-successful status code if this is the case?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I believe so. If the command requested cannot be executed, the process should return a failure exit code.

Copy link

Choose a reason for hiding this comment

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

This seems counter intuitive to me, and it's also not what Linux package managers do as far as I know (using pacman here). If the purpose of the upgrade command is to check if newer version of package(s) are available and get them, then it didn't fail if the last available package(s) are installed already. The output should be different (f.e. "there is nothing to do") but the command would be considered a success anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it comes down to what the purpose is deemed to be semantically. I am not opposed to the purpose being "Make sure I have the latest version of this", which then I agree with you that no error is correct. I was reading it more as "Get a newer version of this", which while I agree is a subtle difference, it is still the difference between a command that cannot be executed, versus one that can.

I suppose there is the question of what is done in the --all case. In that case, should there be an error if no updates are available for anything? That feels wrong to me, as I don't think anyone would ever say "Get a newer version of everything". So for consistency, I believe that the semantics should be Ensure, not Get.

So I agree that there should not be an error exit code, and the verbage here should be "issue a warning" not an error.


[comment]: # Outline the design of the solution. Feel free to include ASCII-art diagrams, etc.

## UI/UX Design

Choose a reason for hiding this comment

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

A suggestion for what could be discussed here: The impact on users that have not used a package manager, will they naturally use update instead of upgrade (or sometimes have issues with distinguishing these two)? And on the other hand, if update was an alias for upgrade, how can similar functionality of upgrade be used with the alias and vice versa? (For example: #120 (comment)) It would be really nice to see what is the plan for dealing with this kind of complexity (update vs. upgrade) from a user standpoint.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer "update'. Upgrade has other semantics (at least to me), and update fits more cleanly into what is happening.

Choose a reason for hiding this comment

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

@JohnMcPMS If you decide to go with update, then I would suggest to make upgrade alias of update to make the life easier for users familiar with other package managers that use upgrade for updating packages.

Copy link

@RobGThai RobGThai Jul 12, 2020

Choose a reason for hiding this comment

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

Brew user here, every time I want to update an app, I have to look up help to see if I need to update or upgrade. One is making sure Brew is up to date, the other is for updating the app. If you trigger update without supplying app identifier then Brew will attempt to update every installed packages. Which I'm not a fan of.

Personally, I'm against making alias between update and upgrade. Having just one version of them at the start should reduce confusion on which one to use for newcomers. Instead of doing this, I'd suggest WinGet to only offer just one.

  • winget update - displaying help for update.
  • winget update <package_id> - updating provided package.
  • winget update winget - updating WinGet itself.
  • winget update -a|--all - updating every installed packages.

Choose a reason for hiding this comment

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

I think winget should use keywork like apt of Debian.

  • Update: for refresh the catalog
  • Upgrade: for actual installation operation of new version

This way we could remove the refresh frequency as winget user does not expect winget run in background as a CLI tool usually run and exit


![show command]()

**Upgrade** when executed by itself will upgrade all apps ready for updates by the Windows Package Manager.

Choose a reason for hiding this comment

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

Would the user be prompted to confirm the package update? If yes, should there be flag to override this when being used in automation tasks, similar to how other package managers does this?

Copy link

Choose a reason for hiding this comment

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

Sounds like a good idea to stick to that already established pattern where by default a confirmation is required to proceed that can be skipped by passing an additional parameter.


The following options are available.

| Option | Description |

Choose a reason for hiding this comment

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

What about specifying a manifest file that is located on the user's machine (or another repository)? Would this be part of another spec?


For inno, wix, nullsoft, and exe, the SystemAppId should be a string that is located in either of the Uninstall keys above.

### Upgrade

Choose a reason for hiding this comment

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

It would be nice to hear how syncing with the remote repository will be handled, will the user have to do this manually (i.e. using an update), or will it be done automatically if the repository hasn't been synced after a said time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Martin, I am curious. I think that the syncing of the remote repository is handled via the source:
C:\Users\kevinla>winget source update
Updating all sources...
Updating source: winget...
██████████████████████████████ 100%
Done.

Do you think this addresses the Update concern?

Choose a reason for hiding this comment

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

It is true you can handle this using the winget source command. I just think the spec should outline how the syncing of the remote repo is handled in context of running an update. Is a user required to run an winget source update before running winget upgrade or will this be implicit from running winget upgrade? (i.e. upgrade will invoke source update)

Copy link

@megamorf megamorf Jun 5, 2020

Choose a reason for hiding this comment

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

In Linux package managers update the package cache for all enabled repositories. If a package was installed from repository "foo" then the search for available updates for that specific application will be narrowed down to repository "foo". If you'd actually want to install a new package that is available in multiple repositories without explicitly providing the correct repository name, the package will be installed from the repository with the highest priority. This priority system is also how the upcoming version 3 of PowerShellGet has been designed to define repository ordering.

Copy link

Choose a reason for hiding this comment

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

As of now, whenever a source (SQLite DB) is opened, it updates the source if it's been more than 5 minutes. winget source update is just the explicit command to update.

// TODO: Enable some amount of user control over this.
constexpr static auto s_DefaultAutoUpdateTime = 5min;

**Update \ Upgrade**
**Uninstall**

### List

Choose a reason for hiding this comment

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

Handling available updates with this would be similar to how dotnet handles packages updates, nice 👍 https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-list-package

Co-authored-by: Fabian Meyertöns <26410884+fmeyertoens@users.noreply.github.com>
@ghost ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Jun 4, 2020
KevinLaMS and others added 6 commits June 4, 2020 09:50
Co-authored-by: Fabian Meyertöns <26410884+fmeyertoens@users.noreply.github.com>
Co-authored-by: Fabian Meyertöns <26410884+fmeyertoens@users.noreply.github.com>
Co-authored-by: Fabian Meyertöns <26410884+fmeyertoens@users.noreply.github.com>
Co-authored-by: Fabian Meyertöns <26410884+fmeyertoens@users.noreply.github.com>
Co-authored-by: Fabian Meyertöns <26410884+fmeyertoens@users.noreply.github.com>
@@ -0,0 +1,230 @@
---
author: <first-name> <last-name> <github-id>/<email>
Copy link
Member

Choose a reason for hiding this comment

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

Update metadata.

---

# Spec Title

Copy link
Member

Choose a reason for hiding this comment

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

Add links to Issues per latest spec template.


![list command](Images\120-List.png)

**List** when executed by itself will show all apps installed by the Windows Package Manager.
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually care about differentiating things installed by winget vs things that weren't? Doesn't seem very useful to me.

| **--moniker** | Filter results by application moniker. |
| **--tag** | Filter results by tag
| **--command** | Filter results by command
| **-s,--source** | Find the application using the specified [source](source.md). |
Copy link
Member

Choose a reason for hiding this comment

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

Not relevant.



## Mapping between Windows Package Manager repo and Apps and Features
*StoreApps*
Copy link
Member

Choose a reason for hiding this comment

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

MSIX. Store is potentially a whole other can of worms. And we might have things available via both (Terminal for instance).


[comment]: # Outline the design of the solution. Feel free to include ASCII-art diagrams, etc.

## UI/UX Design
Copy link
Member

Choose a reason for hiding this comment

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

I prefer "update'. Upgrade has other semantics (at least to me), and update fits more cleanly into what is happening.



*Upgrade app when no update is available*
If the user attempts to update an app that there is no known update for, the package manager will issue an error.
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I believe so. If the command requested cannot be executed, the process should return a failure exit code.


## Usage

```winget uninstall [[-q] <query>] [<options>]```
Copy link
Contributor

Choose a reason for hiding this comment

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

Does uninstall handle apps installed outside of WinGet?
If there're multiple matches with the query. Should we return a warning message saying "please refine the input ..." like what install command does today?

| Argument | Description |
|--------------|-------------|
| **-q,--query** | The query used to search for an application. |
| **-?, --help** | Gets additional help on this command. |

Choose a reason for hiding this comment

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

Not a huge deal, but I'd personally prefer the -h alias as it is far more common and more clearly maps to --help than -?

Choose a reason for hiding this comment

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

Isn't -? the standard on Windows while -h is more a Unix thing?

Copy link
Contributor

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

@KevinLaMS: can you please pull master and then rebase this (start with a temporary branch -- see below)? The spell checker will be upset if this merges as is.

Most of the items it found just need to be added to expect.txt.
Ideally you'd only add words in this manner and you'd use patterns.txt for random strings (I've highlighted some random strings here). But if there's no pattern that works usefully (the random BigId that appears in multiple contexts is a good example), then using expect.txt is a good choice.

Note: you can test on another branch and only force push to this branch once the spell checker is happy.

Supporting the Windows store apps adds a bunch of complexity. Among other things, developers when submitting their applications to the store agreed to certain terms that did not include the Windows Package Manager. There we will take a phased approach to enabling store support.

### Phase 1 ###
Phase 1 will be kick the tires. The Windows Package Manager will only install apps that meet the following critieria:
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
Phase 1 will be kick the tires. The Windows Package Manager will only install apps that meet the following critieria:
Phase 1 will be kick the tires. The Windows Package Manager will only install apps that meet the following criteria:

This spec identifies requirements and implementation for supporting Windows Store apps in the Windows Package Manager.

## Glossary
* **StoreId**: The StoreID is the 12 character number that is used to reference the application in the store. This is a Windows Store ID, often referred to as BigId when interacting with the store team. Example: 9nblggh4nns1.
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
* **StoreId**: The StoreID is the 12 character number that is used to reference the application in the store. This is a Windows Store ID, often referred to as BigId when interacting with the store team. Example: 9nblggh4nns1.
* **StoreId**: The StoreID is the 12 character number that is used to reference the application in the store. This is a Windows Store ID, often referred to as **BigId** when interacting with the store team. Example: `9nblggh4nns1`.

**URL** the URL used for the store is not the same as the URL users and the developers expect. Therefore this field is unnecessary.

**REQUIRED: NEW: StoreID** the Store ID will be used to identify the app.
Example: 9nblggh4nns1
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
Example: 9nblggh4nns1
Example: `9nblggh4nns1`


> [!NOTE] The users preference can override the manifest settings.

**Homepage:** The Homepage will be the URL with the StoreId https://www.microsoft.com/en-us/p/app-installer/9nblggh4nns1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an example?


# Sha256 is a required field. The value is the hash of the installer and used to verify the executable
# Restrictions: [valid sha256 hash]
Sha256: "69D84CA8899800A5575CE31798293CD4FEBAB1D734A07C2E51E56A28E0DF8C82"
Copy link
Contributor

Choose a reason for hiding this comment

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

For this, you'll want to add an entry into patterns.txt,

probably something like:

(?:Signature|)Sha256: "[0-9A-f]{64}"

@ghost ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Nov 20, 2020

#### Uninstall Command

Determining the uninstall string will be based on the type of installer. For EXEs and MSIs the command will be shellexec. If the app requires elevation, then the user will be prompted.
Copy link
Member

Choose a reason for hiding this comment

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

I assume shellexec means using ShellExecute instead of CreateProcess? Is there a reason for that? ShellExecute requires a file path and arguments so the uninstall string would need to be parsed to split it, whereas CreateProcess can take the command line string directly which seems like a better fit.

@ghost ghost added the No-Recent-Activity Issue has no recent activity label Dec 1, 2020
@ghost
Copy link

ghost commented Dec 1, 2020

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@florelis florelis mentioned this pull request Dec 2, 2020
@ghost ghost closed this Dec 8, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Author-Feedback Issue needs attention from issue or PR author No-Recent-Activity Issue has no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet