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

Fix #7679: Check version when comparing scripts #7680

Closed
wants to merge 1 commit into from

Conversation

@Berbe
Copy link
Contributor

@Berbe Berbe commented Aug 3, 2019

Problem

Script version is not checked when scripts are compared.

This goes against documentation of the import Squirrel procedure, explicitely stating:

The version check is very important. If you expect version 1, but on some users computer the library is in version 2, your AI will refuse to load. This is a good thing, as a new version means something changed with the existing functions, and your AI will most likely act up if you would use it. This early problem detection system should avoid many conflicts in the future.

Moreover this is a problem, as uneeded versions of libraries are downloaded for no reason, meaning extra work for no added value.

Dependencies are set for specific scripts, and the initial install does that properly. The use-case of issue #7679 where coming immediately back in the content downloader show available "upgrades" seems odd, as available content has not changed meanwhile.
It means either:

  • Said upgrades are should not be detected as such

or

  • The initial download should have taken care of them.

Both visions are developed hereafter.

Projection on a cleverer upgrade detection

Hence a question opens itself: How to define an upgrade?

This PR's proposal

Upgrade = new version of an existing dependency

What this PR does

Since scripts define exact versions of their dependencies, a change upstream will need to be reflected in the script to be usable.
Since the import procedure of Squirrel script has a rigid version specification, it seems an "upgrade" can only mean the same script, both in name & version, ie in pseudo code: oldScript.version == newScript.version && (oldScript.md5 != newScript.md5 || oldScript.shortName == newScript.shortName)

What would be left to do

In order to obtain a more logical newScript.IsUpgrade(oldScript) && (oldScript.md5 != newScript.md5 || oldScript.shortName == newScript.shortName) logic, it would require:

  1. The import Squirrel procedure to support variable version(s) specification (like 1.* or >=1 && <2, 1.2+ or >=1.2, <3)
  2. New logic in
    static bool IsSameScript(const ContentInfo *ci, bool md5sum, ScriptInfo *info, Subdirectory dir)
    to compare versions based of this new versions specification, on order for the call
    if (proc(ci, false)) ci->upgrade = true;
    to detect a new version of the same script

..., but that is another story.

Original thought?

Upgrade = non-downloaded versions of a downloaded script

This is closer to a Check for other versions, not even Check for newer versions as no version check was done at all.

If this vision is kept, it would be best if the button was reworded or its tooltip explanation & actual behaviour correlate.
It would then also be best to get all variants of a dependency on initial download, as it is inconsistent to immediately offer "upgrades" to scripts on the next visit of the content downloader, without available content having changed.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Sep 26, 2020

If I am reading this right, this would also not upgrade AIs and GSes. That seems unwanted. So I am not sure this is the correct approach to the problem. I will comment on the original issue further.

@@ -241,6 +241,10 @@ static bool IsSameScript(const ContentInfo *ci, bool md5sum, ScriptInfo *info, S
for (int j = 0; j < 4 && *str != '\0'; j++, str++) id |= *str << (8 * j);

if (id != ci->unique_id) return false;
char script_version[6];
memset(script_version, 0, sizeof(script_version));
seprintf(script_version, lastof(script_version) - 1, "%u", info->GetVersion());
Copy link
Member

@michicc michicc Sep 26, 2020

Choose a reason for hiding this comment

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

We've started using std::string, and using it here would probably be easier to understand.

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Dec 27, 2020

I do not think this is the correct solution for the problem. See #7679 for why I think this. Please feel free to reopen if you think it is, explaining a bit why (and addressing my earlier comment that this sounds to do more than it suggests :P)

Tnx :)

@TrueBrain TrueBrain closed this Dec 27, 2020
@Berbe Berbe deleted the content-download branch Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants