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 3061 - Marketing Version #3062

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Mar 10, 2023

@github-actions

This comment has been minimized.

@Trenly Trenly marked this pull request as ready for review April 4, 2023 19:24
@Trenly Trenly requested a review from a team as a code owner April 4, 2023 19:24
denelon
denelon previously approved these changes May 8, 2023
@denelon
Copy link
Contributor

denelon commented May 8, 2023

This looks good to me, but I want to get at least one other person from the engineering team to review.

@Trenly
Copy link
Contributor Author

Trenly commented Jun 22, 2023

@ryfu-msft - I believe @denelon is on PTO this week. Do you know who the right person from the team is to review this?

doc/specs/#3061 - Marketing Version.md Show resolved Hide resolved
```
Name Id Version Available
----------------------------------------------------------
Example Package Example.Package 1.2.4 1.2.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, the installed Version here is from arp entry display version and Available version is from marketing version? Would that cause confusion if Available version is way different from installed version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or were you thinking the installed Version would be mapped to marketing version as well?

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 was thinking that installed version would be mapped to marketing version as well. Say for instance the installed version was 1.2.4-a1b2c3Hash the marketing version could be used to show 1.2.4.1 . Where if the installed version was 1.2.4-d1e2f3Hash the marketing version could be used to show 1.2.4.2.

I do realize that this could potentially make correlation on large correlation operations a little slower, but since the correlation is already being performed between installed and available packages, I would presume the impact of an additional mapping would be minor.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is assuming all installed versions are listed in winget repos, which is not the case most of the time. Currently installed to available mapping only happens when the manifest contains DisplayName in Apps and Features entries. If we want to achieve showing marketing version for installed, we need to map installed to available for all cases. And likely, we'll get < SomeMarketVersion as a result most of the 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.

Hmmm. That's true, but its also not different than what happens currently when there is a DisplayVersion specified and the installed version doesn’t match an available version

Copy link
Contributor

@yao-msft yao-msft Jun 22, 2023

Choose a reason for hiding this comment

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

Yep. We can only do relative version < SomeVersion if the mapping is performed. It's just the question that is showing < SomeMarketVersion is better or < SomePackageVersion is better.

I was thinking to make the logic easier. We only do mapping when ARP DisplayVersion is specified in the manifest, most likely, packages that need marketing version are those packages needing mapping because they write non standard versions in registry.

Copy link
Contributor Author

@Trenly Trenly Jun 22, 2023

Choose a reason for hiding this comment

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

That makes sense, and I agree it would be a rarity to use MarketingVersion without DisplayVersion

@yao-msft
Copy link
Contributor

Just as a note, for this marketing version to take effect, this new marketing version needs to be taken into local index (we do not want to download every manifest to check marketing version for matching).

By the way, is this Marketing Version mainly for better display purpose? I do not see many functional changes around this new field. I guess this will also need to be exposed in Com api so at least our powershell cmdlet would display the same

@Trenly
Copy link
Contributor Author

Trenly commented Jun 22, 2023

Just as a note, for this marketing version to take effect, this new marketing version needs to be taken into local index (we do not want to download every manifest to check marketing version for matching).

I want to be sure that I understand correctly; By local index is this referring to the index.db from the pre-packaged winget source, or the installed.db? Since it is theoretically possible that a publisher could change the marketing version at will, I was thinking it would need to be made available in the pre-indexed package and from REST source implementations.

By the way, is this Marketing Version mainly for better display purpose? I do not see many functional changes around this new field. I guess this will also need to be exposed in Com api so at least our powershell cmdlet would display the same

"Mainly" - yes. However, functionally, it has a large impact to user experience based on the way manifests are created. There is an issue open for handling versions with pre-ambles better - the marketing version would allow versions with pre-ambles to be handled without issue by using the DisplayVersion for correlation, MarketingVersion for display purposes, and PackageVersion for sorting. There is also an issue open for publishers that use dd-mm-yyyy date formats as versions, or where patches are denoted by adding "a" "b" "c" to the base version without a separator, or where there is a git commit hash, to allow them to be sorted correctly. This new marketing version, when used in combination with DisplayVersion and PackageVersion would effectively allow that sorting to happen at the manifest level instead of requiring unique handling in the cli, since the CLI would use DisplayVersion to map installed packages to PackageVersion, continues to perform version comparison using PackageVersion, but then displays the MarketingVersion.

@yao-msft
Copy link
Contributor

I want to be sure that I understand correctly; By local index is this referring to the index.db from the pre-packaged winget source, or the installed.db? Since it is theoretically possible that a publisher could change the marketing version at will, I was thinking it would need to be made available in the pre-indexed package and from REST source implementations.

I meant index.db. Marketing version will be added to installed.db though since both use the same code. But installed.db is only used for correlation heuristics, the marketing version in it will be ignored.

"Mainly" - yes. However, functionally, it has a large impact to user experience based on the way manifests are created. There is an issue open for handling versions with pre-ambles better - the marketing version would allow versions with pre-ambles to be handled without issue by using the DisplayVersion for correlation, MarketingVersion for display purposes, and PackageVersion for sorting. There is also an issue open for publishers that use dd-mm-yyyy date formats as versions, or where patches are denoted by adding "a" "b" "c" to the base version without a separator, or where there is a git commit hash, to allow them to be sorted correctly. This new marketing version, when used in combination with DisplayVersion and PackageVersion would effectively allow that sorting to happen at the manifest level instead of requiring unique handling in the cli, since the CLI would use DisplayVersion to map installed packages to PackageVersion, continues to perform version comparison using PackageVersion, but then displays the MarketingVersion.

If package owners wrote these "commit hash versions" to arp entry, then it would not help improve sorting.
If package owners just wanted to show "commit hash versions" to user. Then I think this marketing version solves their needs. Then in this scenario, for displaying installed, should we really need to map to marketing version?

Name                Id               Version   Available
---------------------------------------------------------- 
Example Package     Example.Package  < 801d129d     43dba4c4

@Trenly
Copy link
Contributor Author

Trenly commented Jun 22, 2023

If package owners wrote these "commit hash versions" to arp entry, then it would not help improve sorting.

But it would, since the PackageVersion could be set to 1.0.0, 1.0.1, 1.0.2, 2.0.1, etc; The arp matching would be handled by DisplayVersion; Then MarketingVersion would control the version displayed in outputs

@yao-msft
Copy link
Contributor

yao-msft commented Jun 22, 2023

If package owners wrote these "commit hash versions" to arp entry, then it would not help improve sorting.

But it would, since the PackageVersion could be set to 1.0.0, 1.0.1, 1.0.2, 2.0.1, etc; The arp matching would be handled by DisplayVersion; Then MarketingVersion would control the version displayed in outputs

Sorry, I meant if package installers wrote these "commit hash versions" to arp registry as DisplayVersion. From sorting perspective, marketing version is not involved in mapping from registry DisplayName to package version.

@Trenly
Copy link
Contributor Author

Trenly commented Jun 22, 2023

Sorry, I meant if package installers wrote these "commit hash versions" to arp registry as DisplayVersion. From sorting perspective, marketing version is not involved in mapping from registry DisplayName to package version.

Yes, agreed. Marketing version, in principle, is just a masking of the Package Version by changing what is displayed.

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.

4 participants