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

KDE.Dolphin.Release #33718

Closed
wants to merge 2 commits into from
Closed

KDE.Dolphin.Release #33718

wants to merge 2 commits into from

Conversation

RokeJulianLockhart
Copy link
Contributor

@RokeJulianLockhart RokeJulianLockhart commented Nov 3, 2021

Improvement of the consistency of the version of KDE.Dolphin.Release and all of the other packages that have been created by KDE, and replacement of the URL for the installer with a version that is much more easy to maintain, but shall not cause submission of invalid installers.

This should allow the improvements that have been submitted to http://github.com/microsoft/winget-pkgs/issues/30234 to be merged.

  • Have you signed the Contributor License Agreement?
  • Have you checked that there aren't other open pull requests for the same manifest update/change?
  • Have you validated your manifest locally with winget validate --manifest <path>?
  • Have you tested your manifest locally with winget install --manifest <path>?
  • Does your manifest conform to the 1.0 schema?

Note: <path> is the name of the directory containing the manifest you're submitting.


Microsoft Reviewers: Open in CodeFlow

Improvement of the consistency of the version of KDE.Dolphin and all of the other packages that have been created by KDE, and replacement of the URL for the installer with a version that is much more easy to maintain, but shall not cause submission of invalid installers.
@wingetbot
Copy link
Collaborator

Service Badge  Service Badge  

@wingetbot
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wingetbot wingetbot added the Manifest-Validation-Error Manifest validation failed label Nov 3, 2021
@ghost
Copy link

ghost commented Nov 3, 2021

Hello @BEEDELLROKEJULIANLOCKHART,
The package manager bot determined that the metadata was not compliant.

Please verify the manifest file is compliant with the package manager 1.0 manifest specification.
Make sure the ID is of the form publisher.appname and that the folder structure is manifests\partition\publisher\appname\version.
Note: The path and "PackageIdentifier" are case sensitive.
Be sure to use a tool like VSCode (https://code.visualstudio.com/) to make sure the manifest YAML syntax is correct.

You could also try our Windows Package Manager Manifest Creator Preview.

For details on the specific error, see the details link below in the build pipeline.

@ghost ghost added Needs-Author-Feedback This needs a response from the author. and removed Needs-Author-Feedback This needs a response from the author. labels Nov 3, 2021
@ghost ghost removed the Manifest-Validation-Error Manifest validation failed label Nov 3, 2021
@wingetbot
Copy link
Collaborator

/AzurePipelines run

1 similar comment
@wingetbot
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@RokeJulianLockhart RokeJulianLockhart changed the title Imoprovement. Improvement. Nov 3, 2021
@wingetbot wingetbot added the Manifest-Validation-Error Manifest validation failed label Nov 3, 2021
@ghost
Copy link

ghost commented Nov 3, 2021

Hello @BEEDELLROKEJULIANLOCKHART,
The package manager bot determined that the metadata was not compliant.

Please verify the manifest file is compliant with the package manager 1.0 manifest specification.
Make sure the ID is of the form publisher.appname and that the folder structure is manifests\partition\publisher\appname\version.
Note: The path and "PackageIdentifier" are case sensitive.
Be sure to use a tool like VSCode (https://code.visualstudio.com/) to make sure the manifest YAML syntax is correct.

You could also try our Windows Package Manager Manifest Creator Preview.

For details on the specific error, see the details link below in the build pipeline.

@ghost ghost added the Needs-Author-Feedback This needs a response from the author. label Nov 3, 2021
@RokeJulianLockhart RokeJulianLockhart changed the title Improvement. KDE.Dolphin.Release Nov 3, 2021
@ghost ghost removed the Needs-Author-Feedback This needs a response from the author. label Nov 3, 2021
@KaranKad
Copy link
Contributor

KaranKad commented Nov 3, 2021

I think adding .Release to stable application might be unnecessary because other stable applications don't have it. Only Nightly, Pre-release, Beta type of applications need it.

@KaranKad KaranKad mentioned this pull request Nov 3, 2021
5 tasks
PackageIdentifier: KDE.Dolphin
PackageVersion: 21.04.3
PackageIdentifier: KDE.Dolphin.Release
PackageVersion: master-1255
Copy link
Contributor

@ItzLevvie ItzLevvie Nov 3, 2021

Choose a reason for hiding this comment

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

Suggested change
PackageVersion: master-1255
PackageVersion: 21.04.3

PackageVersion must match Control Panel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ghost ghost added the Needs-Author-Feedback This needs a response from the author. label Nov 3, 2021
Scope: machine
InstallModes:
- interactive
- silent
Installers:
- Architecture: x64
InstallerType: nullsoft
InstallerUrl: https://binary-factory.kde.org/job/Dolphin_Release_win64/1255/artifact/dolphin-21.04.3-1255-windows-msvc2019_64-cl.exe
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Release_win64/lastSuccessfulBuild/artifact/dolphin-21.04.3-1255-windows-msvc2019_64-cl.exe
Copy link
Contributor

@ItzLevvie ItzLevvie Nov 3, 2021

Choose a reason for hiding this comment

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

Suggested change
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Release_win64/lastSuccessfulBuild/artifact/dolphin-21.04.3-1255-windows-msvc2019_64-cl.exe
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Release_win64/1255/artifact/dolphin-21.04.3-1255-windows-msvc2019_64-cl.exe

Why was this changed? lastSuccessfulBuild in the URL is only valid for 1 day (24 hours), compared to having the build number in the URL which would last for about 4 or 5 days, before needing to be replaced.

If someone doesn't regularly update this like KDE Connect, people will be unable to download due to the expired URL. There will also likely be issue spam from @wingetbot when the URLs are unavailable if it's not updated in a timely matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

http://github.com/microsoft/winget-pkgs/pull/33718/files/5c221a74c3d79381ea178f4bbc75b7d09b24d70c#r741998016.

The reason that I have specified "/lastSuccessfulBuild/" is because if "/lastSuccessfulBuild/" is used, the logic that is required for automatic replacement by new versions is much more simple, because the name of the version is part of the filename, so also switching directories is quite silly.

Although I did not know that the time that shall be was available for before replacement is mandatory is significantly less than the counterpart of it that you have specified, the automation that is currently present within this repository should remediate that problem.

@@ -1,16 +1,16 @@
# Created using wingetcreate 0.4.1.1
# yaml-language-server: $schema=https://aka.ms/winget-manifest.installer.1.0.0.schema.json

PackageIdentifier: KDE.Dolphin
PackageVersion: 21.04.3
PackageIdentifier: KDE.Dolphin.Release
Copy link
Contributor

@ItzLevvie ItzLevvie Nov 3, 2021

Choose a reason for hiding this comment

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

Suggested change
PackageIdentifier: KDE.Dolphin.Release
PackageIdentifier: KDE.Dolphin

I believe this isn't needed here since this would likely cause inconsistencies with other applications outside of what KDE publishes.

Scope: machine
InstallModes:
- interactive
- silent
Installers:
- Architecture: x64
InstallerType: nullsoft
InstallerUrl: https://binary-factory.kde.org/job/Dolphin_Release_win64/1255/artifact/dolphin-21.04.3-1255-windows-msvc2019_64-cl.exe
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Release_win64/lastSuccessfulBuild/artifact/dolphin-21.04.3-1255-windows-msvc2019_64-cl.exe
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall mere HTTP be better than HTTPS?

Copy link
Contributor

@ItzLevvie ItzLevvie Nov 3, 2021

Choose a reason for hiding this comment

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

HTTPS is always preferred over HTTP.

If the URL doesn't work with HTTPS (i.e. server isn't configured to be used for HTTPS), then you can use HTTP.

But this domain supports HTTPS.

Scope: machine
InstallModes:
- interactive
- silent
Installers:
- Architecture: x64
InstallerType: nullsoft
InstallerUrl: https://binary-factory.kde.org/job/Dolphin_Release_win64/1255/artifact/dolphin-21.04.3-1255-windows-msvc2019_64-cl.exe
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Release_win64/lastSuccessfulBuild/artifact/dolphin-21.04.3-1255-windows-msvc2019_64-cl.exe
Copy link
Contributor Author

Choose a reason for hiding this comment

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

http://github.com/microsoft/winget-pkgs/pull/33718/files/5c221a74c3d79381ea178f4bbc75b7d09b24d70c#r741998016.

The reason that I have specified "/lastSuccessfulBuild/" is because if "/lastSuccessfulBuild/" is used, the logic that is required for automatic replacement by new versions is much more simple, because the name of the version is part of the filename, so also switching directories is quite silly.

Although I did not know that the time that shall be was available for before replacement is mandatory is significantly less than the counterpart of it that you have specified, the automation that is currently present within this repository should remediate that problem.

PackageIdentifier: KDE.Dolphin
PackageVersion: 21.04.3
PackageIdentifier: KDE.Dolphin.Release
PackageVersion: master-1255
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ghost ghost added Needs-Attention This work item needs to be reviewed by a member of the core team. and removed Needs-Author-Feedback This needs a response from the author. labels Nov 3, 2021
@ItzLevvie
Copy link
Contributor

ItzLevvie commented Nov 3, 2021

Please do observe http://github.com/microsoft/winget-pkgs/pull/33717#issuecomment-959212573. Consequently, is the reversion that you have proposed definitely mandatory?

Yes, it's mandatory. I don't think wasting people's internet bandwidth would be a good idea if someone has a limited internet connection :)

21.04.3 shows up in Control Panel rather than master-1255
master-1255 is bigger than 21.04.3 which will make the upgrades of this application in an infinite loop and the user may or may not notice it. What happens if the app was 1 GB and the person didn't notice it was in an upgrade loop when doing winget upgrade --all or something?

@RokeJulianLockhart RokeJulianLockhart mentioned this pull request Nov 3, 2021
5 tasks
Copy link
Contributor

@ItzLevvie ItzLevvie left a comment

Choose a reason for hiding this comment

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

@ghost ghost added the Needs-Author-Feedback This needs a response from the author. label Nov 3, 2021
@RokeJulianLockhart
Copy link
Contributor Author

http://github.com/microsoft/winget-pkgs/pull/33718#issuecomment-959640978.

How would usage of that system cause infinite replacement?
Also, because the nightly versions do not use the standard release versions, what should they use? http://github.com/microsoft/winget-pkgs/pull/33717#issuecomment-959212573 would be much less consistent if that were to be true.

@ghost ghost removed the Needs-Author-Feedback This needs a response from the author. label Nov 3, 2021
Copy link
Contributor

@ItzLevvie ItzLevvie left a comment

Choose a reason for hiding this comment

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

How would usage of that system cause infinite replacement?

When it comes to upgrading applications, WinGet takes a look at the installed version from Control Panel (Registry) and compares it to the package repository (winget-pkgs), since master-1255 is bigger than the installed version of 21.04.3, it's considered as an upgrade loop (or an infinity upgrade) because there's a version mismatch between both versions.

So, therefore 21.04.3 from Control Panel must match the one in the package repository.

Also, because the nightly versions do not use the standard release versions

Generally, the versions must match what's shown in Control Panel.

would be much less consistent if that were to be true.

If you want the version to be master-XXXX I would recommend to file an issue about it in KDE's issue tracker.

You could also file an issue on their issue tracker, for allowing other people to upgrade from version to version by asking the developer to add the Jenkins build number at the end of the version in Control Panel, say 21.04.3.1255.

@ghost ghost added the Needs-Author-Feedback This needs a response from the author. label Nov 3, 2021
@RokeJulianLockhart
Copy link
Contributor Author

RokeJulianLockhart commented Nov 3, 2021

This comment is response to “http://github.com/microsoft/winget-pkgs/pull/33718#pullrequestreview-796861130”.

I have filed the report of potential improvement at "http://bugs.kde.org/show_bug.cgi?id=444894". Should we wait until that has been resolved, or should I revert that section of my modification via this pull-request until then?

I hope that my explanation of the problem was adequately correct, specific, and descriptive. If it was not, please add to it.

@ghost ghost removed the Needs-Author-Feedback This needs a response from the author. label Nov 3, 2021
@RokeJulianLockhart
Copy link
Contributor Author

RokeJulianLockhart commented Nov 13, 2021

@ItzLevvie
Copy link
Contributor

Should I use "http://github.com/microsoft/winget-create/releases" for remediation of "http://github.com/microsoft/winget-pkgs/pull/33718#issuecomment-959052300"?

You're free to use either wingetcreate, or YamlCreate.ps1 in the Tools directory in this repository.

YamlCreate.ps1 is preferred because it allows you to add more metadata to the manifest.

@vedantmgoyal9
Copy link
Contributor

I have filed the report of potential improvement at “http://bugs.kde.org/show_bug.cgi?id=444894”. Should we wait until that has been resolved, or should I revert that section of my modification via this pull-request until then?

I think they have deleted the bug.

@ItzLevvie
Copy link
Contributor

I think they have deleted the bug.

It's here: https://bugs.kde.org/show_bug.cgi?id=444894

@vedantmgoyal9
Copy link
Contributor

Yeah, I think it was a redirect issue (HTTP -> HTTPS).

@RokeJulianLockhart
Copy link
Contributor Author

RokeJulianLockhart commented Nov 27, 2021

This comment is response to "http://github.com/microsoft/winget-pkgs/pull/33718#issuecomment-979974212":

@vedantmgoyal2009, the problem that you have encountered was caused by incorrect identification of the hyperlink by GitHub because its parser is inadequate. Consequently, the successive mark-of-speech was interpreted as constituent of the hyperlink. Observe for verification which sections of the hyperlink are blue (if you are able to optically perceive blue).

@ghost ghost closed this in #40459 Jan 11, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Manifest-Validation-Error Manifest validation failed Needs-Attention This work item needs to be reviewed by a member of the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants