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

[WIP] add wesnoth_min_version #3576

Open
wants to merge 8 commits into
base: 1.14
from

Conversation

@gfgtdf
Copy link
Contributor

commented Sep 24, 2018

Addons can now specify a min_wesnoth_version attribute, addons with a
high min_wesnoth_version are hidden users with older wesnoth version in
the addon server, also users with older wesnoth wesnoth cannot join
addons with a high addon_min_version.

We need something like this if we want to add new apis to stable releases as we want to do, but also withotu new apis this can be useful if addons make heavily use of apis that are borken in older wesnoth version.

While this is a good starting step, it's not 100% clear how to continue: It wodul make sense to allow using new api only when the wesnoth_minversion attribute was specified accoringly, but that does no work in all cases, for example if a code woudl like to use wesnoth1.14.7 api but also has a fallback code for older api we shdoul allow that aswell.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

for example if a code woudl like to use wesnoth1.14.7 api but also has a fallback code for older api we shdoul allow that aswell.

In that case I think we should expect the author to set the min version to be whatever version is required for the fallback code. After all, that is the logical minimum version in this scenario; the author has simply added some runtime checks to switch to a different implementation on higher versions.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

yes that makes sense but on the other hand i'd like to force people to specify the correrct wesnoth_min_version, meaning for example assuming i'd want to add a wesnoth.set_dialog_tooltip function in 1.14.7 that sets the tooltip for a widget, i'd like to have:

int intf_set_dialog_tooltip (Lua_State*L )
{
  if(current.game.wesnoth.min_version < version_info("1.14.7")) {
    return error("this function requires api level 1.14.7");
  }
}

So that peopel really cannot use it unless the correct wesnoth_min_version is specified, Otherwise people might forget to set wesnoth_min_version and the addon will crash (attempt call nil value wesnoth.set_dialog_tooltip)

Now of course assuch a function is not vital for an addon people might write code like

if compare_version(wesnoth_version, >= "1.14.7") then
  set_dialog_tooltip(str, patch, to, widget)
end

which shoudl be allowed but will not work if i have the check in intf_set_dialog_tooltip. So my current idea is to have a function like switch_version calls its arguments ina context that can explicitly bypass that check like

wesnoth.switch_version("1.14.7", 
  function()
    -- 1.14.7 or later
    set_dialog_tooltip(str, patch, to, widget)
  end,
  funcion()
    -- 1.14.6 or older
  end)

But of yource that again woudl break on 1.14.4 or older (attempt call nil value wesnoth.switch_version), so i'm still thinking about a good solution.

@wesnoth wesnoth deleted a comment from codacy-bot Sep 24, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Sep 24, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Sep 25, 2018

@gfgtdf gfgtdf force-pushed the gfgtdf:wesnoth_min_version branch from 611b2c5 to 715b252 Sep 25, 2018

@wesnoth wesnoth deleted a comment from codacy-bot Sep 25, 2018

@gfgtdf gfgtdf force-pushed the gfgtdf:wesnoth_min_version branch from 9cfe992 to 2e31bfe Sep 25, 2018

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

I wonder whether we should try to keep the old addon version when the required wesnoth version was raised in an upload so that people that login with an older wesnoth version could then still download the old version.

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

I wonder whether we should try to keep the old addon version when the required wesnoth version was raised in an upload so that people that login with an older wesnoth version could then still download the old version.

Even if the minimum version didn't change it would be good to keep the old addon version available, in case there's a bug in the addon.

@wesnoth wesnoth deleted a comment from codacy-bot Sep 25, 2018

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

Even if the minimum version didn't change it would be good to keep the old addon version available, in case there's a bug in the addon.

i really think this is the authors reposibility, the user is usually not able to tell what the consequences would be of 'downgrading' an addon, a lot of campaigns saves are not 'forward compatible' meaning that you cannot load a version addonv1.4 save with the addonv1.3 addon installed.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

Random thought: Perhaps if this key is missing it should assumed to be 1.14.0. (Or possibly 1.14.5?)

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

Random thought: Perhaps if this key is missing it should assumed to be 1.14.0. (Or possibly 1.14.5?)

well currently it defaults to 0.0.0 which gives just the same result (all known versions can install that addon).

@@ -1027,6 +1027,7 @@ void server::handle_upload(const server::request& req)


std::string req_w_version;
const std::string& lc_name = utf8::lowercase(name);

This comment has been minimized.

Copy link
@GregoryLundberg

GregoryLundberg Sep 26, 2018

Contributor

This smells like a leak. You sure it's safe?

This comment has been minimized.

Copy link
@gfgtdf

gfgtdf Sep 26, 2018

Author Contributor

this was basicially copied from like 807 of the same file, const references extend lifetime in some cases so its valid, but i agree that its clearer if one just writed std::string for such cases.

@gfgtdf gfgtdf force-pushed the gfgtdf:wesnoth_min_version branch from 16d503e to 25f4b39 Sep 26, 2018

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

I'm not sure I see this getting much use, given that it's an optional key that the user themselves would have to set, which then probably means that the large majority of people won't even know it exists (and many who do won't be bothered to keep track of what their add-on should be setting it as)

If this is only meant to allow the implementation of entirely new APIs, then I think a better solution would be to require new additions to the API to declare their implementation version and then automatically detect what the highest version used in each add-on is.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

I'm not sure I see this getting much use, given that it's an optional key that the user themselves would have to set

Well yes, the idea is to make the new api unavailable unless the required_wesnoth_version attribute is specified accoringly. This might not be possible for all apis and has some problems in the detail but i think the majority of the cases this can be done.

then automatically detect what the highest version used in each add-on is.

how would that work exactly?

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

Maybe I'm misunderstanding then, but

well currently it defaults to 0.0.0 which gives just the same result (all known versions can install that addon).

sounds like when not provided it assumes that any version can install/play the add-on, which would mean that by default nothing would be restricted, right?

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

sounds like when not provided it assumes that any version can install/play the add-on

yes

which would mean that by default nothing would be restricted, right?

no, see my first and second post.

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

So, assuming 1.14.6 is the first of a new rolling release schedule, if wesnoth_min_version is not provided, then it defaults to "0.0.0", which means the add-on has access to APIs available in 1.14.5 and earlier?

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

yes exactly, note however that there will likeley be exceptions to this:

  1. features that will not 'break' on old versions and will just be ignored on old versions without causing much problems will probably work unrelated to the specified apilevel.

  2. some some features it might be technicially too hard to really implement the check (don't know a concreate example, just saying that it could happen)

Also note that when multiple addons work together the engine is not able to make a differnce which addon used that api, so if for example if you have a scenario that uses apilevel 1.14.5 together with a [modification] that uses apilevel 1.14.9 all the code inclusing that that come from the scenario will have acess to apilevel 1.14.9 during that game.

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Sep 29, 2018

Also note that when multiple addons work together the engine is not able to make a differnce which addon used that api, so if for example if you have a scenario that uses apilevel 1.14.5 together with a [modification] that uses apilevel 1.14.9 all the code inclusing that that come from the scenario will have acess to apilevel 1.14.9 during that game.

Would it not make more sense to prevent those add-ons from being used together at all?

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

Would it not make more sense to prevent those add-ons from being used together at all?

Hmm nbo i think that disallowing that woudl be too restrictive and too annyoing for the users while the things i mentioned in my post is anot a real problem since the apis are fully backwards compatible.

gfgtdf added some commits Sep 24, 2018

add min_wesnoth_version
Addons can now specify a min_wesnoth_version attribute, addons with a
high min_wesnoth_version are hidden users with older wesnoth version in
the addon server, also users with older wesnoth wesnoth cannot join
addons with a high addon_min_version.
add wesnoth.switch_version
for versions after 1.14.7 wesnoth.switch_version offers a way to safely
call function that require a higher api level than specified in
wesnoth_min_version. This makes sense when you want to use a newer api
function, but also have a fallback for older wesnoth versions.
try to detect umc versions checks
now the game attepts to automatically detect 'good' umc code that check
for newer wesnoth versions before attempting to use new api
```
if wesnoth.compare_version(game_coinfig.version, ">=", "1.14.9") then
.. use new 1.14.9 api
end
```
and raise the api level in that case. of course this does not work
perfectly and wrongly detect version checks when when there are none, in
particular theere is no way to detect leaving that 'if' or whether there
is an 'if' in the first place, so player could in theory still use the
new api after they leave that 'if' . The better solution which is the
`wesnoth.switch_version` function  obviously does not work in for api
that is new in 1.14.6 becasue that function does not exists in 1.14.5 or
older. hopefully at some point, probably at wesnoth 1.15 we can remove
this hack so that people will use wesnoth.switch_version() instead.
addonserver: keep old version when an addon raises apilevel
now the addon server keeps the old versions of a addon when te author
uploads a new version that raises the minimum required wesnoth version
to run the addon so ther older clients can still download the older
addon version.

Doesn't work completely yet, in particular some admin commans of the
addonserver might not work anymore..
add default ctors to onfig iterators
this is a required for forward iterators and some algorithms that work
on forward iterators require this.

@gfgtdf gfgtdf force-pushed the gfgtdf:wesnoth_min_version branch from 25f4b39 to 72b8973 Oct 9, 2018

@GregoryLundberg GregoryLundberg added this to the 1.16.0 milestone Oct 18, 2018

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