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

Run Minetest update checker on startup #7629

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@SmallJoker
Copy link
Member

commented Aug 8, 2018

"well, it works".
The configuration setters and some return lines were commented out for obvious reasons: (preview)
Screenshot

However, the version checking is not optimal (relies on MINOR.MAJOR.PATCH) and GitHub seems to limit the API page calls (source). core.get_version should probably also be extended by a is_dev_build field to get around the ugly detection.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

This, unfortunately, doesn't help with the biggest offender - Ubuntu. I've heard their policy was to remove update notifications and such from programs, but I can't find a citation for this (although I have seen one in firefox). Maybe they won't notice? :D

@@ -286,6 +286,9 @@ void set_default_settings(Settings *settings)
settings->setDefault("mono_font_size", font_size_str);
settings->setDefault("contentdb_url", "https://content.minetest.net");

settings->setDefault("update_information_url", "https://api.github.com/repos/minetest/minetest/releases");

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Aug 8, 2018

Member

this is a bad idea, it would be better to put a file on minetest.net imo (or have minetest.net/api/releases redirect to the github API url)

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Aug 8, 2018

Author Member

Not redirect, but cache the JSON file.

$ curl -i https://api.github.com/repos/minetest/minetest/releases
HTTP/1.1 200 OK
Server: GitHub.com
Date: Wed, 08 Aug 2018 16:24:19 GMT
Content-Type: application/json; charset=utf-8
Content-Length: 92175
Status: 200 OK
X-RateLimit-Limit: 60
X-RateLimit-Remaining: 52  <-- issue is here
X-RateLimit-Reset: 1533746023

This comment has been minimized.

Copy link
@sfan5

sfan5 Aug 8, 2018

Member

Since the code barely uses any of the 92 KB that JSON file is big, we should definitely host a replacement ourselves.

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Aug 8, 2018

Member

Yeah, agreed. Probably easiest to just add a file directly to https://github.com/minetest/minetest.github.io

This comment has been minimized.

Copy link
@nerzhul

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Aug 9, 2018

Member

We should not rely on the GitHub API as we may not use it forever. We could use the Github API in Jekyll to automatically make a JSON file with the latest release, this would be future proof

@ClobberXD

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

It'd be nice if the update check interval can be selected from a pre-defined set of choices like so

  • 1 day
  • 1 week
  • 1 month
@Calinou

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

This, unfortunately, doesn't help with the biggest offender - Ubuntu. I've heard their policy was to remove update notifications and such from programs, but I can't find a citation for this (although I have seen one in firefox). Maybe they won't notice? :D

That is indeed the policy of most distributions (including Debian and Ubuntu). It makes sense from their point of view, as users are not supposed to download Minetest directly from minetest.net according to them.

It'd be nice if the update check interval can be selected from a pre-defined set of choices like so

Is it really necessary to add one more configuration option just for this? We have a lot of them already. I think it would be enough to have a single boolean that controls whether updates should be checked daily.

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

That is indeed the policy of most distributions (including Debian and Ubuntu). It makes sense from their point of view, as users are not supposed to download Minetest directly from minetest.net according to them.

It would be nice if they also didn't provide an outdated version of a networked program :/

@ClobberXD

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

Is it really necessary to add one more configuration option just for this? We have a lot of them already.

I feel that the user should have control over how often MT checks for updates, and I don't think one extra drop-down would do any harm. :)

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

I don't think there is a need for the setting, personally. It adds more complexity and more settings is more things to test

@nerzhul

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Just use the github last release API instead of full release API:

#7629

@rubenwardy

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

We should not rely on the GitHub API as we may not use it forever. We could use the Github API in Jekyll to automatically make a JSON file with the latest release, this would be future proof

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

"was found" somewhere in the dungeons of the internet?

@@ -22,7 +22,7 @@ local function version_info_formspec(data)
return (
"size[9,4.5,true]" ..
"textarea[0.5,0.5;8.5,3;;" ..
fgettext("A new Minetest version was found!") .. ";" ..
fgettext("New Minetest version is available") .. ";" ..

This comment has been minimized.

Copy link
@ClobberXD

ClobberXD Aug 17, 2018

Contributor

"A new Minetest version..." maybe? Sounds better and more like a full sentence... :)

@orwell96

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

The "never notify me again" tickbox is a bit misleading IMO, it should read "don't notify me on future updates" or so. I wouldn't expect from such a checkbox that it disabled notifications for future updates.
Also, update notifications should be disabled in builds that are neither run_in_place nor installed by the wix installer #6153, by setting the default setting this way

@ghost

This comment has been minimized.

Copy link

commented Sep 25, 2018

I think not good, I suggest : Add a news column in main menu is not possible ?

@SmallJoker

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2018

@orwell96

is a bit misleading IMO, it should read "don't notify me on future updates"

That's also a possibility, will adjust the PR accordingly.

update notifications should be disabled in builds that are neither run_in_place nor installed by the wix installer #6153, by setting the default setting this way

Why? Referring the users to the official download page covers all (or at least should) platforms which we support. Official repositories tend to be outdated quickly and people can tick the checkbox once on the initial startup to never get a notification again.

@Mrchiantos This is possible but wouldn't provide enough new and helpful information since the (half/)yearly updates would be likely the only content.

@orwell96

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

update notifications should be disabled in builds that are neither run_in_place nor installed by the wix installer #6153, by setting the default setting this way

Why? Referring the users to the official download page covers all (or at least should) platforms which we support. Official repositories tend to be outdated quickly and people can tick the checkbox once on the initial startup to never get a notification again.

You're right, that suggestion was bullsh*t...

@t4im

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

How about setting the defaults via cmake flag, with it defaulting to no updates?

  • During release builds you can flip this on, so that the distributed prebuild releases come with the check enabled
  • Distributions building their own releases can enable or disable updatechecks according to their policies
  • Anyone building a dev build from source would automatically skip it (if you build from git, you are already in full control of the version you are building anyway, especially if you intentionally build an older version for testing)

This would then work as wanted without depending on having the right minetest.conf ready or additional version information passed around.

@SmallJoker SmallJoker force-pushed the SmallJoker:update_info branch from 8b3fe58 to e2a1326 Oct 27, 2018

@SmallJoker SmallJoker force-pushed the SmallJoker:update_info branch from e2a1326 to 3f4effe Oct 28, 2018

@SmallJoker

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2018

@t4im Thanks for your suggestion. I added the CMake config UPDATE_CHECKER_DEFAULT which allows distributions to disable the checker by default. Defaulting to false for in-development builds seems however wrong, because there are people who use the Windows development builds from the forums or Linux users which take the binaries from the daily PPA.

@t4im

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2018

Well, anyone doing the windows development builds and daily ppa builds could flip this on as well.

The idea behind the default was that someone cloning the git repository to compile it themselves would have a default of no-updates without having to opt-out.
But anyone downloading a prebuild binary from someone else could have it enabled by the person doing the build (or more likely the CI system configuration) flipping the updates on.

This ensures that no one has to switch it on and off all the time (assuming CI systems being used for daily development builds)

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2018

BTW, what is the difference between "OK" and "Remind me later"?

@DS-Minetest

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

I think, "Ok" means something like "OK, I understood, don't tell me about this version again.".

Remove superfluous check
Co-Authored-By: SmallJoker <SmallJoker@users.noreply.github.com>
@nerzhul

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

i think it can be good to embed the update API in server list or another such program which will cache the github API calls for 1h or more, and control the backend API in case of at a point a migration outside github will be done (i don't say in the next year, but maybe at a point github will have too many problems).

@rubenwardy rubenwardy force-pushed the minetest:master branch 2 times, most recently from 0e3b135 to 39c54e1 Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.