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

Spec #147 - Support different release channels [released, beta, alpha] #1742

Closed
wants to merge 6 commits into from

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Nov 23, 2021


Microsoft Reviewers: Open in CodeFlow

@Trenly Trenly changed the title Spec #147 Spec #147 - Support different release channels [released, beta, alpha] Nov 23, 2021
@github-actions
Copy link

github-actions bot commented Nov 23, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • Badlion
  • discussioncomment
  • gmail
  • googlechromedevstandaloneenterprise
  • googlechromestandaloneenterprise
  • Trenly
  • trenlymc
  • wingetbot
Previously acknowledged words that are now absent activatable Globals Google mytool
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]:Trenly/winget-cli.git repository
on the Spec#147 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/977173120" > "$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

Copy link
Contributor

@jedieaston jedieaston left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up :D

I think we need to consider what these changes mean for other repos, like the Microsoft Store or "bob's homegrown organic REST source".

Comment on lines +26 to +44
Instead, the use of multiple installer manifest files will be used to define and manage the various channels available for a package in a way similar to locales. This will ensure that the client remains backwards compatible with old manifest versions, and will make the creation and maintenance of the various channels easier for contributors in the community repository. This will require a key be added to the manifest schema. Channels should not be compatible with singleton manifests.
> The `DefaultChannel` key should be added to the manifest v1.1.0 schema for version manifests

An example of the manifest file structure, with installer channel manifests:
```raw
/manifests/g/Google/Chrome/1.0
| Google.Chrome.installer.beta.yaml
| Google.Chrome.installer.canary.yaml
| Google.Crome.installer.dev.yaml
| Google.Chrome.installer.Stable.yaml
| Google.Chrome.locale.en-US.yaml
| Google.Chrome.locale.nb-NO.yaml
| Google.Chrome.yaml
/manifests/g/Google/Chrome/2.0
| Google.Chrome.installer.canary.yaml
| Google.Crome.installer.dev.yaml
| Google.Chrome.locale.en-US.yaml
| Google.Chrome.locale.nb-NO.yaml
| Google.Chrome.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem I have with this is that it relies on the folder/file naming setup of the community repo, which the client is not aware of (since it is designed to work with any source). How would channels be expressed without a schema field in REST sources, or other pre-indexed sources?

I think we keep the channel field in the schema, and add the DefaultChannel to the version schema. The channel can be given per installer entry, which I'm aware will get unwieldy (but hopefully we don't have to use it often enough for it to be a problem).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I’m not mistaken, for other sources, the channel would still be included in the package schema. Since the package schema is different than the manifest schema, I believe this would still be supported?

Either that, or I would suggest having it both ways. That way the community repo and other sources could still be kept relatively organized by taking advantage of splitting the manifests, but other sources which aren’t able to could still support channels

Copy link
Member

Choose a reason for hiding this comment

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

The original thought (pre multi-file manifests) was to have a structure like:

publisher\package\version
publisher\package\non-default-channel\version

With Channel being in the installer schema, your design can work too. I think it might be a bit more confusing though, as it is harder to see what is in each channel at a glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original thought (pre multi-file manifests) was to have a structure like:

publisher\package\version
publisher\package\non-default-channel\version

With Channel being in the installer schema, your design can work too. I think it might be a bit more confusing though, as it is harder to see what is in each channel at a glance.

That is the way it works currently, but based on the validation over at winget-pkgs, these would be completely separate package Identifiers. If I’m not mistaken, the whole point of channels is to combine package identifiers to be better representative of how packages are published, since some publishers do not allow side-by-side compatibility for different channels and so a user may not realize that Example.Package would be overwritten if they install Example.Package.Dev

Copy link
Member

Choose a reason for hiding this comment

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

We would update winget-pkgs validation to handle non-empty channels in this way so that both would still be Example.Package.

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 would work too. If that is the preferred approach, I will update the spec. Just so long as we capture the work that is needed

## Solution Design

### Manifest Structure
The Windows Package Manager manifest v1.1.0 schema has provided a key for declaring which channel an installer belongs to. However, this has the potential to make the installer manifests extremely complex when there are multiple release channels with multiple installers each.
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, this appears to be an oversight on my part due to the too many things effect...

It really should have been in the version schema; maybe we can fix it by moving it to the correct location and using PackageChannel as the supported value. I think it could also work in the installer schema, so long as it is only allowed at the root, not on individual installers (I think it is but I don't 100% trust myself on a quick look at the JSON schema).

But if it is a single value, the location that it is defined is more for readability than the technical part. That is the route that was planned long ago, and it will probably be easier to stick with that than change course now.

Copy link
Contributor Author

@Trenly Trenly Nov 30, 2021

Choose a reason for hiding this comment

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

I think that each installer should have a channel associated with it. The main issue I see here is that a single channel could have many many installers or just a single installer. Providing the support to have different channels as different installer manifests would remove this issue.

The key could potentially be left as it is to allow for the case where there is one installer for each channel too. It wouldn’t necessarily have to be at just the root level.

I think the biggest thing is that we align and understand what the best path forward is at the moment

The Windows Package Manager manifest v1.1.0 schema has provided a key for declaring which channel an installer belongs to. However, this has the potential to make the installer manifests extremely complex when there are multiple release channels with multiple installers each.
> The `Channel` key should be removed from the manifest v1.1.0 schema.

Instead, the use of multiple installer manifest files will be used to define and manage the various channels available for a package in a way similar to locales. This will ensure that the client remains backwards compatible with old manifest versions, and will make the creation and maintenance of the various channels easier for contributors in the community repository. This will require a key be added to the manifest schema. Channels should not be compatible with singleton manifests.
Copy link
Member

Choose a reason for hiding this comment

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

Everything needs be compatible with any manifest representation, we just impose certain patterns on winget-pkgs.

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'm not entirely sure what you mean by this comment. Is this just saying that singletons should support channels?

Copy link
Contributor

Choose a reason for hiding this comment

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

Manifests from any source, i.e. REST sources that don’t use the manifest schema, preindexed sources (which are generated from the YAML files but converted to a different format), and local (-m on a folder) manifests is what I think that means.

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 see; In that case, there still shouldn’t be an issue, since the rest sources should still be able to specify the channel, pre-indexed should work as it would just require them to support building from the proposed mutiple-installer-manifests yaml structure, and local would still be handled just fine since the CLI would be able to find the channels based on the filenames or the specified value in the manifest file

Comment on lines +26 to +44
Instead, the use of multiple installer manifest files will be used to define and manage the various channels available for a package in a way similar to locales. This will ensure that the client remains backwards compatible with old manifest versions, and will make the creation and maintenance of the various channels easier for contributors in the community repository. This will require a key be added to the manifest schema. Channels should not be compatible with singleton manifests.
> The `DefaultChannel` key should be added to the manifest v1.1.0 schema for version manifests

An example of the manifest file structure, with installer channel manifests:
```raw
/manifests/g/Google/Chrome/1.0
| Google.Chrome.installer.beta.yaml
| Google.Chrome.installer.canary.yaml
| Google.Crome.installer.dev.yaml
| Google.Chrome.installer.Stable.yaml
| Google.Chrome.locale.en-US.yaml
| Google.Chrome.locale.nb-NO.yaml
| Google.Chrome.yaml
/manifests/g/Google/Chrome/2.0
| Google.Chrome.installer.canary.yaml
| Google.Crome.installer.dev.yaml
| Google.Chrome.locale.en-US.yaml
| Google.Chrome.locale.nb-NO.yaml
| Google.Chrome.yaml
Copy link
Member

Choose a reason for hiding this comment

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

The original thought (pre multi-file manifests) was to have a structure like:

publisher\package\version
publisher\package\non-default-channel\version

With Channel being in the installer schema, your design can work too. I think it might be a bit more confusing though, as it is harder to see what is in each channel at a glance.

| Google.Chrome.yaml
```

To maintain backwards compatability, `null` should be treated as a valid `DefaultChannel`. If the default channel is not specified, `<PackageIdentifier>.installer.yaml` would be treated as the default. Additional channels may still be added to the package, and all functionality would still be present.
Copy link
Member

Choose a reason for hiding this comment

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

The code already expects that an empty value is the default channel. Having a user defined default channel value will complicate things. I don't see the value in allowing it even if it was easy either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One that I could think of is Ubuntu. Canonical.Ubuntu 16.04, 18.04, and 20.04 all have their own release cadence, but so does the un-versioned channel. In this case, 20.04 would make the most sense to be the default channel.

Copy link
Member

Choose a reason for hiding this comment

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

Is the thinking that the default channel will march forward with the latest release? So currently it would be 20.04 but when 22.04 is released it would change to that, while still leaving the 20.04 channel as is. I see the value there; if we used an empty channel then to achieve the same thing would require moving/renaming all of the manifests.

I think it still makes sense (and I think what you are saying the spec) to allow for the empty channel to be the default default. Then only allow for DefaultChannel when there is no empty channel.


#### winget show
When the user specifies `--versions`, the versions for all channels along with the channels should be shown. The versions should be grouped by channel, sorted by channel, then sorted by version descending
When the user specifies `--channels`, the available channels for the package should be shown. If a package contains a named default channel and unnamed (null) channels, only the named channels should be shown
Copy link
Member

Choose a reason for hiding this comment

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

I'm not following this part, but I assert that there should be only one empty channel, and any number of non-empty channels.

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 this goes back to your point about whether or not a named channel could or should be allowed to be default


#### winget export/import
When exporting the list of installed packages, where possible, the release channel should be included with the export.
When importing the list of installed packages, where possible, the release channel should be honored. If the release channel is not able to be honored, a warning should be thrown, and the default release channel should be used
Copy link
Member

Choose a reason for hiding this comment

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

I think our default stance is to fail the import without doing anything if we know up front that we can't do what is asked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a fair stance. I’ll update this if needed

Found 7-zip [7zip.7zip]
Channel
-------
Default
Copy link
Member

Choose a reason for hiding this comment

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

If the default channel is always "empty", then there is no need to have a reserved value or even show it in this case.


### Compatibility

The current implementation for different release channels is to have separate package identifiers for each package. Packages which have channel-specific identifiers currently existing in the community repository /should/ be updated to use this feature, but will not be required to.
Copy link
Member

Choose a reason for hiding this comment

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

If they do not support side-by-side installation, they really should be moved to this pattern.

If they do support it, I think that is a whole other design spec to deal with them better.

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 the point of channels is to have them be combined to a single identifier where possible. The packages which do support side by side are the best to have in the Example.Package / Example.Package.Dev format. It is when they do not support side by side that they should have the channel specified for the installer to ensure the proper channel is used for updates and installation

@denelon
Copy link
Contributor

denelon commented Dec 1, 2021

Would you mind updating the allow.txt for valid words and expected.txt for URL fragments and programming vernacular?

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • Badlion
  • discussioncomment
  • gmail
  • googlechromedevstandaloneenterprise
  • googlechromestandaloneenterprise
  • Trenly
  • trenlymc
  • wingetbot
Previously acknowledged words that are now absent activatable Globals Google mytool
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]:Trenly/winget-cli.git repository
on the Spec#147 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/984864889" > "$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

@Trenly
Copy link
Contributor Author

Trenly commented Mar 10, 2022

I'm closing this to allow MSFT to better align on a spec internally, as this feature has several complexities and is likely not in the next several client releases. To ensure that the spec stays up to date with the client, it is best for this to be closed and be used as a reference for future ideas and considerations

@Trenly Trenly closed this Mar 10, 2022
@Trenly Trenly deleted the Spec#147 branch March 10, 2022 17:07
@vedantmgoyal9
Copy link
Contributor

vedantmgoyal9 commented Apr 20, 2022

Why can't we just include a Channel key in the version manifest?

I mean WinGet finally merges the manifests, for e.g. see https://winget.azureedge.net/cache/manifests/m/Microsoft/Teams/1.5.00.8070/d9c7-Microsoft.Teams.yaml and note ManifestType: merged

It's rare that the publisher publishes the "same version" to two channels and for that, we can keep the Channel field as an array.

The Upgrade Cases may be complex because: In the current state, a new package identifier must be created for each release channel which can cause conflicts when the versions are not side-by-side compatible. For this, we can modify the existing UpgradeBehavior key which can be set to:

  • When the Beta version can be installed over a Stable version [install] + user will have to specify --force parameter to look for updates as well as upgrade across different channels.
  • When the Beta version can be installed side-by-side with the Stable version [installStrict].
    Note: When the package has an upgrade available in the Stable channel, it should not modify the installation of Beta version.
  • When a Stable version has to be uninstalled before installing the Beta version [uninstallPrevious]

The CLI Behavior and the UI/UX would remain same as mentioned in the spec at the time of writing this comment.

Additionally, we can have a PackageChannels in #1902 which will be an array and will be used in the validation pipeline so that if any new contributor updates a package and uses, for e.g. Production instead of Stable, @wingetbot will notify the contributor to make the updates accordingly.

This may make the implementation of channels into winget-cli somewhat complex, but it would be easy on the winget-pkgs repository side and for users/orgs who manage their own rest-source.

Sorry! Apologies in advance if I'm misunderstanding something but I really don't understand why it has to be too complex.

@Trenly
Copy link
Contributor Author

Trenly commented Apr 21, 2022

The reason I was looking at the structure of manifests is because the CLI will have to be able to search for, and differentiate, between channels even with all other things about the package being the same

Because there is not a set way for publishers to do things, there are many cases where the only differentiator between a prerelease and an actual release is quite literally what the publisher says it is. Take winget for example, If winget were looking at the ARP entries of the PreRelease vs the Latest release, they would be identical. So, the client has to have some way to track that on install.

This is also a reason why the channel would likely have to be an Installer property, because there are publishers who do use the same version within their channels. Google Chrome is a great example, since the version starts in Canary and then gets promoted through to stable. Now, just because it gets promoted doesn’t mean that it can’t also be the latest beta or latest stable, the same version can be in multiple channels at the same time (even with different URLS to download from)

Then, when upgrading a package, winget should choose the latest version that is within the channel. As I was thinking about how the client may accomplish this, it seemed to me that instead of loading each manifest into the index it would be easier to identify specific files. Honestly, this is partly because (at the time) I didn’t know much about the different source types and how each are created.

However, I think there are still complexities in that sometimes a user will specify to install a version without specifying a channel, sometimes they may specify a channel and a version, and they can do all sorts of other weird things we may not expect.


All of that to say - Channels impacts the core functionality of how packages are searched for, installed, and upgraded. Not only that, but they impact almost every other feature within the client. Additionally, it impacts validation from the client all the way through the pipelines into the validation of what is installed on users machines currently.

My first draft of a spec (which you will notice I closed in favor of letting the Experts at MSFT create) was trying to account for all the complexities but still didn’t take into consideration the range of impact this feature has.

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.

5 participants