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.

Sign up for GitHub

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

Jump to bottom

KDE.Dolphin.Nightly #33717

Closed
wants to merge 2 commits into from
Closed

KDE.Dolphin.Nightly #33717

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40276aa
Improvement.
Nov 3, 2021
dc9a6de
Update manifests/k/KDE/Dolphin/Nightly/master-b6ec4b1c9/KDE.Dolphin.N…
Nov 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion manifests/k/KDE/Dolphin/Nightly/master-b6ec4b1c9/KDE.Dolphin.Nightly.installer.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ InstallModes:
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 •
edited
Loading

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.

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.

Copy link
Contributor

@ItzLevvie ItzLevvie Nov 3, 2021 •
edited
Loading

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.

http://github.com/microsoft/winget-pkgs/pull/33717/files/40276aa68283b421fcf494cc32d1360d524e05f5#r742082211.

Do observe "http://github.com/microsoft/winget-pkgs/pull/33718#discussion_r742010117" for explanation of why I am believing that this modification is advantageous.

Copy link
Contributor Author

@RokeJulianLockhart RokeJulianLockhart Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Please do observe "http://github.com/microsoft/winget-pkgs/pull/33717#issuecomment-959718502".

InstallerSha256: 486CB47FD3171870A2AAF2A18702645BF780605AF2432FD6013A825EB488D18F
ManifestType: installer
ManifestVersion: 1.0.0
Expand Down