Fix #7679: Check version when comparing scripts #7680
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
michicc
reviewed
Sep 26, 2020
| @@ -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()); | |||
There was a problem hiding this comment.
We've started using std::string, and using it here would probably be easier to understand.
TrueBrain
added
candidate: needs considering
|
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 :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Script version is not checked when scripts are compared.
This goes against documentation of the
importSquirrel procedure, explicitely stating: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:
or
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
importprocedure 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:importSquirrel procedure to support variable version(s) specification (like1.*or>=1 && <2,1.2+or>=1.2,<3)OpenTTD/src/script/script_scanner.cpp
Line 237 in afbf6a5
OpenTTD/src/network/network_content.cpp
Line 130 in afbf6a5
..., 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.