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.Nightly #33717

Closed
wants to merge 2 commits into from
Closed

KDE.Dolphin.Nightly #33717

wants to merge 2 commits into from

Conversation

RokeJulianLockhart
Copy link
Contributor

@RokeJulianLockhart RokeJulianLockhart commented Nov 3, 2021

Rectification of the version of KDE.Dolphin.Nightly, 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/30233 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

Rectification of the version of KDE.Dolphin.Nightly, 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
@RokeJulianLockhart RokeJulianLockhart changed the title Improvement. KDE.Dolphin.Nightly Nov 3, 2021
@KaranKad
Copy link
Contributor

KaranKad commented Nov 3, 2021

PackageVersion needs to be according to the one in control panel otherwise it might cause issues while upgrading.
image

@@ -2,15 +2,15 @@
# yaml-language-server: $schema=https://aka.ms/winget-manifest.installer.1.0.0.schema.json

PackageIdentifier: KDE.Dolphin.Nightly
PackageVersion: master-b6ec4b1c9
PackageVersion: master-271
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be the same as the version in the ARP entries, which in this case is master-b6ec4b1c9, so the original manifest is correct

Copy link
Contributor

Choose a reason for hiding this comment

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

@KaranKad you beat me by mere seconds 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Are previous versions of nightly builds removed? If they are, then using the last successful build is good. If they are not, then we should keep using the specific build numbers to maintain the version history and keep previous versions available through winget

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/33717/commits/40276aa68283b421fcf494cc32d1360d524e05f5#r741972545.

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. This should not prevent operation any current functionality of winget.

If this explanation is sufficient, please do resolve it or instruct me to resolve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are for KDE projects. These are hard to maintain because the file name changes too.

@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

http://github.com/microsoft/winget-pkgs/pull/33717#issuecomment-959160630.

@KaranKad, within http://binary-factory.kde.org, all entries are versioned consistently. Are you sure that replacement of it with that shall be assistive? I am also not sure of where that value is from.

For evidence of this, please do observe the submitted metadata that is for other software that has been created by KDE e.V:
http://github.com/microsoft/winget-pkgs/issues/30233 (master-0271), and:
http://github.com/microsoft/winget-pkgs/issues/30234 (master-1255), and:
http://github.com/microsoft/winget-pkgs/issues/33704 (master-1347), and:
http://github.com/microsoft/winget-pkgs/issues/33705 (master-0426), and:
http://github.com/microsoft/winget-pkgs/issues/33706 (master-0164), and:
http://github.com/microsoft/winget-pkgs/issues/33709 (master-0928), and:
http://github.com/microsoft/winget-pkgs/issues/33710 (master-0924), and:
http://github.com/microsoft/winget-pkgs/issues/33711 (master-0747), and:
http://github.com/microsoft/winget-pkgs/issues/33712 (master-0743), and:
http://github.com/microsoft/winget-pkgs/issues/33713 (master-0741), and:
http://github.com/microsoft/winget-pkgs/issues/33715 (master-0073), and:
http://github.com/microsoft/winget-pkgs/issues/33716 (master-0338), and:
http://github.com/microsoft/winget-pkgs/issues/33719 (master-0338), and:
http://github.com/microsoft/winget-pkgs/issues/33721 (master-0936), and:
http://github.com/microsoft/winget-pkgs/issues/33722 (master-0936), and:
http://github.com/microsoft/winget-pkgs/issues/33725 (master-0931), and:
http://github.com/microsoft/winget-pkgs/issues/33726 (master-0785), and:
http://github.com/microsoft/winget-pkgs/issues/33727 (master-0744), and:
http://github.com/microsoft/winget-pkgs/issues/33729 (master-0436), and:
http://github.com/microsoft/winget-pkgs/issues/33730 (master-0932), and:
http://github.com/microsoft/winget-pkgs/issues/33735 (master-0741), and:
http://github.com/microsoft/winget-pkgs/issues/33736 (master-0027), and:
http://github.com/microsoft/winget-pkgs/issues/33737 (master-0744).

Consequently, usage of the value that you have proposed (master-b6ec4b1c9, rather than master-271) for this package surely shall not be beneficial.

@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
@jedieaston
Copy link
Contributor

It's the value they are putting in the Add and Remove Programs table, which is how winget knows to upgrade something (given that it's text and not a number I don't know if the comparison algorithm will work correctly here, but that's how it works). We need to use that value for PackageVersion.

Scope: machine
InstallModes:
- interactive
- silent
Installers:
- Architecture: x64
InstallerType: nullsoft
InstallerUrl: https://binary-factory.kde.org/job/Dolphin_Nightly_win64/271/artifact/dolphin-master-271-windows-msvc2019_64-cl.exe
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Nightly_win64/lastSuccessfulBuild/artifact/dolphin-master-271-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_Nightly_win64/lastSuccessfulBuild/artifact/dolphin-master-271-windows-msvc2019_64-cl.exe
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Nightly_win64/271/artifact/dolphin-master-271-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 or a slightly bit more, 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.

Copy link
Contributor Author

@RokeJulianLockhart RokeJulianLockhart Nov 3, 2021

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
…ightly.installer.yaml

Co-authored-by: Levvie - she/her <[email protected]>
@ghost ghost removed Needs-Author-Feedback This needs a response from the author. Manifest-Validation-Error Manifest validation failed Needs-Attention This work item needs to be reviewed by a member of the core team. labels Nov 3, 2021
@wingetbot
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Scope: machine
InstallModes:
- interactive
- silent
Installers:
- Architecture: x64
InstallerType: nullsoft
InstallerUrl: https://binary-factory.kde.org/job/Dolphin_Nightly_win64/271/artifact/dolphin-master-271-windows-msvc2019_64-cl.exe
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Nightly_win64/lastSuccessfulBuild/artifact/dolphin-master-271-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?

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_Nightly_win64/271/artifact/dolphin-master-271-windows-msvc2019_64-cl.exe
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Nightly_win64/lastSuccessfulBuild/artifact/dolphin-master-271-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/33717/commits/40276aa68283b421fcf494cc32d1360d524e05f5#r741972545.

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. This should not prevent operation any current functionality of winget.

If this explanation is sufficient, please do resolve it or instruct me to resolve it.

@@ -2,15 +2,15 @@
# yaml-language-server: $schema=https://aka.ms/winget-manifest.installer.1.0.0.schema.json

PackageIdentifier: KDE.Dolphin.Nightly
PackageVersion: master-b6ec4b1c9
PackageVersion: master-271
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -2,15 +2,15 @@
# yaml-language-server: $schema=https://aka.ms/winget-manifest.installer.1.0.0.schema.json

PackageIdentifier: KDE.Dolphin.Nightly
PackageVersion: master-b6ec4b1c9
PackageVersion: master-271
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

What is an ARP entry?

ARP == Add and Remove Programs table. It's winget's source of truth for all of the non-MSIX apps installed on the machine, and how it determines whether or not an upgrade is necessary. We need the version number to be whatever is in that table (until schema 1.1).

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. This should not prevent operation any current functionality of winget.

Using lastSuccessfulBuild breaks the links, and a find and replace on the link will fix all instances of the number anyway, so I don't know why having it in two places in the string is worse than having it in one place.

Scope: machine
InstallModes:
- interactive
- silent
Installers:
- Architecture: x64
InstallerType: nullsoft
InstallerUrl: https://binary-factory.kde.org/job/Dolphin_Nightly_win64/271/artifact/dolphin-master-271-windows-msvc2019_64-cl.exe
InstallerUrl: https://binary-factory.kde.org/view/Windows%2064-bit/job/Dolphin_Nightly_win64/lastSuccessfulBuild/artifact/dolphin-master-271-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.

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.

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

jedieaston commented Nov 3, 2021

for anyone who cares: Jenkins has a pretty good API, I just wrote a little Python and can get all of the latest versioned nightly links. Shouldn't be too bad to automate this.

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/33717#issuecomment-959731646.

Are the nightly builds released inconsistently?

@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.

Are the nightly builds released inconsistently?

If you're talking about all KDE apps included (Nightly + Release), then yes.

One release every day at 5 AM for KDE Kate - Release
One release every day at 10 PM for KDE Dolphin - Release
One release every day at 8 PM for KDE Connect - Release

One release every day at 2 AM for KDE Kate - Nightly
One release every day at 1 PM for KDE Dolphin - Nightly
One release every day at 10 AM for KDE Connect - Nightly

The timings of these are different for every single KDE app, this becomes difficult to use lastSuccessfulBuild because the URL will no longer work when there's a new release of a KDE app.

So, what happens if the automation (either @wingetbot or vedant's automation) runs at, say 6 PM, and there's a new KDE release for say, Connect or Dolphin, packages that use lastSuccessfulBuild will break easily and people will be unable to download until the automation catches up.

I have had to fix people's PR when they used lastSuccessfulBuild, because the next day when I validated the PR - I got 404 Not Found and then some people will have to update the URL and Hash.

@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

Response

This comment is response to "http://github.com/microsoft/winget-pkgs/pull/33717#pullrequestreview-796879170".

Ideally, KDE should remediate this problem so that automatic replacement is less burdensome for the maintainers of package-managers that are similar to winget, and so that manual replacement shall be less frequent. Consequently, how best might this be remediated: should invalid request for packages that were, but not anymore, within "/lastsuccessfulbuild/" be resolved correctly automatically by "http://jenkins.io" or "http://binary-factory.kde.org"?

This is worth remediating now, so that before more of KDE's software has been added to winget, this problem shall have become non-existent.

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

Trenly commented Nov 3, 2021

This comment is response to "http://github.com/microsoft/winget-pkgs/pull/33717#pullrequestreview-796879170".

Ideally, KDE should remediate this problem so that automatic replacement is less burdensome for the maintainers of package-managers that are similar to winget, and so that manual replacement shall be less frequent. Consequently, how best might this be remediated: should invalid request for packages that were, but not anymore, within "/lastsuccessfulbuild/" be resolved correctly automatically by "http://jenkins.io" or "http://binary-factory.kde.org"?

This is worth remediating now, so that before more of KDE's software has been added to winget, this problem shall have become non-existent.

One way to have the automatic update functionality that you're looking for would be to have a vanity url which always stays the same. That way the URL never needs to be changed, only the version does. What I mean by a vanity url is something like https://binary-factory.kde.org/download/Dolphin_Nightly_win64/latest?type=exe or https://binary-factory.kde.org/download/Dolphin_Nightly_win64/latest?type=appx or https://binary-factory.kde.org/download/Dolphin_Nightly_win64. If the build definitions that KDE uses allowed these URLs to always point to the latest version, because the URL is static wingetbot would be able to update them.

However, this is most definitely something that would need to be changed on KDE's end

@RokeJulianLockhart
Copy link
Contributor Author

RokeJulianLockhart commented Nov 13, 2021

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

@Trenly, I have reported some of this problemage via "http://bugs.kde.org/show_bug.cgi?id=444894#c0".

@denelon denelon added Area-External Blocking-Issue Manifest validation is blocked by a known issue. and removed Needs-Attention This work item needs to be reviewed by a member of the core team. labels Feb 12, 2022
@ghost ghost added the Moderator-Approved One of the Moderators has reviewed and approved this PR label Mar 22, 2022
@RokeJulianLockhart RokeJulianLockhart marked this pull request as ready for review March 22, 2022 21:59
@wingetbot wingetbot added the Validation-Merge-Conflict Package could not be tested due to merge conflict. label Mar 28, 2022
@ghost ghost removed the Azure-Pipeline-Passed Validation pipeline passed. There may still be manual validation requirements. label Mar 28, 2022
@ghost
Copy link

ghost commented Mar 28, 2022

Hello @BEEDELLROKEJULIANLOCKHART,
The PR could not be validated because there is a merge conflict which adds bad characters to the manifest. Please address the merge conflict and resubmit.

@ghost ghost added the Needs-Author-Feedback This needs a response from the author. label Mar 28, 2022
@ghost ghost assigned yao-msft and ryfu-msft Mar 28, 2022
@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 Jun 15, 2022
@denelon denelon removed the Needs-Attention This work item needs to be reviewed by a member of the core team. label Aug 23, 2022
@Trenly
Copy link
Contributor

Trenly commented Sep 2, 2022

Close with reason: Merge Conflict + Package URLs change daily, and builds are not retained;

@ghost ghost closed this Sep 2, 2022
@RokeJulianLockhart
Copy link
Contributor Author

Yeah, the currently present manifests are all 404. We'll just have to wait until a solution of provided, I suppose, or use different URLs (if any are available).

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-External Blocking-Issue Manifest validation is blocked by a known issue. Moderator-Approved One of the Moderators has reviewed and approved this PR Validation-Completed Validation passed Validation-Merge-Conflict Package could not be tested due to merge conflict.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants