feat(GameIdentifier): include the engine version #647

keturn · 2021-08-01T04:11:41Z

Adds the Terasology engine version to GameIdentifier.

As discussed for #646.

keturn · 2021-08-01T04:14:22Z

keturn · 2021-08-01T04:21:14Z

There are a few very shady-looking FIXMEs here where there's code that tries to rebuild a GameIdentifier from a single string.

skaldarnar · 2021-08-01T09:49:50Z

src/main/java/org/terasology/launcher/model/GameIdentifier.java

@@ -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!


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.

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).

This will break with existing installations of nightly builds 😕 alpha-20+150 is not a valid semver, and this will just fail...

... 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...

nah, it'd break on new installs too, if they're still using displayVersion but not engineVersion in GameIdentifier.toString

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.

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.

skaldarnar · 2021-08-01T19:02:12Z

src/main/java/org/terasology/launcher/repositories/JenkinsRepositoryAdapter.java

-                    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);


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.

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.

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 🤔

skaldarnar · 2021-08-01T19:06:11Z

src/main/java/org/terasology/launcher/game/GameManager.java

+                        // FIXME: Assumes version==engineVersion. Should we pull engineVersion from the installation's
+                        //   files instead of the name of its directory?


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

The engine version is encoded in libs/engine-4.3.0-SNAPSHOT.jar, but parsing it from that also sounds horrible...

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.

Implemented. (I think. Needs some manual testing because I don't have integration tests involving actual release files.)

Added a comment about this in my review.

…ng an exception

…insRepositoryAdapter

Instead of relying on the directory names.

keturn · 2021-08-04T21:27:50Z

There are a few refactorings here I could pull out to separate PRs, if that would help:

031232d: GameManager#scanInstallationDir to use java.nio APIs
49a6834: Moving the versionInfo.properties loader from JenkinsRepositoryAdapter to GameIdentifier

src/main/java/org/terasology/launcher/game/GameManager.java

src/main/java/org/terasology/launcher/model/GameIdentifier.java

src/main/java/org/terasology/launcher/game/GameManager.java

skaldarnar · 2021-08-06T09:39:34Z

src/main/java/org/terasology/launcher/game/GameManager.java

+    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


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?

skaldarnar · 2021-08-06T09:53:36Z

src/main/java/org/terasology/launcher/model/GameIdentifier.java

@@ -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!


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.

skaldarnar

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/.

…ot be parsed

keturn

Thank you for filling in the method documentation, @skaldarnar! LGTM

… displayVersion largely reverts e1e530f (#647)

feat(GameIdentifier): include the engine version

116557b

skaldarnar reviewed Aug 1, 2021

View reviewed changes

keturn added 5 commits August 2, 2021 18:09

fix(JenkinsRepositoryAdapter): guard against Optional<> method throwi…

3ba8e70

…ng an exception

refactor(GameManager): use new file API

031232d

fixup! refactor(GameManager): use new file API

85c7ec8

refactor(GameIdentifier): factor out versionInfo constructor for Jenk…

49a6834

…insRepositoryAdapter

feat(GameManager): read version info out of installed files

291f7ab

Instead of relying on the directory names.

keturn added 4 commits August 4, 2021 14:50

fixup! feat(GameManager): read version info out of installed files

2b3bb5f

chore(settings)! use Json serialization instead of regex

fd0570d

chore(GameIdentifier): update toString to include engineVersion

cc4cbbd

fixup! feat(GameManager): read version info out of installed files

ddae77c

keturn commented Aug 5, 2021

View reviewed changes

src/main/java/org/terasology/launcher/game/GameManager.java Show resolved Hide resolved

src/main/java/org/terasology/launcher/model/GameIdentifier.java Outdated Show resolved Hide resolved

src/main/java/org/terasology/launcher/game/GameManager.java Show resolved Hide resolved

skaldarnar reviewed Aug 6, 2021

View reviewed changes

skaldarnar requested changes Aug 6, 2021

View reviewed changes

skaldarnar added 4 commits August 7, 2021 20:42

doc: add docstring for helper methods in GameManager

4d8f660

doc: add docstring for GameIdentifier#fromVersionInfo

27b4d9e

chore: reduce logging level to WARN when "lastPlayedGameVersion" cann…

2a58f6e

…ot be parsed

skaldarnar approved these changes Aug 7, 2021

View reviewed changes

keturn commented Aug 7, 2021

View reviewed changes

keturn merged commit e1e530f into master Aug 7, 2021

keturn deleted the feat/engineVersion branch August 7, 2021 20:01

keturn mentioned this pull request Aug 7, 2021

feat: update command line options for Terasology #646

Merged

3 tasks

keturn added a commit that referenced this pull request Aug 9, 2021

chore(GameIdentifier): remove engineVersion; rename String version to…

3e6d046

… displayVersion largely reverts e1e530f (#647)

keturn mentioned this pull request Aug 9, 2021

chore(GameIdentifier): remove engineVersion; rename String version to displayVersion #655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(GameIdentifier): include the engine version #647

feat(GameIdentifier): include the engine version #647

keturn commented Aug 1, 2021 •

edited

Loading

keturn commented Aug 1, 2021

keturn commented Aug 1, 2021

skaldarnar Aug 1, 2021

keturn Aug 1, 2021

skaldarnar Aug 1, 2021

skaldarnar Aug 1, 2021

keturn Aug 1, 2021

keturn Aug 4, 2021

skaldarnar Aug 6, 2021

skaldarnar Aug 1, 2021

keturn Aug 3, 2021

skaldarnar Aug 6, 2021

skaldarnar Aug 1, 2021

skaldarnar Aug 1, 2021

keturn Aug 1, 2021

keturn Aug 5, 2021

skaldarnar Aug 6, 2021

keturn commented Aug 4, 2021

skaldarnar Aug 6, 2021

skaldarnar Aug 6, 2021

skaldarnar left a comment

keturn left a comment

		// FIXME: Assumes version==engineVersion. Should we pull engineVersion from the installation's
		// files instead of the name of its directory?

feat(GameIdentifier): include the engine version #647

feat(GameIdentifier): include the engine version #647

Conversation

keturn commented Aug 1, 2021 • edited Loading

keturn commented Aug 1, 2021

keturn commented Aug 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skaldarnar left a comment

Choose a reason for hiding this comment

keturn left a comment

Choose a reason for hiding this comment

keturn commented Aug 1, 2021 •

edited

Loading