-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat(GameIdentifier): include the engine version #647
Conversation
There are a few very shady-looking FIXMEs here where there's code that tries to rebuild a GameIdentifier from a single string. |
@@ -81,7 +85,8 @@ public static GameIdentifier fromString(String identifier) { | |||
final Build build = Build.valueOf(matcher.group("build")); | |||
final Profile profile = Profile.valueOf(matcher.group("profile")); | |||
final String version = matcher.group("version"); | |||
return new GameIdentifier(version, build, profile); | |||
Semver engineVersion = new Semver(version, Semver.SemverType.IVY); // FIXME: assumes engineVersion==version! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we do this?
For backwards compatibility, we would add another pattern (or adjust the one in https://github.com/MovingBlocks/TerasologyLauncher/pull/647/files#diff-4b5886892dcec7bb3c50a372d9c03de38e2ddbc2714cae8f73ce8894c28e6aafL24) to also look for engineVersion
. If it exists, take it, otherwise fall back to assume engineVersion == version
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GameIdentifier.fromString has a comment saying
TODO: move deserialization somewhere else (probably to LauncherSettings?)
It's used by BaseLauncherSettings.getLastPlayedGameVersion, and takes its value from an entry in the settings file (stored in Java properties format).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break with existing installations of nightly builds 😕 alpha-20+150
is not a valid semver, and this will just fail...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... but on the other hand, re-installing the same game should work, right? 🤔 so, we'd need a way to at least inform the user that they should un-install and install again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, it'd break on new installs too, if they're still using displayVersion
but not engineVersion
in GameIdentifier.toString
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed BaseLauncherSettings to use Gson instead of this fromString method.
I did not include backwards compatibility for the old (non-json) value of this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This results in an ERROR being logged, even though it does not crash the launcher - would log level WARN be enough?
A user may see this error multiple times, but on the upside it will be automatically fixed when the user starts a (different?) game 👍 Afterwards, the error logs are gone, so I think not being backwards compatible here is okay.
String versionString = displayVersion + "+" + jenkinsBuildInfo.number; | ||
return new GameIdentifier(versionString, buildProfile, profile); | ||
String buildVersion = displayVersion + "+" + jenkinsBuildInfo.number; | ||
Semver engineVersion = new Semver(versionInfo.getProperty("engineVersion"), Semver.SemverType.IVY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will throw an exception if engineVersion
is not present in the properties file, or if it is present but no valid semver.
The method contract is based on an Optional
- either you get a valid identifier or an empty optional, but the call-side is not prepared for an exception. To bring it in line with the current code we should surround this with a try-catch to return null
in case of error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every time I hear this I think it makes Optional
worse than @Nullable
. We got a Result<T, Exception>
type yet?
Actually, client.requestProperties
looks like it's asynchronous, so Mono<GameIdentifier>
would be appropriate here.
anyway. I've pushed a change to avoid the exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a Result
or Either
in plain Java, but it's added by numerous functional-style libraries, I think...
Probably making use of ReactiveX Mono
is not such a bad call 🤔
// FIXME: Assumes version==engineVersion. Should we pull engineVersion from the installation's | ||
// files instead of the name of its directory? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the version written in the installation files?
There's a VERSION
file, but the content does not look very promising...
Terasology - Version
====================
Jenkins:
URL: http://jenkins.terasology.io/teraorg/job/Terasology/job/engine/job/develop/399/
Build number: 399
Created at: 2021-03-12T21:35:42Z
http://terasology.org
https://github.com/MovingBlocks/Terasology
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The engine version is encoded in libs/engine-4.3.0-SNAPSHOT.jar
, but parsing it from that also sounds horrible...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that VERSION file is more for humans than something intended to be machine-readable.
I checked, and I think we do currently do not have any terasology --version
command that shows version information.
A thing that is kind of gross, but at least has the advantage of being something unambiguous, is to extract the versionInfo.properties from inside that engine.jar
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented. (I think. Needs some manual testing because I don't have integration tests involving actual release files.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment about this in my review.
…insRepositoryAdapter
Instead of relying on the directory names.
src/main/java/org/terasology/launcher/model/GameIdentifier.java
Outdated
Show resolved
Hide resolved
private static Properties getVersionPropertiesFromJar(Path jarLocation) throws IOException { | ||
try (var jar = new JarFile(jarLocation.toFile())) { | ||
var versionEntry = jar.stream().filter(entry -> | ||
entry.getName().equals("versionInfo.properties") // FIXME: use const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got some older installations in my testing workspace, and this will throw an error, even though the version info file is found.
10:14:56.283 [Launcher init thread] ERROR o.t.launcher.game.GameManager - Error while looking for version in /home/skaldarnar/Code/movingblocks/terasologylauncher/build/classes/games/OMEGA/NIGHTLY/alpha-20+150/libs/engine-4.4.0-SNAPSHOT.jar.
java.io.FileNotFoundException: Found no versionInfo.properties in /home/skaldarnar/Code/movingblocks/terasologylauncher/build/classes/games/OMEGA/NIGHTLY/alpha-20+150/libs/engine-4.4.0-SNAPSHOT.jar
at org.terasology.launcher.game.GameManager.getVersionPropertiesFromJar(GameManager.java:217)
at org.terasology.launcher.game.GameManager.getInstalledVersion(GameManager.java:190)
at org.terasology.launcher.game.GameManager.getInstalledVersion(GameManager.java:167)
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177)
at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177)
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
at org.terasology.launcher.game.GameManager.scanInstallationDir(GameManager.java:148)
at org.terasology.launcher.game.GameManager.<init>(GameManager.java:49)
at org.terasology.launcher.LauncherInitTask.call(LauncherInitTask.java:95)
at org.terasology.launcher.LauncherInitTask.call(LauncherInitTask.java:42)
at javafx.graphics/javafx.concurrent.Task$TaskCallable.call(Task.java:1425)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.lang.Thread.run(Thread.java:832)
There is a versionInfo.properties
file in alpha-20+150/libs/engine-4.4.0-SNAPSHOT.jar
, located under /org/terasology/version/
. As the entry names contain the full path to the file it is not matched by entry.getName().equals("versionInfo.properties")
. Should we rather use endsWith(...)
here, or split the path and only check for the actual file name?
Or should we try to figure out which locations we might have used in the past to restrict it to those specifically?
@@ -81,7 +85,8 @@ public static GameIdentifier fromString(String identifier) { | |||
final Build build = Build.valueOf(matcher.group("build")); | |||
final Profile profile = Profile.valueOf(matcher.group("profile")); | |||
final String version = matcher.group("version"); | |||
return new GameIdentifier(version, build, profile); | |||
Semver engineVersion = new Semver(version, Semver.SemverType.IVY); // FIXME: assumes engineVersion==version! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This results in an ERROR being logged, even though it does not crash the launcher - would log level WARN be enough?
A user may see this error multiple times, but on the upside it will be automatically fixed when the user starts a (different?) game 👍 Afterwards, the error logs are gone, so I think not being backwards compatible here is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented, this does not work for older releases, e.g., "alpha-20+150" with engine-4.4.0-SNAPSHOT
. I think we moved the versionInfo.properties
file at some point and we need to take that into account here.
In older releases of the Terasology engine the `versionInfo.properties` file was not located at the root of the JAR but somewhere else. For instance, there is a `versionInfo.properties` file in `engine-4.4.0-SNAPSHOT.jar` located under /org/terasology/version/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for filling in the method documentation, @skaldarnar! LGTM
Adds the Terasology engine version to GameIdentifier.
As discussed for #646.